[ISSUE #54 #50] Operator multiple mq cluster && start sequence by linjiemiao · Pull Request #56 · apache/rocketmq-operator

linjiemiao · 2020-09-22T06:43:58Z

ensure rocketmq-operator can operator more than one rocketmq cluster;
make sure nameserver must ready before broker cluster.

2. make sure nameserver must ready before broker cluster.

liuruiyiyang · 2020-09-24T03:43:36Z

  size: 1
  # nameServers is the [ip:port] list of name service
  nameServers: ""
+  # rocketMQName is the rocketmq name, must equal to nameserver.spec.rocketMQName and topictransfer.spec.rocketMQName


We can not ensure users set insistent name correctly, is there a better way to do this?

I did this because we don't have a rocketmq resource as the parent resource of the broker and nameserver. Without a common parent controller, we can only specify the connection of the child resource in the spec. So I hope to add a rocketmq api.

In that case should we add rocketmq higher level api before this PR?

drivebyer · 2023-08-11T02:58:14Z

still need this

RockteMQ-AI · 2026-07-30T10:40:50Z

⚠️ Merge conflict detected

This PR has conflicts with the base branch and cannot be merged. Please rebase or merge the base branch into your branch and resolve the conflicts:

git fetch origin
git checkout issue-54-codereview
git rebase origin/main
# resolve conflicts, then:
git push --force-with-lease

This is a one-time reminder. Feel free to @mention me for a re-review after conflicts are resolved.

Automated notification by github-manager-bot

RockteMQ-AI

Summary

This PR modifies 19 file(s) with 702 lines of diff. No test changes detected — consider adding test coverage.

Automated review by github-manager-bot

Additional notes (not anchored to a changed line)

[INFO] README.md:1 — Large diff (702 lines). Consider breaking into smaller, focused PRs for easier review. (line outside diff)

RockteMQ-AI · 2026-08-05T14:58:32Z

@@ -27,7 +27,6 @@ import (
 	"github.com/apache/rocketmq-operator/pkg/apis"


No test changes detected alongside source modifications. Consider adding tests to cover the changes.

RockteMQ-AI

Summary

PR received and logged for review. This PR requires detailed code review by a maintainer.

Diff size: 702 lines
Author: linjiemiao (NONE)

Automated review by RockteMQ-AI

RockteMQ-AI

Summary

Review of PR #56: [ISSUE #54 #50] Operator multiple mq cluster && start sequence

Findings: 15 issue(s) identified (4 critical).
CLA: unknown

Please address the inline comments above.

Automated review by github-manager-bot

RockteMQ-AI · 2026-08-23T20:04:16Z

+}
+
+func (sMap *ItemSyncMap) LoadOrStore(key string, value ShareItem) (actual ShareItem, loaded bool) {
+	a, loaded := sMap.m.LoadOrStore(key, value)


LoadOrStore will panic if the key exists and the stored value is nil, because a.(ShareItem) is an unconditional type assertion. Although ShareItem is a struct (not a pointer), any future refactor to a pointer type would cause a nil panic. More critically, if the underlying sync.Map somehow stores a non-ShareItem value (e.g., due to a bug), this will panic at runtime. Use a safe assertion: actual, _ = a.(ShareItem) to avoid panic.

RockteMQ-AI · 2026-08-23T20:04:16Z

 		return reconcile.Result{}, err
 	}

+	actualKey := broker.Namespace + "-" + broker.Spec.RocketMQName


The broker reconcile loop spins in a tight busy-wait (for { if actual.IsNameServersStrInitialized { break } else { time.Sleep(...) } }) on the controller goroutine. This blocks the entire reconcile goroutine indefinitely, preventing any other reconcile requests from being processed for this controller. This should be replaced with a requeue-based approach (return reconcile.Result{Requeue: true, RequeueAfter: ...}) so the controller manager can continue handling other events.

RockteMQ-AI · 2026-08-23T20:04:16Z

 		return reconcile.Result{}, err
 	}

+	actualKey := broker.Namespace + "-" + broker.Spec.RocketMQName


Race condition: actual is loaded from the sync map into a local variable at line ~134, then the busy-wait loop re-loads actual from the map inside the loop body, but the outer defer at line ~139 always stores the local actual variable back on function exit. If actual.IsNameServersStrInitialized becomes true during the wait loop (set by the nameservice controller), the defer will overwrite the map with the snapshot captured at loop-entry time, potentially clobbering fields like NameServersStr that the nameservice controller wrote.

RockteMQ-AI · 2026-08-23T20:04:16Z

