[ISSUE #65]Add RocketMQ-on-DLeger group mode deploy - #87
Conversation
caigy
changed the title
|
I see there are some codes conflicts in this pr, can you resolve this pr in your local enviroments? |
|
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 master
git rebase origin/main
# resolve conflicts, then:
git push --force-with-leaseThis 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
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
This PR modifies 16 file(s) with 2427 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 (2427 lines). Consider breaking into smaller, focused PRs for easier review. (line outside diff)
| @@ -34,14 +34,18 @@ type BrokerSpec struct { | |||
| Size int `json:"size"` | |||
There was a problem hiding this comment.
No test changes detected alongside source modifications. Consider adding tests to cover the changes.
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
PR received and logged for review. This PR requires detailed code review by a maintainer.
Diff size: 2427 lines
Author: haifzhu (NONE)
Automated review by RockteMQ-AI
| - brokerImage | ||
| - imagePullPolicy | ||
| - nameServers | ||
| - allowRestart |
There was a problem hiding this comment.
Adding enableDLeger to the required list is a breaking API change for the served v1alpha1 Broker CRD. Existing Broker CRs that do not specify this field will fail validation on updates, and the schema does not declare a default. The field should be optional (removed from required) to preserve backward compatibility.
| size: 1 | ||
| # nameServers is the [ip:port] list of name service | ||
| nameServers: "" | ||
| # Whether enable rocketmq-on-dleger group deploy |
There was a problem hiding this comment.
README documents enableDLeger: false as optional with an implicit default, but the CRD schema marks the field as required and does not declare a default value. The documented behavior does not match the schema.
| nameServers: | ||
| description: NameServers defines the name service list e.g. 192.168.1.1:9876;192.168.1.2:9876 | ||
| type: string | ||
| enableDLeger: |
There was a problem hiding this comment.
The diff only updates the CRD schema and README; no controller, StatefulSet generation, or broker configuration code is modified to consume the new enableDLeger field. As submitted, the field has no effect and the RocketMQ-on-DLeger deployment mode is not implemented.
| - brokerImage | ||
| - imagePullPolicy | ||
| - nameServers | ||
| - allowRestart |
There was a problem hiding this comment.
No controller tests, reconciliation tests, or RBAC changes are included for the new DLeger mode, affinity, or tolerations support. A feature affecting broker topology and scheduling should have test coverage and any required RBAC updates.
4 participants
The Commit is added to add RocketMQ-on-DLeger group mode deploy.