caigy commented on code in PR #183:
URL: https://github.com/apache/rocketmq-operator/pull/183#discussion_r1487485076
##########
pkg/controller/nameservice/nameservice_controller.go:
##########
@@ -216,17 +229,40 @@ func (r *ReconcileNameService)
updateNameServiceStatus(instance *rocketmqv1alpha
}
// use admin tool to update broker config
- if share.IsNameServersStrUpdated && (len(oldNameServerListStr)
> cons.MinIpListLength) && (len(share.NameServersStr) > cons.MinIpListLength) {
+ if isNameServersStrUpdated && (len(oldNameServerListStr) >
cons.MinIpListLength) && (len(newNameServerListStr) > cons.MinIpListLength) {
+ // bash-4.4$ ./mqadmin clusterList -n
192.168.180.36:9876
+ // #Cluster Name #Broker Name #BID
#Addr #Version #InTPS(LOAD) #OutTPS(LOAD)
#PCWait(ms) #Hour #SPACE
+ // broker broker-0 0
192.168.180.40:10911 V4_5_0 0.00(0,0ms) 0.00(0,0ms)
0 471030.34 -1.0000
+ // broker broker-0 1
192.168.137.89:10911 V4_5_0 0.00(0,0ms) 0.00(0,0ms)
0 471030.34 0.2673
+ clusterListCmd := exec.Command("sh", cons.AdminToolDir,
cons.ClusterList, "-n", oldNameServerListStr)
Review Comment:
As `isNameServersStrUpdated` is true, why using `oldNameServerListStr` as
the addresses of name servers?
##########
pkg/controller/nameservice/nameservice_controller.go:
##########
@@ -68,6 +67,19 @@ func newReconciler(mgr manager.Manager) reconcile.Reconciler
{
// add adds a new Controller to mgr with r as the reconcile.Reconciler
func add(mgr manager.Manager, r reconcile.Reconciler) error {
+ err := mgr.GetFieldIndexer().IndexField(context.TODO(),
&rocketmqv1alpha1.NameService{},
rocketmqv1alpha1.NameServiceRocketMqNameIndexKey,
+ func(rawObj client.Object) []string {
+ n, ok := rawObj.(*rocketmqv1alpha1.NameService)
+ if !ok {
+ return nil
+ }
+ return []string{n.Spec.RocketMqName + "-" + n.Namespace}
+ },
+ )
+ if err != nil {
+ return err
+ }
Review Comment:
What's the usage of this block? You'd better show more about your design.
##########
pkg/controller/nameservice/nameservice_controller.go:
##########
@@ -216,17 +229,40 @@ func (r *ReconcileNameService)
updateNameServiceStatus(instance *rocketmqv1alpha
}
// use admin tool to update broker config
- if share.IsNameServersStrUpdated && (len(oldNameServerListStr)
> cons.MinIpListLength) && (len(share.NameServersStr) > cons.MinIpListLength) {
+ if isNameServersStrUpdated && (len(oldNameServerListStr) >
cons.MinIpListLength) && (len(newNameServerListStr) > cons.MinIpListLength) {
+ // bash-4.4$ ./mqadmin clusterList -n
192.168.180.36:9876
+ // #Cluster Name #Broker Name #BID
#Addr #Version #InTPS(LOAD) #OutTPS(LOAD)
#PCWait(ms) #Hour #SPACE
+ // broker broker-0 0
192.168.180.40:10911 V4_5_0 0.00(0,0ms) 0.00(0,0ms)
0 471030.34 -1.0000
+ // broker broker-0 1
192.168.137.89:10911 V4_5_0 0.00(0,0ms) 0.00(0,0ms)
0 471030.34 0.2673
+ clusterListCmd := exec.Command("sh", cons.AdminToolDir,
cons.ClusterList, "-n", oldNameServerListStr)
+ clusterListOutput, err := clusterListCmd.Output()
+ if err != nil {
+ reqLogger.Error(err, "Get cluster list failed,
command: "+cons.AdminToolDir+" "+cons.ClusterList+" -n "+oldNameServerListStr)
+ return reconcile.Result{Requeue: true}, err
+ }
+ // get cluster of output
+ clusterName := ""
+ for _, line := range
strings.Split(string(clusterListOutput), "\n") {
+ if strings.HasPrefix(line, "#Cluster Name") {
+ continue
+ }
Review Comment:
It's not stable to get cluster name by analyzing the output of a command.
##########
pkg/controller/nameservice/nameservice_controller.go:
##########
@@ -216,17 +229,40 @@ func (r *ReconcileNameService)
updateNameServiceStatus(instance *rocketmqv1alpha
}
// use admin tool to update broker config
- if share.IsNameServersStrUpdated && (len(oldNameServerListStr)
> cons.MinIpListLength) && (len(share.NameServersStr) > cons.MinIpListLength) {
+ if isNameServersStrUpdated && (len(oldNameServerListStr) >
cons.MinIpListLength) && (len(newNameServerListStr) > cons.MinIpListLength) {
+ // bash-4.4$ ./mqadmin clusterList -n
192.168.180.36:9876
+ // #Cluster Name #Broker Name #BID
#Addr #Version #InTPS(LOAD) #OutTPS(LOAD)
#PCWait(ms) #Hour #SPACE
+ // broker broker-0 0
192.168.180.40:10911 V4_5_0 0.00(0,0ms) 0.00(0,0ms)
0 471030.34 -1.0000
+ // broker broker-0 1
192.168.137.89:10911 V4_5_0 0.00(0,0ms) 0.00(0,0ms)
0 471030.34 0.2673
+ clusterListCmd := exec.Command("sh", cons.AdminToolDir,
cons.ClusterList, "-n", oldNameServerListStr)
+ clusterListOutput, err := clusterListCmd.Output()
+ if err != nil {
+ reqLogger.Error(err, "Get cluster list failed,
command: "+cons.AdminToolDir+" "+cons.ClusterList+" -n "+oldNameServerListStr)
+ return reconcile.Result{Requeue: true}, err
+ }
+ // get cluster of output
+ clusterName := ""
+ for _, line := range
strings.Split(string(clusterListOutput), "\n") {
+ if strings.HasPrefix(line, "#Cluster Name") {
+ continue
+ }
+ for _, f := range strings.Fields(line) {
+ clusterName = f
+ break
+ }
+ }
+ if clusterName == "" {
+ reqLogger.Error(err, "Get empty cluster name,
command: "+cons.AdminToolDir+" "+cons.ClusterList+" -n "+oldNameServerListStr)
+ return reconcile.Result{Requeue: true}, err
+ }
+
mqAdmin := cons.AdminToolDir
subCmd := cons.UpdateBrokerConfig
key := cons.ParamNameServiceAddress
- reqLogger.Info("share.GroupNum=broker.Spec.Size=" +
strconv.Itoa(share.GroupNum))
-
- clusterName := share.BrokerClusterName
reqLogger.Info("Updating config " + key + " of cluster"
+ clusterName)
- command := mqAdmin + " " + subCmd + " -c " +
clusterName + " -k " + key + " -n " + oldNameServerListStr + " -v " +
share.NameServersStr
- cmd := exec.Command("sh", mqAdmin, subCmd, "-c",
clusterName, "-k", key, "-n", oldNameServerListStr, "-v", share.NameServersStr)
+ command := mqAdmin + " " + subCmd + " -c " +
clusterName + " -k " + key + " -n " + oldNameServerListStr + " -v " +
newNameServerListStr
+ cmd := exec.Command("sh", mqAdmin, subCmd, "-c",
clusterName, "-k", key, "-n", oldNameServerListStr, "-v", newNameServerListStr)
Review Comment:
Only brokers registering successfully to name servers can receive the
command, others will not be updated.
##########
pkg/controller/nameservice/nameservice_controller.go:
##########
@@ -241,16 +277,6 @@ func (r *ReconcileNameService)
updateNameServiceStatus(instance *rocketmqv1alpha
reqLogger.Info("NameServers IP " + strconv.Itoa(i) + ": " +
value)
}
- runningNameServerNum := getRunningNameServersNum(podList.Items)
- if runningNameServerNum == instance.Spec.Size {
- share.IsNameServersStrInitialized = true
- share.NameServersStr = nameServerListStr // reassign if
operator restarts
- }
Review Comment:
Can this block be safely removed?
--
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]
