[GitHub] [iotdb] CRZbulabula commented on a diff in pull request #7008: [IOTDB-4140] Mark the datanode as removing status when execute remove-datanode.sh

[GitBox <gi...@apache.org>]

CRZbulabula commented on code in PR #7008:
URL: https://github.com/apache/iotdb/pull/7008#discussion_r947547094


##########
confignode/src/main/java/org/apache/iotdb/confignode/manager/load/heartbeat/DataNodeHeartbeatCache.java:
##########
@@ -22,27 +22,44 @@
 
 import java.util.LinkedList;
 
-/** DataNodeHeartbeatCache caches and maintains all the heartbeat data */
+/**
+ * DataNodeHeartbeatCache caches and maintains all the heartbeat data
+ *
+ * <p>TODO: This class might be split into DataNodeCache and ConfigNodeCache
+ */
 public class DataNodeHeartbeatCache implements INodeCache {
 
-  // TODO: This class might be split into DataNodeCache and ConfigNodeCache
+  // when the response time of heartbeat is more than 20s,
+  // the datanode is considered as down
+  private static final int HEARTBEAT_TIMEOUT_TIME = 20_000;

Review Comment:
   The HEARTBEAT_TIMEOUT_TIME can be moved to INodeCache since it's also used in ConfigNodeHeartbeatCache



##########
confignode/src/main/java/org/apache/iotdb/confignode/manager/PartitionManager.java:
##########
@@ -531,7 +532,10 @@ public Map<TConsensusGroupId, Integer> getAllLeadership() {
       regionGroupCacheMap.forEach(
           (consensusGroupId, regionGroupCache) -> {
             if (consensusGroupId.getType().equals(TConsensusGroupType.SchemaRegion)) {
-              result.put(consensusGroupId, regionGroupCache.getLeaderDataNodeId());
+              int leaderDataNodeId = regionGroupCache.getLeaderDataNodeId();
+              if (!configManager.getNodeManager().isNodeRemoving(leaderDataNodeId)) {
+                result.put(consensusGroupId, leaderDataNodeId);
+              }

Review Comment:
   We'd better put -1 instead of nothing when the leaderNode happens to be in the status of Removing. This might avoid NPE.



##########
confignode/src/main/java/org/apache/iotdb/confignode/manager/PartitionManager.java:
##########
@@ -544,8 +548,12 @@ public Map<TConsensusGroupId, Integer> getAllLeadership() {
                       regionReplicaSet.getDataNodeLocations().get(0).getDataNodeId()));
     } else {
       regionGroupCacheMap.forEach(
-          (consensusGroupId, regionGroupCache) ->
-              result.put(consensusGroupId, regionGroupCache.getLeaderDataNodeId()));
+          (consensusGroupId, regionGroupCache) -> {
+            int leaderDataNodeId = regionGroupCache.getLeaderDataNodeId();
+            if (!configManager.getNodeManager().isNodeRemoving(leaderDataNodeId)) {
+              result.put(consensusGroupId, leaderDataNodeId);
+            }

Review Comment:
   We'd better put -1 instead of nothing when the leaderNode happens to be in the status of Removing. This might avoid NPE.



-- 
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: reviews-unsubscribe@iotdb.apache.org

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