RockteMQ-AI commented on code in PR #87:
URL: https://github.com/apache/rocketmq-operator/pull/87#discussion_r3839455765
##########
README.md:
##########
@@ -187,6 +187,8 @@ spec:
size: 1
# nameServers is the [ip:port] list of name service
nameServers: ""
+ # Whether enable rocketmq-on-dleger group deploy
Review 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.
##########
deploy/crds/rocketmq_v1alpha1_broker_crd.yaml:
##########
@@ -59,6 +61,9 @@ spec:
nameServers:
description: NameServers defines the name service list e.g.
192.168.1.1:9876;192.168.1.2:9876
type: string
+ enableDLeger:
Review 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.
##########
deploy/crds/rocketmq_v1alpha1_broker_crd.yaml:
##########
@@ -95,24 +841,26 @@ spec:
type: object
type: array
required:
- - size
- - replicaPerGroup
- - brokerImage
- - imagePullPolicy
- - nameServers
- - allowRestart
- - resources
- - storageMode
- - hostPath
- - env
- - volumes
- - volumeClaimTemplates
- - scalePodName
+ - size
+ - enableDLeger
+ - replicaPerGroup
+ - brokerImage
+ - imagePullPolicy
+ - nameServers
+ - allowRestart
Review 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.
##########
deploy/crds/rocketmq_v1alpha1_broker_crd.yaml:
##########
@@ -95,24 +841,26 @@ spec:
type: object
type: array
required:
- - size
- - replicaPerGroup
- - brokerImage
- - imagePullPolicy
- - nameServers
- - allowRestart
- - resources
- - storageMode
- - hostPath
- - env
- - volumes
- - volumeClaimTemplates
- - scalePodName
+ - size
+ - enableDLeger
+ - replicaPerGroup
+ - brokerImage
+ - imagePullPolicy
+ - nameServers
+ - allowRestart
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