@@ -189,8 +201,8 @@ func (r *ReconcileBroker) Reconcile(request reconcile.Request) (reconcile.Result
 	// Check for name server scaling
 	if broker.Spec.AllowRestart {
 		// The following code will restart all brokers to update NAMESRV_ADDR env


Inconsistent indentation in the if actual.IsNameServersStrUpdated block: the inner for loop is indented with extra tabs compared to surrounding code. This is a minor formatting issue but indicates the code may not have been run through gofmt, which can cause CI lint failures.

RockteMQ-AI · 2026-08-23T20:04:16Z

@@ -253,10 +265,17 @@ func (r *ReconcileBroker) Reconcile(request reconcile.Request) (reconcile.Result
 	podNames := getPodNames(podList.Items)


After the early return when len(podNames) == 0, the reconcile returns without storing the updated actual back to the sync map (the defer will still run, but actual.GroupNum and actual.BrokerClusterName will have been set just before this point). However, actual.NameServersStr may not be populated yet if nameServers is empty and the wait loop hasn't run. The defer stores a potentially incomplete actual, which could overwrite a valid previously-stored value if the nameservice controller already populated it.

RockteMQ-AI · 2026-08-23T20:04:17Z

 	sourceCluster := topicTransfer.Spec.SourceCluster

-	nameServer := strings.Split(share.NameServersStr, ";")[0]
+	actualKey := topicTransfer.Namespace + "-" + topicTransfer.Spec.RocketMQName


strings.Split(actual.NameServersStr, ";")[0] will return an empty string (not panic) if actual.NameServersStr is empty, and the subsequent len(nameServer) < cons.MinIpListLength check handles that. However, if actual was just default-initialized by LoadOrStore (i.e., the nameservice for this rocketMQName has not yet reconciled), the TopicTransfer will silently terminate rather than requeue with an informative error. Consider returning a requeue result instead of terminating.

RockteMQ-AI · 2026-08-23T20:04:17Z

 	SourceCluster string `json:"sourceCluster,omitempty"`
 	// The cluster where the topic will be transferred to
 	TargetCluster string `json:"targetCluster,omitempty"`
+	// 	// RocketMQ Name, the broker and nameserver in the same cluster must be filled with the same name


Double comment marker on line 40: // // RocketMQ Name, ... — there is a tab and an extra // before the actual comment text. This is a copy-paste artifact and should be // RocketMQ Name, ....

RockteMQ-AI · 2026-08-23T20:04:17Z

@@ -93,6 +96,7 @@ spec:
          - volumes


rocketMQName is now a required field in the CRD. This is a breaking change for any existing Broker CR that does not have this field set. Existing clusters that upgrade to this operator version will have their Broker CRs fail validation. A defaulting webhook or a non-required field with a documented migration path is needed for safe upgrades.

RockteMQ-AI · 2026-08-23T20:04:17Z

@@ -73,6 +76,7 @@ spec:
          - storageMode


Same breaking change as the Broker CRD: making rocketMQName required on NameService CRD will invalidate all existing NameService CRs on upgrade. Needs a migration strategy.

RockteMQ-AI · 2026-08-23T20:04:17Z

@@ -100,7 +93,6 @@ func main() {



Removing Namespace from manager.Options changes the operator from namespace-scoped to cluster-scoped watching. This is intentional (given the new ClusterRole), but it is a significant behavioral change: the operator will now watch all namespaces, increasing API server load and requiring the new ClusterRole/ClusterRoleBinding to be applied. This should be explicitly documented in the PR and migration notes, and the old namespace-scoped Role/RoleBinding files should be deprecated or removed to avoid confusion.

linjiemiao added 2 commits September 22, 2020 14:40

1. ensure rocketmq-operator can operator more than one rocketmq cluster.

a4cf865

2. make sure nameserver must ready before broker cluster.

1. fixed bug, if nameserver.spec.size is 0, need requeue

93a9383

liuruiyiyang changed the title ~~【ISSUE #54 #50】Operator multiple mq cluster && start sequence~~ [ISSUE #54 #50] Operator multiple mq cluster && start sequence Sep 22, 2020

liuruiyiyang assigned liuruiyiyang and unassigned liuruiyiyang Sep 22, 2020

liuruiyiyang added the enhancement New feature or request label Sep 22, 2020

liuruiyiyang reviewed Sep 24, 2020

View reviewed changes

RockteMQ-AI reviewed Aug 5, 2026

View reviewed changes

RockteMQ-AI reviewed Aug 20, 2026

View reviewed changes

RockteMQ-AI suggested changes Aug 23, 2026

View reviewed changes

		@@ -27,7 +27,6 @@ import (
		"github.com/apache/rocketmq-operator/pkg/apis"

		@@ -253,10 +265,17 @@ func (r *ReconcileBroker) Reconcile(request reconcile.Request) (reconcile.Result
		podNames := getPodNames(podList.Items)

Uh oh!

Conversation

linjiemiao commented Sep 22, 2020

Uh oh!

liuruiyiyang Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drivebyer commented Aug 11, 2023

Uh oh!

RockteMQ-AI commented Jul 30, 2026

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Additional notes (not anchored to a changed line)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

liuruiyiyang Sep 24, 2020 •

edited

Loading