Issue/cleanup config node removal

iotdb-core/confignode/src/assembly/resources/sbin/remove-confignode.bat

@@ -25,8 +25,6 @@ IF "%~1"=="--help" (
     echo Usage:
     echo Remove the ConfigNode with confignode_id
     echo ./sbin/remove-confignode.bat [confignode_id]
-    echo Remove the ConfigNode with address:port
-    echo ./sbin/remove-confignode.bat [cn_internal_address:cn_internal_port]
     EXIT /B 0
 )
 

iotdb-core/confignode/src/assembly/resources/sbin/remove-confignode.sh

@@ -24,8 +24,6 @@ if [ "$#" -eq 1 ] && [ "$1" == "--help" ]; then
     echo "Usage:"
     echo "Remove the ConfigNode with confignode_id"
     echo "./sbin/remove-confignode.sh [confignode_id]"
-    echo "Remove the ConfigNode with address:port"
-    echo "./sbin/remove-confignode.sh [cn_internal_address:cn_internal_port]"
     exit 0
 fi
 

iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/conf/ConfigNodeConstant.java

@@ -30,8 +30,7 @@ public class ConfigNodeConstant {
   public static final String CONFIGNODE_PACKAGE = "org.apache.iotdb.confignode.service";
   public static final String CONFIGNODE_JMX_PORT = "confignode.jmx.port";
 
-  public static final String REMOVE_CONFIGNODE_USAGE =
-      "Executed failed, check usage: <Node-id>/<internal_address>:<internal_port>";
+  public static final String REMOVE_CONFIGNODE_USAGE = "Executed failed, check usage: <Node-id>";
 
   public static final String REMOVE_DATANODE_PROCESS = "[REMOVE_DATANODE_PROCESS]";
   public static final String REGION_MIGRATE_PROCESS = "[REGION_MIGRATE_PROCESS]";

iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/conf/ConfigNodeRemoveCheck.java

@@ -20,11 +20,9 @@
 package org.apache.iotdb.confignode.conf;
 
 import org.apache.iotdb.common.rpc.thrift.TConfigNodeLocation;
-import org.apache.iotdb.common.rpc.thrift.TEndPoint;
 import org.apache.iotdb.common.rpc.thrift.TSStatus;
 import org.apache.iotdb.commons.conf.IoTDBConstant;
 import org.apache.iotdb.commons.exception.BadNodeUrlException;
-import org.apache.iotdb.commons.utils.NodeUrlUtils;
 import org.apache.iotdb.confignode.client.CnToCnNodeRequestType;
 import org.apache.iotdb.confignode.client.sync.SyncConfigNodeClientPool;
 import org.apache.iotdb.rpc.TSStatusCode;
@@ -48,53 +46,43 @@ public class ConfigNodeRemoveCheck {
 
   private static final Logger LOGGER = LoggerFactory.getLogger(ConfigNodeRemoveCheck.class);
 
-  private static final ConfigNodeConfig CONF = ConfigNodeDescriptor.getInstance().getConf();
-
+  private final SyncConfigNodeClientPool configNodeClientPool;
   private final File systemPropertiesFile;
   private final Properties systemProperties;
 
   public ConfigNodeRemoveCheck() {
-    systemPropertiesFile =
-        new File(CONF.getSystemDir() + File.separator + ConfigNodeConstant.SYSTEM_FILE_NAME);
-    systemProperties = new Properties();
+    this(ConfigNodeDescriptor.getInstance().getConf(), SyncConfigNodeClientPool.getInstance());
+  }
+
+  public ConfigNodeRemoveCheck(
+      ConfigNodeConfig conf, SyncConfigNodeClientPool configNodeClientPool) {
+    this.configNodeClientPool = configNodeClientPool;
+    this.systemPropertiesFile =
+        new File(conf.getSystemDir() + File.separator + ConfigNodeConstant.SYSTEM_FILE_NAME);
+    this.systemProperties = new Properties();
   }
 
   public TConfigNodeLocation removeCheck(String args) {
-    TConfigNodeLocation nodeLocation = new TConfigNodeLocation();
     if (!systemPropertiesFile.exists()) {
       LOGGER.error("The system properties file is not exists. IoTDB-ConfigNode is shutdown.");
-      return nodeLocation;
+      return null;
     }
     try (FileInputStream inputStream = new FileInputStream(systemPropertiesFile)) {
       systemProperties.load(new InputStreamReader(inputStream, StandardCharsets.UTF_8));
       if (isNumeric(args)) {
         int id = Integer.parseInt(args);
-        nodeLocation =
-            getConfigNodeList().stream()
-                .filter(e -> e.getConfigNodeId() == id)
-                .findFirst()
-                .orElse(null);
+        return getConfigNodeList().stream()
+            .filter(e -> e.getConfigNodeId() == id)
+            .findFirst()
+            .orElse(null);
       } else {
-        try {
-          TEndPoint endPoint = NodeUrlUtils.parseTEndPointUrl(args);
-          nodeLocation =
-              getConfigNodeList().stream()
-                  .filter(e -> e.getInternalEndPoint().equals(endPoint))
-                  .findFirst()
-                  .orElse(null);
-        } catch (BadNodeUrlException e) {
-          LOGGER.info(
-              "Usage: remove-confignode.sh <confignode-id> "
-                  + "or remove-confignode.sh <internal_address>:<internal_port>",
-              e);
-          return nodeLocation;
-        }
+        LOGGER.error("Invalid format. Expected a numeric node id, but got: {}", args);
       }
     } catch (IOException | BadNodeUrlException e) {
       LOGGER.error("Load system properties file failed.", e);
     }
 
-    return nodeLocation;
+    return null;
   }
 
   public void removeConfigNode(TConfigNodeLocation removedNode)
@@ -108,11 +96,10 @@ public void removeConfigNode(TConfigNodeLocation removedNode)
     for (TConfigNodeLocation configNodeLocation : configNodeList) {
       status =
           (TSStatus)
-              SyncConfigNodeClientPool.getInstance()
-                  .sendSyncRequestToConfigNodeWithRetry(
-                      configNodeLocation.getInternalEndPoint(),
-                      removedNode,
-                      CnToCnNodeRequestType.REMOVE_CONFIG_NODE);
+              configNodeClientPool.sendSyncRequestToConfigNodeWithRetry(
+                  configNodeLocation.getInternalEndPoint(),
+                  removedNode,
+                  CnToCnNodeRequestType.REMOVE_CONFIG_NODE);
       if (status.getCode() == TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
         break;
       }

iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/service/ConfigNodeCommandLine.java

@@ -42,47 +42,66 @@ public class ConfigNodeCommandLine extends ServerCommandLine {
   // Remove ConfigNode
   private static final String MODE_REMOVE = "-r";
 
+  private final ConfigNode configNode;
+  private final ConfigNodeRemoveCheck configNodeRemoveCheck;
+
   private static final String USAGE =
       "Usage: <-s|-r> "
           + "[-D{} <configure folder>] \n"
           + "-s: Start the ConfigNode and join to the cluster\n"
           + "-r: Remove the ConfigNode out of the cluster\n";
 
+  /** Default constructor using the singletons for initializing the relationship. */
+  public ConfigNodeCommandLine() {
+    this(ConfigNode.getInstance(), ConfigNodeRemoveCheck.getInstance());
+  }
+
+  /**
+   * Additional constructor allowing injection of custom instances (mainly for testing)
+   *
+   * @param configNode reference to a config node
+   * @param configNodeRemoveCheck reference to the config node remove check
+   */
+  public ConfigNodeCommandLine(ConfigNode configNode, ConfigNodeRemoveCheck configNodeRemoveCheck) {
+    this.configNode = configNode;
+    this.configNodeRemoveCheck = configNodeRemoveCheck;
+  }
+
   @Override
   protected String getUsage() {
     return USAGE;
   }
 
   @Override
   protected int run(String[] args) {
-    String mode;
-    if (args.length < 1) {
-      mode = MODE_START;
-      LOGGER.warn(
-          "ConfigNode does not specify a startup mode. The default startup mode {} will be used",
-          MODE_START);
-    } else {
-      mode = args[0];
+    if ((args == null) || (args.length < 1)) {
+      usage(null);
+      return -1;
     }
 
+    String mode = args[0];
     LOGGER.info("Running mode {}", mode);
     if (MODE_START.equals(mode)) {
       try {
         // Do ConfigNode startup checks
-        LOGGER.info("Starting IoTDB {}", IoTDBConstant.VERSION_WITH_BUILD);
+        LOGGER.info("Starting ConfigNode {}", IoTDBConstant.VERSION_WITH_BUILD);
         ConfigNodeStartupCheck checks = new ConfigNodeStartupCheck(IoTDBConstant.CN_ROLE);
         checks.startUpCheck();
       } catch (StartupException | ConfigurationException | IOException e) {
-        LOGGER.error("Meet error when doing start checking", e);
+        LOGGER.error("Received an error during startup of ConfigNode", e);
         return -1;
       }
       activeConfigNodeInstance();
     } else if (MODE_REMOVE.equals(mode)) {
+      if (args.length != 2) {
+        LOGGER.error("Expected 2 arguments, but got {}", args.length);
+        return -1;
+      }
       // remove ConfigNode
       try {
         doRemoveConfigNode(args);
-      } catch (IOException e) {
-        LOGGER.error("Meet error when doing remove ConfigNode", e);
+      } catch (IOException | BadNodeUrlException e) {
+        LOGGER.error("Received an error during removal of ConfigNode", e);
         return -1;
       }
     } else {
@@ -94,28 +113,25 @@ protected int run(String[] args) {
   }
 
   protected void activeConfigNodeInstance() {
-    ConfigNode.getInstance().active();
+    configNode.active();
   }
 
-  protected void doRemoveConfigNode(String[] args) throws IOException {
-
+  protected void doRemoveConfigNode(String[] args) throws BadNodeUrlException, IOException {
     if (args.length != 2) {
       LOGGER.info(REMOVE_CONFIGNODE_USAGE);
       return;
     }
 
     LOGGER.info("Starting to remove ConfigNode, parameter: {}, {}", args[0], args[1]);
 
-    try {
-      TConfigNodeLocation removeConfigNodeLocation =
-          ConfigNodeRemoveCheck.getInstance().removeCheck(args[1]);
-      if (removeConfigNodeLocation == null) {
-        LOGGER.error(
-            "The ConfigNode to be removed is not in the cluster, or the input format is incorrect.");
-        return;
-      }
+    TConfigNodeLocation removeConfigNodeLocation = configNodeRemoveCheck.removeCheck(args[1]);
+    // If for any reason no config node could be found, abort here.
+    if (removeConfigNodeLocation == null) {
+      throw new BadNodeUrlException("No ConfigNode to remove");
+    }
 
-      ConfigNodeRemoveCheck.getInstance().removeConfigNode(removeConfigNodeLocation);
+    try {
+      configNodeRemoveCheck.removeConfigNode(removeConfigNodeLocation);
     } catch (BadNodeUrlException e) {
       LOGGER.warn("No ConfigNodes need to be removed.", e);
       return;

iotdb-core/confignode/src/test/java/org/apache/iotdb/confignode/conf/ConfigNodeRemoveCheckTest.java

@@ -0,0 +1,31 @@
+/*
+ * 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.iotdb.confignode.conf;
+
+import junit.framework.TestCase;
+
+public class ConfigNodeRemoveCheckTest extends TestCase {
+
+  public void testRemoveCheckNodeId() {}
+
+  public void testRemoveCheckRpcAndPort() {}
+
+  public void testRemoveConfigNode() {}
+}