You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by GitBox <gi...@apache.org> on 2022/08/14 14:38:57 UTC

[GitHub] [rocketmq] mxsm opened a new pull request, #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

mxsm opened a new pull request, #4823:
URL: https://github.com/apache/rocketmq/pull/4823

   **Make sure set the target branch to `develop`**
   
   ## What is the purpose of the change
   
   close #4817 
   
   ## Brief changelog
   
   - Add a command to clear broker data from controller for CLI
   
   ## Verifying this change
   as following picture
   ![QQ截图20220814221956](https://user-images.githubusercontent.com/15797831/184541965-24fa0235-eb52-4854-9164-f88619b4abba.jpg)
   ![QQ截图20220814223141](https://user-images.githubusercontent.com/15797831/184541969-59a54894-4df7-42cc-892e-08d1e1657ecd.jpg)
   
   
   Follow this checklist to help us incorporate your contribution quickly and easily. Notice, `it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR`.
   
   - [x] Make sure there is a [Github issue](https://github.com/apache/rocketmq/issues) filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue. 
   - [x] Format the pull request title like `[ISSUE #123] Fix UnknownException when host config not exist`. Each commit in the pull request should have a meaningful subject line and body.
   - [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
   - [x] Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in [test module](https://github.com/apache/rocketmq/tree/master/test).
   - [x] Run `mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle` to make sure basic checks pass. Run `mvn clean install -DskipITs` to make sure unit-test pass. Run `mvn clean test-compile failsafe:integration-test`  to make sure integration-test pass.
   - [ ] If this contribution is large, please file an [Apache Individual Contributor License Agreement](http://www.apache.org/licenses/#clas).
   


-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] mxsm commented on a diff in pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
mxsm commented on code in PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#discussion_r950794994


##########
tools/src/main/java/org/apache/rocketmq/tools/command/controller/CleanControllerBrokerDataSubCommand.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.tools.command.controller;
+
+import org.apache.commons.cli.CommandLine;
+import org.apache.commons.cli.Option;
+import org.apache.commons.cli.Options;
+import org.apache.rocketmq.remoting.RPCHook;
+import org.apache.rocketmq.tools.admin.DefaultMQAdminExt;
+import org.apache.rocketmq.tools.command.SubCommand;
+import org.apache.rocketmq.tools.command.SubCommandException;
+
+public class CleanControllerBrokerDataSubCommand implements SubCommand {
+
+    @Override
+    public String commandName() {
+        return "cleanBrokerData";
+    }
+
+    @Override
+    public String commandDesc() {
+        return "Clean data of broker which shut down";
+    }
+
+    @Override
+    public Options buildCommandlineOptions(Options options) {
+
+        Option opt = new Option("a", "controllerAddress", true, "The address of controller");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("b", "brokerAddress", true, "The address of the broker which requires to clean metadata. eg: 192.168.0.1:30911;192.168.0.2:30911");
+        opt.setRequired(false);
+        options.addOption(opt);
+
+        opt = new Option("n", "brokerName", true, "The broker name of the replicas that require to be manipulated");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("c", "clusterName", true, "the clusterName of broker");

Review Comment:
   > OK, I got it. But we need to discuss whether we can only clean up non-living brokers.
   
   @RongtongJin  Can we give the user an option to decide whether only clean up non-living brokers. 



-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] mxsm commented on a diff in pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
mxsm commented on code in PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#discussion_r945882681


##########
controller/src/main/java/org/apache/rocketmq/controller/impl/DLedgerController.java:
##########
@@ -193,6 +194,32 @@ public RemotingServer getRemotingServer() {
         return this.dLedgerServer.getRemotingServer();
     }
 
+    @Override
+    public CompletableFuture<RemotingCommand> cleanBrokerData(
+        final CleanControllerBrokerDataRequestHeader requestHeader,
+        final BiPredicate<String, String> brokerAlivePredicate) {
+
+        String brokerAddrs = requestHeader.getBrokerAddress();
+        String clusterName = requestHeader.getClusterName();
+        if ((null == brokerAddrs || brokerAddrs.trim().isEmpty()) && brokerAlivePredicate.test(clusterName, null)) {
+            RemotingCommand responseCommand = RemotingCommand.createResponseCommand(ResponseCode.CONTROLLER_INVALID_REQUEST,
+                String.format("Broker %s is still alive, clean up failure", requestHeader.getBrokerName()));
+            return CompletableFuture.completedFuture(responseCommand);
+        } else {
+            String[] brokerAddrArray = brokerAddrs.split(";");
+            for (String brokerAddr : brokerAddrArray) {
+                if (brokerAlivePredicate.test(clusterName, brokerAddr)) {
+                    RemotingCommand responseCommand = RemotingCommand.createResponseCommand(ResponseCode.CONTROLLER_INVALID_REQUEST,
+                        String.format("Broker [%s,  %s] is still alive, clean up failure", requestHeader.getBrokerName(), brokerAddr));
+                    return CompletableFuture.completedFuture(responseCommand);
+                }
+            }
+        }

Review Comment:
   I will moving these to replicasInfoManager 



-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] codecov-commenter commented on pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#issuecomment-1214401255

   # [Codecov](https://codecov.io/gh/apache/rocketmq/pull/4823?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#4823](https://codecov.io/gh/apache/rocketmq/pull/4823?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d6ed6cc) into [develop](https://codecov.io/gh/apache/rocketmq/commit/8dd07c84697d3709f1f4bef5d78089b7d171b6cd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8dd07c8) will **decrease** coverage by `0.23%`.
   > The diff coverage is `4.57%`.
   
   ```diff
   @@              Coverage Diff              @@
   ##             develop    #4823      +/-   ##
   =============================================
   - Coverage      44.94%   44.71%   -0.24%     
   + Complexity      7640     7614      -26     
   =============================================
     Files            980      983       +3     
     Lines          68135    68279     +144     
     Branches        9014     9031      +17     
   =============================================
   - Hits           30622    30529      -93     
   - Misses         33729    33956     +227     
   - Partials        3784     3794      +10     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/rocketmq/pull/4823?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...g/apache/rocketmq/client/impl/MQClientAPIImpl.java](https://codecov.io/gh/apache/rocketmq/pull/4823/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9NUUNsaWVudEFQSUltcGwuamF2YQ==) | `23.24% <0.00%> (-0.20%)` | :arrow_down: |
   | [...g/apache/rocketmq/common/protocol/RequestCode.java](https://codecov.io/gh/apache/rocketmq/pull/4823/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jb21tb24vcHJvdG9jb2wvUmVxdWVzdENvZGUuamF2YQ==) | `0.00% <ø> (ø)` | |
   | [...roller/CleanControllerBrokerDataRequestHeader.java](https://codecov.io/gh/apache/rocketmq/pull/4823/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jb21tb24vcHJvdG9jb2wvaGVhZGVyL25hbWVzcnYvY29udHJvbGxlci9DbGVhbkNvbnRyb2xsZXJCcm9rZXJEYXRhUmVxdWVzdEhlYWRlci5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...he/rocketmq/controller/impl/DLedgerController.java](https://codecov.io/gh/apache/rocketmq/pull/4823/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvY29udHJvbGxlci9pbXBsL0RMZWRnZXJDb250cm9sbGVyLmphdmE=) | `65.30% <0.00%> (-5.03%)` | :arrow_down: |
   | [...controller/impl/DefaultBrokerHeartbeatManager.java](https://codecov.io/gh/apache/rocketmq/pull/4823/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvY29udHJvbGxlci9pbXBsL0RlZmF1bHRCcm9rZXJIZWFydGJlYXRNYW5hZ2VyLmphdmE=) | `70.21% <0.00%> (-10.28%)` | :arrow_down: |
   | [...mq/controller/impl/event/CleanBrokerDataEvent.java](https://codecov.io/gh/apache/rocketmq/pull/4823/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvY29udHJvbGxlci9pbXBsL2V2ZW50L0NsZWFuQnJva2VyRGF0YUV2ZW50LmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...ocketmq/controller/impl/event/EventSerializer.java](https://codecov.io/gh/apache/rocketmq/pull/4823/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvY29udHJvbGxlci9pbXBsL2V2ZW50L0V2ZW50U2VyaWFsaXplci5qYXZh) | `68.96% <0.00%> (-2.47%)` | :arrow_down: |
   | [...e/rocketmq/controller/impl/manager/BrokerInfo.java](https://codecov.io/gh/apache/rocketmq/pull/4823/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvY29udHJvbGxlci9pbXBsL21hbmFnZXIvQnJva2VySW5mby5qYXZh) | `78.94% <0.00%> (-9.29%)` | :arrow_down: |
   | [...ocketmq/controller/impl/manager/SyncStateInfo.java](https://codecov.io/gh/apache/rocketmq/pull/4823/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvY29udHJvbGxlci9pbXBsL21hbmFnZXIvU3luY1N0YXRlSW5mby5qYXZh) | `83.33% <0.00%> (-7.58%)` | :arrow_down: |
   | [...apache/rocketmq/tools/admin/DefaultMQAdminExt.java](https://codecov.io/gh/apache/rocketmq/pull/4823/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dG9vbHMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3Rvb2xzL2FkbWluL0RlZmF1bHRNUUFkbWluRXh0LmphdmE=) | `31.60% <0.00%> (-0.37%)` | :arrow_down: |
   | ... and [37 more](https://codecov.io/gh/apache/rocketmq/pull/4823/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] mxsm commented on a diff in pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
mxsm commented on code in PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#discussion_r950776521


##########
tools/src/main/java/org/apache/rocketmq/tools/command/controller/CleanControllerBrokerDataSubCommand.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.tools.command.controller;
+
+import org.apache.commons.cli.CommandLine;
+import org.apache.commons.cli.Option;
+import org.apache.commons.cli.Options;
+import org.apache.rocketmq.remoting.RPCHook;
+import org.apache.rocketmq.tools.admin.DefaultMQAdminExt;
+import org.apache.rocketmq.tools.command.SubCommand;
+import org.apache.rocketmq.tools.command.SubCommandException;
+
+public class CleanControllerBrokerDataSubCommand implements SubCommand {
+
+    @Override
+    public String commandName() {
+        return "cleanBrokerData";
+    }
+
+    @Override
+    public String commandDesc() {
+        return "Clean data of broker which shut down";
+    }
+
+    @Override
+    public Options buildCommandlineOptions(Options options) {
+
+        Option opt = new Option("a", "controllerAddress", true, "The address of controller");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("b", "brokerAddress", true, "The address of the broker which requires to clean metadata. eg: 192.168.0.1:30911;192.168.0.2:30911");
+        opt.setRequired(false);
+        options.addOption(opt);
+
+        opt = new Option("n", "brokerName", true, "The broker name of the replicas that require to be manipulated");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("c", "clusterName", true, "the clusterName of broker");

Review Comment:
   > Hi, @mxsm Could you explain the meaning of clusterName here?
   
   In Controller server ReplicasInfoManager#cleanBrokerData I need it and brokerAddrs to test broker is active.
   ```java
   # ReplicasInfoManager#cleanBrokerData
       public ControllerResult<Void> cleanBrokerData(final CleanControllerBrokerDataRequestHeader requestHeader,
           final BiPredicate<String, String> brokerAlivePredicate) {
           final ControllerResult<Void> result = new ControllerResult<>();
   
           final String clusterName = requestHeader.getClusterName();
           final String brokerName = requestHeader.getBrokerName();
           final String brokerAddrs = requestHeader.getBrokerAddress();
   
           //if SyncStateInfo.masterAddress is not empty, at least one broker with the same BrokerName is alive
           SyncStateInfo syncStateInfo = this.syncStateSetInfoTable.get(brokerName);
           if (StringUtils.isBlank(brokerAddrs) && null != syncStateInfo && StringUtils.isNotEmpty(syncStateInfo.getMasterAddress())) {
               String remark = String.format("Broker %s is still alive, clean up failure", requestHeader.getBrokerName());
               result.setCodeAndRemark(ResponseCode.CONTROLLER_INVALID_REQUEST, remark);
               return result;
           }
           Set<String> brokerAddressSet = null;
           if (StringUtils.isNotBlank(brokerAddrs)) {
               brokerAddressSet = Stream.of(brokerAddrs.split(";")).collect(Collectors.toSet());
               for (String brokerAddr : brokerAddressSet) {
                   if (brokerAlivePredicate.test(clusterName, brokerAddr)) {
                       String remark = String.format("Broker [%s,  %s] is still alive, clean up failure", requestHeader.getBrokerName(), brokerAddr);
                       result.setCodeAndRemark(ResponseCode.CONTROLLER_INVALID_REQUEST, remark);
                       return result;
                   }
               }
           }
           if (isContainsBroker(brokerName)) {
               final CleanBrokerDataEvent event = new CleanBrokerDataEvent(brokerName, brokerAddressSet);
               result.addEvent(event);
               return result;
           }
           result.setCodeAndRemark(ResponseCode.CONTROLLER_INVALID_REQUEST, String.format("Broker %s is not existed", brokerName));
           return result;
       }
   ```



-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] RongtongJin commented on a diff in pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
RongtongJin commented on code in PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#discussion_r950774344


##########
controller/src/main/java/org/apache/rocketmq/controller/Controller.java:
##########
@@ -106,4 +107,11 @@ CompletableFuture<RemotingCommand> alterSyncStateSet(
      * Get the remotingServer used by the controller, the upper layer will reuse this remotingServer.
      */
     RemotingServer getRemotingServer();
+
+    /**
+     * Clean controller broker data, the broker which  shut down or offline

Review Comment:
   “Clean controller broker data, the broker which  shut down or offline” -> "Clean controller broker data"
   



##########
controller/src/main/java/org/apache/rocketmq/controller/impl/manager/ReplicasInfoManager.java:
##########
@@ -387,6 +429,29 @@ private void handleElectMaster(final ElectMasterEvent event) {
         }
     }
 
+    private void handleCleanBrokerDataEvent(final CleanBrokerDataEvent event) {
+        final String brokerName = event.getBrokerName();
+        final Set<String> brokerAddressSet = event.getBrokerAddressSet();
+
+        if (!isContainsBroker(brokerName) || null == brokerAddressSet || brokerAddressSet.isEmpty()) {

Review Comment:
   Why do we need `this.replicaInfoTable.remove(brokerName)` and `this.syncStateSetInfoTable.remove(brokerName)` when !isContainsBroker(brokerName)?



##########
tools/src/main/java/org/apache/rocketmq/tools/command/controller/CleanControllerBrokerDataSubCommand.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.tools.command.controller;
+
+import org.apache.commons.cli.CommandLine;
+import org.apache.commons.cli.Option;
+import org.apache.commons.cli.Options;
+import org.apache.rocketmq.remoting.RPCHook;
+import org.apache.rocketmq.tools.admin.DefaultMQAdminExt;
+import org.apache.rocketmq.tools.command.SubCommand;
+import org.apache.rocketmq.tools.command.SubCommandException;
+
+public class CleanControllerBrokerDataSubCommand implements SubCommand {
+
+    @Override
+    public String commandName() {
+        return "cleanBrokerData";
+    }
+
+    @Override
+    public String commandDesc() {
+        return "Clean data of broker which shut down";
+    }
+
+    @Override
+    public Options buildCommandlineOptions(Options options) {
+
+        Option opt = new Option("a", "controllerAddress", true, "The address of controller");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("b", "brokerAddress", true, "The address of the broker which requires to clean metadata. eg: 192.168.0.1:30911;192.168.0.2:30911");
+        opt.setRequired(false);
+        options.addOption(opt);
+
+        opt = new Option("n", "brokerName", true, "The broker name of the replicas that require to be manipulated");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("c", "clusterName", true, "the clusterName of broker");

Review Comment:
   Hi, @mxsm Could you explain the meaning of clusterName here?



##########
controller/src/main/java/org/apache/rocketmq/controller/impl/manager/ReplicasInfoManager.java:
##########
@@ -316,6 +320,41 @@ public ControllerResult<Void> getSyncStateData(final List<String> brokerNames) {
         return result;
     }
 
+    public ControllerResult<Void> cleanBrokerData(final CleanControllerBrokerDataRequestHeader requestHeader,
+        final BiPredicate<String, String> brokerAlivePredicate) {
+        final ControllerResult<Void> result = new ControllerResult<>();
+
+        final String clusterName = requestHeader.getClusterName();
+        final String brokerName = requestHeader.getBrokerName();
+        final String brokerAddrs = requestHeader.getBrokerAddress();
+
+        //if SyncStateInfo.masterAddress is not empty, at least one broker with the same BrokerName is alive
+        SyncStateInfo syncStateInfo = this.syncStateSetInfoTable.get(brokerName);
+        if (StringUtils.isBlank(brokerAddrs) && null != syncStateInfo && StringUtils.isNotEmpty(syncStateInfo.getMasterAddress())) {
+            String remark = String.format("Broker %s is still alive, clean up failure", requestHeader.getBrokerName());
+            result.setCodeAndRemark(ResponseCode.CONTROLLER_INVALID_REQUEST, remark);
+            return result;
+        }
+        Set<String> brokerAddressSet = null;
+        if (StringUtils.isNotBlank(brokerAddrs)) {
+            brokerAddressSet = Stream.of(brokerAddrs.split(";")).collect(Collectors.toSet());
+            for (String brokerAddr : brokerAddressSet) {
+                if (brokerAlivePredicate.test(clusterName, brokerAddr)) {
+                    String remark = String.format("Broker [%s,  %s] is still alive, clean up failure", requestHeader.getBrokerName(), brokerAddr);
+                    result.setCodeAndRemark(ResponseCode.CONTROLLER_INVALID_REQUEST, remark);
+                    return result;
+                }
+            }
+        }

Review Comment:
   Hi, @mxsm I don't think it is necessary to check whether the brokers are alive before cleaning. Even if the living brokers are cleaned, they will register again.



-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] coveralls commented on pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
coveralls commented on PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#issuecomment-1214403397

   
   [![Coverage Status](https://coveralls.io/builds/51647160/badge)](https://coveralls.io/builds/51647160)
   
   Coverage decreased (-0.2%) to 48.999% when pulling **d6ed6ccfaa19a6faf5a86b9d0d5dc02c98b931bb on mxsm:rocketmq-4817** into **8dd07c84697d3709f1f4bef5d78089b7d171b6cd on apache:develop**.
   


-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] RongtongJin commented on a diff in pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
RongtongJin commented on code in PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#discussion_r953396587


##########
tools/src/main/java/org/apache/rocketmq/tools/command/controller/CleanControllerBrokerDataSubCommand.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.tools.command.controller;
+
+import org.apache.commons.cli.CommandLine;
+import org.apache.commons.cli.Option;
+import org.apache.commons.cli.Options;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.rocketmq.remoting.RPCHook;
+import org.apache.rocketmq.tools.admin.DefaultMQAdminExt;
+import org.apache.rocketmq.tools.command.SubCommand;
+import org.apache.rocketmq.tools.command.SubCommandException;
+
+public class CleanControllerBrokerDataSubCommand implements SubCommand {
+
+    @Override
+    public String commandName() {
+        return "cleanBrokerData";
+    }
+
+    @Override
+    public String commandDesc() {
+        return "Clean data of broker on controller";
+    }
+
+    @Override
+    public Options buildCommandlineOptions(Options options) {
+
+        Option opt = new Option("a", "controllerAddress", true, "The address of controller");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("b", "brokerAddress", true, "The address of the broker which requires to clean metadata. eg: 192.168.0.1:30911;192.168.0.2:30911");
+        opt.setRequired(false);
+        options.addOption(opt);
+
+        opt = new Option("n", "brokerName", true, "The broker name of the replicas that require to be manipulated");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("c", "clusterName", true, "the clusterName of broker");
+        opt.setRequired(false);
+        options.addOption(opt);
+
+        opt = new Option("l", "cleanLivingBroker", true, " whether clean up living brokers. value is false or true");
+        opt.setRequired(true);

Review Comment:
   IMO, cleanLivingBroker option is not required, the default value is false.



-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] mxsm commented on a diff in pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
mxsm commented on code in PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#discussion_r950804237


##########
tools/src/main/java/org/apache/rocketmq/tools/command/controller/CleanControllerBrokerDataSubCommand.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.tools.command.controller;
+
+import org.apache.commons.cli.CommandLine;
+import org.apache.commons.cli.Option;
+import org.apache.commons.cli.Options;
+import org.apache.rocketmq.remoting.RPCHook;
+import org.apache.rocketmq.tools.admin.DefaultMQAdminExt;
+import org.apache.rocketmq.tools.command.SubCommand;
+import org.apache.rocketmq.tools.command.SubCommandException;
+
+public class CleanControllerBrokerDataSubCommand implements SubCommand {
+
+    @Override
+    public String commandName() {
+        return "cleanBrokerData";
+    }
+
+    @Override
+    public String commandDesc() {
+        return "Clean data of broker which shut down";
+    }
+
+    @Override
+    public Options buildCommandlineOptions(Options options) {
+
+        Option opt = new Option("a", "controllerAddress", true, "The address of controller");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("b", "brokerAddress", true, "The address of the broker which requires to clean metadata. eg: 192.168.0.1:30911;192.168.0.2:30911");
+        opt.setRequired(false);
+        options.addOption(opt);
+
+        opt = new Option("n", "brokerName", true, "The broker name of the replicas that require to be manipulated");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("c", "clusterName", true, "the clusterName of broker");

Review Comment:
   > > > OK, I got it. But we need to discuss whether we can only clean up non-living brokers.
   > > 
   > > 
   > > @RongtongJin Can we give the user an option to decide whether only clean up non-living brokers.
   > 
   > Good suggestion
   
   I will follow this current idea to implement @RongtongJin 



-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] RongtongJin merged pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
RongtongJin merged PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823


-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] mxsm commented on pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
mxsm commented on PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#issuecomment-1215212020

   > Hi @mxsm ,
   > 
   > Thx for your contribution. Is there any test for this change?
   
   I will add test for this soon after optimize code logic
   
   


-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] RongtongJin commented on pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
RongtongJin commented on PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#issuecomment-1225259368

   Hi, @mxsm Could you rebase or merge the latest develop branch to this pull request? Develop branch add new required status checks, this pull request must pass the checker before merging.


-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] mxsm commented on a diff in pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
mxsm commented on code in PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#discussion_r952058091


##########
tools/src/main/java/org/apache/rocketmq/tools/command/controller/CleanControllerBrokerDataSubCommand.java:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.tools.command.controller;
+
+import org.apache.commons.cli.CommandLine;
+import org.apache.commons.cli.Option;
+import org.apache.commons.cli.Options;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.rocketmq.remoting.RPCHook;
+import org.apache.rocketmq.tools.admin.DefaultMQAdminExt;
+import org.apache.rocketmq.tools.command.SubCommand;
+import org.apache.rocketmq.tools.command.SubCommandException;
+
+public class CleanControllerBrokerDataSubCommand implements SubCommand {
+
+    @Override
+    public String commandName() {
+        return "cleanBrokerData";
+    }
+
+    @Override
+    public String commandDesc() {
+        return "Clean data of broker on controller";
+    }
+
+    @Override
+    public Options buildCommandlineOptions(Options options) {
+
+        Option opt = new Option("a", "controllerAddress", true, "The address of controller");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("b", "brokerAddress", true, "The address of the broker which requires to clean metadata. eg: 192.168.0.1:30911;192.168.0.2:30911");
+        opt.setRequired(false);
+        options.addOption(opt);
+
+        opt = new Option("n", "brokerName", true, "The broker name of the replicas that require to be manipulated");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("c", "clusterName", true, "the clusterName of broker");
+        opt.setRequired(false);
+        options.addOption(opt);
+
+        opt = new Option("l", "cleanLivingBroker", false, " whether clean up living brokers. default value is false");
+        opt.setRequired(false);
+        options.addOption(opt);
+
+        return options;
+    }
+
+    @Override
+    public void execute(CommandLine commandLine, Options options, RPCHook rpcHook) throws SubCommandException {
+
+        DefaultMQAdminExt defaultMQAdminExt = new DefaultMQAdminExt(rpcHook);
+        defaultMQAdminExt.setInstanceName(Long.toString(System.currentTimeMillis()));
+
+        String controllerAddress = commandLine.getOptionValue('a').trim();
+        String brokerName = commandLine.getOptionValue('n').trim();
+        String clusterName = null;
+        String brokerAddress = null;
+        boolean isCleanLivingBroker = false;
+        if (commandLine.hasOption('c')) {
+            clusterName = commandLine.getOptionValue('c').trim();
+        }
+        if (commandLine.hasOption('b')) {
+            brokerAddress = commandLine.getOptionValue('b').trim();
+        }
+        if (commandLine.hasOption('l')) {
+            isCleanLivingBroker = true;
+        }

Review Comment:
   OK I got it, need user to set the value



-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] mxsm commented on pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
mxsm commented on PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#issuecomment-1219541176

   Hi @RongtongJin  I has solved conflicts and resubmit code! please help review the code! Thks


-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] RongtongJin commented on a diff in pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
RongtongJin commented on code in PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#discussion_r945407081


##########
controller/src/main/java/org/apache/rocketmq/controller/impl/manager/ReplicasInfoManager.java:
##########
@@ -373,6 +374,19 @@ public ControllerResult<Void> getSyncStateData(final List<String> brokerNames) {
         return result;
     }
 
+    public ControllerResult<Void> cleanBrokerData(final CleanControllerBrokerDataRequestHeader requestHeader) {
+        final ControllerResult<Void> result = new ControllerResult<>();
+        String brokerName = requestHeader.getBrokerName();
+        String brokerAddress = requestHeader.getBrokerAddress();

Review Comment:
   Multiple brokerAddress are not handled here, eg: 192.168.0.1:30911;192.168.0.2:30911



##########
controller/src/main/java/org/apache/rocketmq/controller/impl/DLedgerController.java:
##########
@@ -193,6 +194,32 @@ public RemotingServer getRemotingServer() {
         return this.dLedgerServer.getRemotingServer();
     }
 
+    @Override
+    public CompletableFuture<RemotingCommand> cleanBrokerData(
+        final CleanControllerBrokerDataRequestHeader requestHeader,
+        final BiPredicate<String, String> brokerAlivePredicate) {
+
+        String brokerAddrs = requestHeader.getBrokerAddress();
+        String clusterName = requestHeader.getClusterName();
+        if ((null == brokerAddrs || brokerAddrs.trim().isEmpty()) && brokerAlivePredicate.test(clusterName, null)) {
+            RemotingCommand responseCommand = RemotingCommand.createResponseCommand(ResponseCode.CONTROLLER_INVALID_REQUEST,
+                String.format("Broker %s is still alive, clean up failure", requestHeader.getBrokerName()));
+            return CompletableFuture.completedFuture(responseCommand);
+        } else {
+            String[] brokerAddrArray = brokerAddrs.split(";");
+            for (String brokerAddr : brokerAddrArray) {
+                if (brokerAlivePredicate.test(clusterName, brokerAddr)) {
+                    RemotingCommand responseCommand = RemotingCommand.createResponseCommand(ResponseCode.CONTROLLER_INVALID_REQUEST,
+                        String.format("Broker [%s,  %s] is still alive, clean up failure", requestHeader.getBrokerName(), brokerAddr));
+                    return CompletableFuture.completedFuture(responseCommand);
+                }
+            }
+        }

Review Comment:
   How about moving these to replicasInfoManager just like other methods do?



##########
controller/src/main/java/org/apache/rocketmq/controller/impl/DefaultBrokerHeartbeatManager.java:
##########
@@ -112,6 +115,24 @@ public void registerBroker(String clusterName, String brokerName, String brokerA
         }
     }
 
+    public boolean isBrokerActiveOfBrokerName(String clusterName, String brokerName) {
+
+        Set<BrokerAddrInfo> addrInfoSet = this.brokerLiveTable.keySet().stream()
+            .filter(item -> StringUtils.equals(clusterName, item.getClusterName())).collect(Collectors.toSet());
+        if (null == addrInfoSet || addrInfoSet.size() == 0) {
+            return false;
+        }
+        for (BrokerAddrInfo addrInfo : addrInfoSet) {
+            BrokerLiveInfo brokerLiveInfo = this.brokerLiveTable.get(addrInfo);
+            if (brokerLiveInfo != null && StringUtils.equals(brokerLiveInfo.brokerName, brokerName)) {
+                long last = brokerLiveInfo.lastUpdateTimestamp;
+                long timeoutMillis = brokerLiveInfo.heartbeatTimeoutMillis;
+                return (last + timeoutMillis) >= System.currentTimeMillis();
+            }
+        }
+        return false;
+    }

Review Comment:
   Is it necessary to add this function? Maybe we can do this in replicasInfoManager



##########
controller/src/main/java/org/apache/rocketmq/controller/Controller.java:
##########
@@ -106,4 +108,11 @@ CompletableFuture<RemotingCommand> alterSyncStateSet(
      * Get the remotingServer used by the controller, the upper layer will reuse this remotingServer.
      */
     RemotingServer getRemotingServer();
+
+    /**
+     * Clean controller broker data, the broker which  shut down or offline
+     * @return
+     */
+    CompletableFuture<RemotingCommand> cleanBrokerData(final CleanControllerBrokerDataRequestHeader requestHeader,
+        final BiPredicate<String,String> brokerAlivePredicate);

Review Comment:
   brokerAlivePredicate may not be required. The implementation class DLedgerController contains brokerAlivePredicate.



-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] mxsm commented on a diff in pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
mxsm commented on code in PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#discussion_r950776521


##########
tools/src/main/java/org/apache/rocketmq/tools/command/controller/CleanControllerBrokerDataSubCommand.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.tools.command.controller;
+
+import org.apache.commons.cli.CommandLine;
+import org.apache.commons.cli.Option;
+import org.apache.commons.cli.Options;
+import org.apache.rocketmq.remoting.RPCHook;
+import org.apache.rocketmq.tools.admin.DefaultMQAdminExt;
+import org.apache.rocketmq.tools.command.SubCommand;
+import org.apache.rocketmq.tools.command.SubCommandException;
+
+public class CleanControllerBrokerDataSubCommand implements SubCommand {
+
+    @Override
+    public String commandName() {
+        return "cleanBrokerData";
+    }
+
+    @Override
+    public String commandDesc() {
+        return "Clean data of broker which shut down";
+    }
+
+    @Override
+    public Options buildCommandlineOptions(Options options) {
+
+        Option opt = new Option("a", "controllerAddress", true, "The address of controller");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("b", "brokerAddress", true, "The address of the broker which requires to clean metadata. eg: 192.168.0.1:30911;192.168.0.2:30911");
+        opt.setRequired(false);
+        options.addOption(opt);
+
+        opt = new Option("n", "brokerName", true, "The broker name of the replicas that require to be manipulated");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("c", "clusterName", true, "the clusterName of broker");

Review Comment:
   > Hi, @mxsm Could you explain the meaning of clusterName here?
   
   Hi @RongtongJin In Controller server ReplicasInfoManager#cleanBrokerData I need it and brokerAddrs to test broker is active.
   ```java
   # ReplicasInfoManager#cleanBrokerData
       public ControllerResult<Void> cleanBrokerData(final CleanControllerBrokerDataRequestHeader requestHeader,
           final BiPredicate<String, String> brokerAlivePredicate) {
           final ControllerResult<Void> result = new ControllerResult<>();
   
           final String clusterName = requestHeader.getClusterName();
           final String brokerName = requestHeader.getBrokerName();
           final String brokerAddrs = requestHeader.getBrokerAddress();
   
           //if SyncStateInfo.masterAddress is not empty, at least one broker with the same BrokerName is alive
           SyncStateInfo syncStateInfo = this.syncStateSetInfoTable.get(brokerName);
           if (StringUtils.isBlank(brokerAddrs) && null != syncStateInfo && StringUtils.isNotEmpty(syncStateInfo.getMasterAddress())) {
               String remark = String.format("Broker %s is still alive, clean up failure", requestHeader.getBrokerName());
               result.setCodeAndRemark(ResponseCode.CONTROLLER_INVALID_REQUEST, remark);
               return result;
           }
           Set<String> brokerAddressSet = null;
           if (StringUtils.isNotBlank(brokerAddrs)) {
               brokerAddressSet = Stream.of(brokerAddrs.split(";")).collect(Collectors.toSet());
               for (String brokerAddr : brokerAddressSet) {
                   if (brokerAlivePredicate.test(clusterName, brokerAddr)) {
                       String remark = String.format("Broker [%s,  %s] is still alive, clean up failure", requestHeader.getBrokerName(), brokerAddr);
                       result.setCodeAndRemark(ResponseCode.CONTROLLER_INVALID_REQUEST, remark);
                       return result;
                   }
               }
           }
           if (isContainsBroker(brokerName)) {
               final CleanBrokerDataEvent event = new CleanBrokerDataEvent(brokerName, brokerAddressSet);
               result.addEvent(event);
               return result;
           }
           result.setCodeAndRemark(ResponseCode.CONTROLLER_INVALID_REQUEST, String.format("Broker %s is not existed", brokerName));
           return result;
       }
   ```



-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] RongtongJin commented on a diff in pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
RongtongJin commented on code in PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#discussion_r950780306


##########
tools/src/main/java/org/apache/rocketmq/tools/command/controller/CleanControllerBrokerDataSubCommand.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.tools.command.controller;
+
+import org.apache.commons.cli.CommandLine;
+import org.apache.commons.cli.Option;
+import org.apache.commons.cli.Options;
+import org.apache.rocketmq.remoting.RPCHook;
+import org.apache.rocketmq.tools.admin.DefaultMQAdminExt;
+import org.apache.rocketmq.tools.command.SubCommand;
+import org.apache.rocketmq.tools.command.SubCommandException;
+
+public class CleanControllerBrokerDataSubCommand implements SubCommand {
+
+    @Override
+    public String commandName() {
+        return "cleanBrokerData";
+    }
+
+    @Override
+    public String commandDesc() {
+        return "Clean data of broker which shut down";
+    }
+
+    @Override
+    public Options buildCommandlineOptions(Options options) {
+
+        Option opt = new Option("a", "controllerAddress", true, "The address of controller");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("b", "brokerAddress", true, "The address of the broker which requires to clean metadata. eg: 192.168.0.1:30911;192.168.0.2:30911");
+        opt.setRequired(false);
+        options.addOption(opt);
+
+        opt = new Option("n", "brokerName", true, "The broker name of the replicas that require to be manipulated");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("c", "clusterName", true, "the clusterName of broker");

Review Comment:
   OK, I got it. But we need to discuss whether we can only clean up non-living brokers.



-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] RongtongJin commented on a diff in pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
RongtongJin commented on code in PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#discussion_r950800185


##########
tools/src/main/java/org/apache/rocketmq/tools/command/controller/CleanControllerBrokerDataSubCommand.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.tools.command.controller;
+
+import org.apache.commons.cli.CommandLine;
+import org.apache.commons.cli.Option;
+import org.apache.commons.cli.Options;
+import org.apache.rocketmq.remoting.RPCHook;
+import org.apache.rocketmq.tools.admin.DefaultMQAdminExt;
+import org.apache.rocketmq.tools.command.SubCommand;
+import org.apache.rocketmq.tools.command.SubCommandException;
+
+public class CleanControllerBrokerDataSubCommand implements SubCommand {
+
+    @Override
+    public String commandName() {
+        return "cleanBrokerData";
+    }
+
+    @Override
+    public String commandDesc() {
+        return "Clean data of broker which shut down";
+    }
+
+    @Override
+    public Options buildCommandlineOptions(Options options) {
+
+        Option opt = new Option("a", "controllerAddress", true, "The address of controller");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("b", "brokerAddress", true, "The address of the broker which requires to clean metadata. eg: 192.168.0.1:30911;192.168.0.2:30911");
+        opt.setRequired(false);
+        options.addOption(opt);
+
+        opt = new Option("n", "brokerName", true, "The broker name of the replicas that require to be manipulated");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("c", "clusterName", true, "the clusterName of broker");

Review Comment:
   > > OK, I got it. But we need to discuss whether we can only clean up non-living brokers.
   > 
   > @RongtongJin Can we give the user an option to decide whether only clean up non-living brokers.
   
   Good suggestion



-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] mxsm commented on a diff in pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
mxsm commented on code in PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#discussion_r950776521


##########
tools/src/main/java/org/apache/rocketmq/tools/command/controller/CleanControllerBrokerDataSubCommand.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.tools.command.controller;
+
+import org.apache.commons.cli.CommandLine;
+import org.apache.commons.cli.Option;
+import org.apache.commons.cli.Options;
+import org.apache.rocketmq.remoting.RPCHook;
+import org.apache.rocketmq.tools.admin.DefaultMQAdminExt;
+import org.apache.rocketmq.tools.command.SubCommand;
+import org.apache.rocketmq.tools.command.SubCommandException;
+
+public class CleanControllerBrokerDataSubCommand implements SubCommand {
+
+    @Override
+    public String commandName() {
+        return "cleanBrokerData";
+    }
+
+    @Override
+    public String commandDesc() {
+        return "Clean data of broker which shut down";
+    }
+
+    @Override
+    public Options buildCommandlineOptions(Options options) {
+
+        Option opt = new Option("a", "controllerAddress", true, "The address of controller");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("b", "brokerAddress", true, "The address of the broker which requires to clean metadata. eg: 192.168.0.1:30911;192.168.0.2:30911");
+        opt.setRequired(false);
+        options.addOption(opt);
+
+        opt = new Option("n", "brokerName", true, "The broker name of the replicas that require to be manipulated");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("c", "clusterName", true, "the clusterName of broker");

Review Comment:
   > Hi, @mxsm Could you explain the meaning of clusterName here?
   
   Hi @RongtongJin In Controller server ReplicasInfoManager#cleanBrokerData I need it and brokerAddrs to test broker is active.
   ```java
   // ReplicasInfoManager#cleanBrokerData
       public ControllerResult<Void> cleanBrokerData(final CleanControllerBrokerDataRequestHeader requestHeader,
           final BiPredicate<String, String> brokerAlivePredicate) {
           final ControllerResult<Void> result = new ControllerResult<>();
   
           final String clusterName = requestHeader.getClusterName();
           final String brokerName = requestHeader.getBrokerName();
           final String brokerAddrs = requestHeader.getBrokerAddress();
   
           //if SyncStateInfo.masterAddress is not empty, at least one broker with the same BrokerName is alive
           SyncStateInfo syncStateInfo = this.syncStateSetInfoTable.get(brokerName);
           if (StringUtils.isBlank(brokerAddrs) && null != syncStateInfo && StringUtils.isNotEmpty(syncStateInfo.getMasterAddress())) {
               String remark = String.format("Broker %s is still alive, clean up failure", requestHeader.getBrokerName());
               result.setCodeAndRemark(ResponseCode.CONTROLLER_INVALID_REQUEST, remark);
               return result;
           }
           Set<String> brokerAddressSet = null;
           if (StringUtils.isNotBlank(brokerAddrs)) {
               brokerAddressSet = Stream.of(brokerAddrs.split(";")).collect(Collectors.toSet());
               for (String brokerAddr : brokerAddressSet) {
                   if (brokerAlivePredicate.test(clusterName, brokerAddr)) {
                       String remark = String.format("Broker [%s,  %s] is still alive, clean up failure", requestHeader.getBrokerName(), brokerAddr);
                       result.setCodeAndRemark(ResponseCode.CONTROLLER_INVALID_REQUEST, remark);
                       return result;
                   }
               }
           }
           if (isContainsBroker(brokerName)) {
               final CleanBrokerDataEvent event = new CleanBrokerDataEvent(brokerName, brokerAddressSet);
               result.addEvent(event);
               return result;
           }
           result.setCodeAndRemark(ResponseCode.CONTROLLER_INVALID_REQUEST, String.format("Broker %s is not existed", brokerName));
           return result;
       }
   ```



-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] mxsm commented on a diff in pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
mxsm commented on code in PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#discussion_r950794994


##########
tools/src/main/java/org/apache/rocketmq/tools/command/controller/CleanControllerBrokerDataSubCommand.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.tools.command.controller;
+
+import org.apache.commons.cli.CommandLine;
+import org.apache.commons.cli.Option;
+import org.apache.commons.cli.Options;
+import org.apache.rocketmq.remoting.RPCHook;
+import org.apache.rocketmq.tools.admin.DefaultMQAdminExt;
+import org.apache.rocketmq.tools.command.SubCommand;
+import org.apache.rocketmq.tools.command.SubCommandException;
+
+public class CleanControllerBrokerDataSubCommand implements SubCommand {
+
+    @Override
+    public String commandName() {
+        return "cleanBrokerData";
+    }
+
+    @Override
+    public String commandDesc() {
+        return "Clean data of broker which shut down";
+    }
+
+    @Override
+    public Options buildCommandlineOptions(Options options) {
+
+        Option opt = new Option("a", "controllerAddress", true, "The address of controller");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("b", "brokerAddress", true, "The address of the broker which requires to clean metadata. eg: 192.168.0.1:30911;192.168.0.2:30911");
+        opt.setRequired(false);
+        options.addOption(opt);
+
+        opt = new Option("n", "brokerName", true, "The broker name of the replicas that require to be manipulated");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("c", "clusterName", true, "the clusterName of broker");

Review Comment:
   > OK, I got it. But we need to discuss whether we can only clean up non-living brokers.
   
   Can we give the user an option to decide whether only clean up non-living brokers. 



-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] mxsm commented on a diff in pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
mxsm commented on code in PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#discussion_r950796469


##########
controller/src/main/java/org/apache/rocketmq/controller/impl/manager/ReplicasInfoManager.java:
##########
@@ -316,6 +320,41 @@ public ControllerResult<Void> getSyncStateData(final List<String> brokerNames) {
         return result;
     }
 
+    public ControllerResult<Void> cleanBrokerData(final CleanControllerBrokerDataRequestHeader requestHeader,
+        final BiPredicate<String, String> brokerAlivePredicate) {
+        final ControllerResult<Void> result = new ControllerResult<>();
+
+        final String clusterName = requestHeader.getClusterName();
+        final String brokerName = requestHeader.getBrokerName();
+        final String brokerAddrs = requestHeader.getBrokerAddress();
+
+        //if SyncStateInfo.masterAddress is not empty, at least one broker with the same BrokerName is alive
+        SyncStateInfo syncStateInfo = this.syncStateSetInfoTable.get(brokerName);
+        if (StringUtils.isBlank(brokerAddrs) && null != syncStateInfo && StringUtils.isNotEmpty(syncStateInfo.getMasterAddress())) {
+            String remark = String.format("Broker %s is still alive, clean up failure", requestHeader.getBrokerName());
+            result.setCodeAndRemark(ResponseCode.CONTROLLER_INVALID_REQUEST, remark);
+            return result;
+        }
+        Set<String> brokerAddressSet = null;
+        if (StringUtils.isNotBlank(brokerAddrs)) {
+            brokerAddressSet = Stream.of(brokerAddrs.split(";")).collect(Collectors.toSet());
+            for (String brokerAddr : brokerAddressSet) {
+                if (brokerAlivePredicate.test(clusterName, brokerAddr)) {
+                    String remark = String.format("Broker [%s,  %s] is still alive, clean up failure", requestHeader.getBrokerName(), brokerAddr);
+                    result.setCodeAndRemark(ResponseCode.CONTROLLER_INVALID_REQUEST, remark);
+                    return result;
+                }
+            }
+        }

Review Comment:
    I got it



-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] mxsm commented on a diff in pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
mxsm commented on code in PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#discussion_r945885240


##########
controller/src/main/java/org/apache/rocketmq/controller/impl/DefaultBrokerHeartbeatManager.java:
##########
@@ -112,6 +115,24 @@ public void registerBroker(String clusterName, String brokerName, String brokerA
         }
     }
 
+    public boolean isBrokerActiveOfBrokerName(String clusterName, String brokerName) {
+
+        Set<BrokerAddrInfo> addrInfoSet = this.brokerLiveTable.keySet().stream()
+            .filter(item -> StringUtils.equals(clusterName, item.getClusterName())).collect(Collectors.toSet());
+        if (null == addrInfoSet || addrInfoSet.size() == 0) {
+            return false;
+        }
+        for (BrokerAddrInfo addrInfo : addrInfoSet) {
+            BrokerLiveInfo brokerLiveInfo = this.brokerLiveTable.get(addrInfo);
+            if (brokerLiveInfo != null && StringUtils.equals(brokerLiveInfo.brokerName, brokerName)) {
+                long last = brokerLiveInfo.lastUpdateTimestamp;
+                long timeoutMillis = brokerLiveInfo.heartbeatTimeoutMillis;
+                return (last + timeoutMillis) >= System.currentTimeMillis();
+            }
+        }
+        return false;
+    }

Review Comment:
   > Is it necessary to add this function? Maybe we can do this in replicasInfoManager
   
   I will do this in replicasInfoManager, and judge by the SyncStateInfo.masterAddress, I will get SyncStateInfo from syncStateSetInfoTable by brokerName. then if SyncStateInfo.masterAddress is not empty, at least one broker with the same BrokerName is alive



-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [rocketmq] RongtongJin commented on a diff in pull request #4823: [ISSUE #4817] Add a command to clear broker data from controller for CLI

Posted by GitBox <gi...@apache.org>.
RongtongJin commented on code in PR #4823:
URL: https://github.com/apache/rocketmq/pull/4823#discussion_r952051872


##########
tools/src/main/java/org/apache/rocketmq/tools/command/controller/CleanControllerBrokerDataSubCommand.java:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.tools.command.controller;
+
+import org.apache.commons.cli.CommandLine;
+import org.apache.commons.cli.Option;
+import org.apache.commons.cli.Options;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.rocketmq.remoting.RPCHook;
+import org.apache.rocketmq.tools.admin.DefaultMQAdminExt;
+import org.apache.rocketmq.tools.command.SubCommand;
+import org.apache.rocketmq.tools.command.SubCommandException;
+
+public class CleanControllerBrokerDataSubCommand implements SubCommand {
+
+    @Override
+    public String commandName() {
+        return "cleanBrokerData";
+    }
+
+    @Override
+    public String commandDesc() {
+        return "Clean data of broker on controller";
+    }
+
+    @Override
+    public Options buildCommandlineOptions(Options options) {
+
+        Option opt = new Option("a", "controllerAddress", true, "The address of controller");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("b", "brokerAddress", true, "The address of the broker which requires to clean metadata. eg: 192.168.0.1:30911;192.168.0.2:30911");
+        opt.setRequired(false);
+        options.addOption(opt);
+
+        opt = new Option("n", "brokerName", true, "The broker name of the replicas that require to be manipulated");
+        opt.setRequired(true);
+        options.addOption(opt);
+
+        opt = new Option("c", "clusterName", true, "the clusterName of broker");
+        opt.setRequired(false);
+        options.addOption(opt);
+
+        opt = new Option("l", "cleanLivingBroker", false, " whether clean up living brokers. default value is false");
+        opt.setRequired(false);
+        options.addOption(opt);
+
+        return options;
+    }
+
+    @Override
+    public void execute(CommandLine commandLine, Options options, RPCHook rpcHook) throws SubCommandException {
+
+        DefaultMQAdminExt defaultMQAdminExt = new DefaultMQAdminExt(rpcHook);
+        defaultMQAdminExt.setInstanceName(Long.toString(System.currentTimeMillis()));
+
+        String controllerAddress = commandLine.getOptionValue('a').trim();
+        String brokerName = commandLine.getOptionValue('n').trim();
+        String clusterName = null;
+        String brokerAddress = null;
+        boolean isCleanLivingBroker = false;
+        if (commandLine.hasOption('c')) {
+            clusterName = commandLine.getOptionValue('c').trim();
+        }
+        if (commandLine.hasOption('b')) {
+            brokerAddress = commandLine.getOptionValue('b').trim();
+        }
+        if (commandLine.hasOption('l')) {
+            isCleanLivingBroker = true;
+        }

Review Comment:
   The specific value of commandLine shall prevail, and cannot be directly set to true



-- 
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: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org