[GitHub] [iotdb] Beyyes commented on a diff in pull request #7531: [IOTDB-4550] Add NodeId to Peer

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

Beyyes commented on code in PR #7531:
URL: https://github.com/apache/iotdb/pull/7531#discussion_r990613590


##########
consensus/src/main/java/org/apache/iotdb/consensus/config/ConsensusConfig.java:
##########
@@ -24,16 +24,19 @@
 public class ConsensusConfig {
 
   private final TEndPoint thisNode;

Review Comment:
   Renaming `thisNode` to `thisNodeEndPoint` is better?



##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/Utils.java:
##########
@@ -70,27 +70,36 @@ public static RaftPeerId fromTEndPointToRaftPeerId(TEndPoint endpoint) {
     return RaftPeerId.valueOf(fromTEndPointToString(endpoint));
   }
 
-  public static TEndPoint formRaftPeerIdToTEndPoint(RaftPeerId id) {
-    String[] items = id.toString().split("_");
+  public static RaftPeerId fromNodeIdToRaftPeerId(int nodeId) {
+    return RaftPeerId.valueOf(String.valueOf(nodeId));
+  }
+
+  public static TEndPoint formRaftPeerAddressToTEndPoint(String address) {
+    String[] items = address.split(":");
     return new TEndPoint(items[0], Integer.parseInt(items[1]));
   }
 
+  public static int formRaftPeerIdToNodeId(RaftPeerId id) {
+    return Integer.parseInt(id.toString());
+  }
+
   public static TEndPoint formRaftPeerProtoToTEndPoint(RaftPeerProto proto) {

Review Comment:
   `from`?



##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/Utils.java:
##########
@@ -99,11 +108,21 @@ public static List<RaftPeer> fromPeersAndPriorityToRaftPeers(List<Peer> peers, i
         .collect(Collectors.toList());
   }
 
+  // TODO: RaftPeerProto的Id是否是nodeId

Review Comment:
   Do not use Chinese



##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/Utils.java:
##########
@@ -99,11 +108,21 @@ public static List<RaftPeer> fromPeersAndPriorityToRaftPeers(List<Peer> peers, i
         .collect(Collectors.toList());
   }
 
+  // TODO: RaftPeerProto的Id是否是nodeId
+  public static int formRaftPeerProtoToNodeId(RaftPeerProto proto) {

Review Comment:
   `from`?



##########
confignode/src/main/java/org/apache/iotdb/confignode/manager/node/NodeManager.java:
##########
@@ -765,4 +768,8 @@ private ClusterSchemaManager getClusterSchemaManager() {
   private PartitionManager getPartitionManager() {
     return configManager.getPartitionManager();
   }
+
+  public int generateNodeId() {

Review Comment:
   Maybe it's unnecessary to create a new method.



##########
confignode/src/main/java/org/apache/iotdb/confignode/manager/ConfigManager.java:
##########
@@ -197,6 +200,13 @@ public ConfigManager() throws IOException {
 
     // ConsensusManager must be initialized last, as it would load states from disk and reinitialize
     // above managers
+    // TODO:
+    // consensusManager不做初始化，因为创建时内部的RatisConsensus需要nodeId作为server的id，但是这个id只有在向seedConfignode发送注册时才会生成

Review Comment:
   Do not use chinese



##########
confignode/src/main/java/org/apache/iotdb/confignode/manager/node/NodeManager.java:
##########
@@ -383,8 +383,9 @@ public List<TConfigNodeInfo> getRegisteredConfigNodeInfoList() {
    * @param configNodeLocation The new ConfigNode
    */
   public void applyConfigNode(TConfigNodeLocation configNodeLocation) {
-    // Generate new ConfigNode's index
-    configNodeLocation.setConfigNodeId(nodeInfo.generateNextNodeId());
+    if (configNodeLocation.getConfigNodeId() != 0) {

Review Comment:
   Why add this judgement? `if (configNodeLocation.getConfigNodeId() != 0) {`



##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/Utils.java:
##########
@@ -99,11 +108,21 @@ public static List<RaftPeer> fromPeersAndPriorityToRaftPeers(List<Peer> peers, i
         .collect(Collectors.toList());
   }
 
+  // TODO: RaftPeerProto的Id是否是nodeId

Review Comment:
   We can add unit test to verify it



##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -603,8 +607,8 @@ public Peer getLeader(ConsensusGroupId groupId) {
     if (leaderId == null) {
       return null;
     }
-    TEndPoint leaderEndpoint = Utils.formRaftPeerIdToTEndPoint(leaderId);
-    return new Peer(groupId, leaderEndpoint);
+    int nodeId = Utils.formRaftPeerIdToNodeId(leaderId);
+    return new Peer(groupId, nodeId, null);

Review Comment:
   This line used endpoint before, I'm not sure if there exist problem if we replace with nodeId. 



-- 
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