[GitHub] [incubator-iotdb] neuyilan commented on a change in pull request #1691: [IOTDB-814] Cache and redirect distributed IoTDB's leader for write requests in IoTDB's client

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

neuyilan commented on a change in pull request #1691:
URL: https://github.com/apache/incubator-iotdb/pull/1691#discussion_r486809083



##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/member/MetaGroupMember.java
##########
@@ -1486,7 +1485,11 @@ private TSStatus processNonPartitionedMetaPlan(PhysicalPlan plan) {
         return status;
       }
     } else if (leader != null) {
-      return forwardPlan(plan, leader, null);
+      TSStatus result = forwardPlan(plan, leader, null);
+      if (!StatusUtils.NO_LEADER.equals(result)) {
+        result.setRedirectNode(new EndPoint(leader.getIp(), leader.getClientPort()));
+        return result;

Review comment:
       same as above, outside of the if statement

##########
File path: service-rpc/src/main/java/org/apache/iotdb/rpc/RpcUtils.java
##########
@@ -53,6 +55,14 @@ public static void verifySuccess(TSStatus status) throws StatementExecutionExcep
     }
   }
 
+  public static void verifySuccessWithRedirection(TSStatus status)
+      throws StatementExecutionException, RedirectException {
+    verifySuccess(status);
+    if (status.isSetRedirectNode()) {
+      throw new RedirectException(status.getRedirectNode());

Review comment:
       What this method is used for? and why need to throw an exception?

##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/member/MetaGroupMember.java
##########
@@ -1688,13 +1693,20 @@ private TSStatus forwardInsertTabletPlan(Map<PhysicalPlan, PartitionGroup> planG
     TSStatus[] subStatus = null;
     boolean noFailure = true;
     boolean isBatchFailure = false;
+    boolean allRedirect = true;
+    EndPoint endPoint = null;
     for (Map.Entry<PhysicalPlan, PartitionGroup> entry : planGroupMap.entrySet()) {
       tmpStatus = forwardToSingleGroup(entry);
       logger.debug("{}: from {},{},{}", name, entry.getKey(), entry.getValue(), tmpStatus);
       noFailure =
           (tmpStatus.getCode() == TSStatusCode.SUCCESS_STATUS.getStatusCode()) && noFailure;
       isBatchFailure = (tmpStatus.getCode() == TSStatusCode.MULTIPLE_ERROR.getStatusCode())
           || isBatchFailure;
+      if (tmpStatus.isSetRedirectNode()) {
+        endPoint = tmpStatus.getRedirectNode();
+      } else {
+        allRedirect = false;
+      }

Review comment:
       One more thing:
   if the last element of the _tmpStatus_ not set the RedirectNode, the final allRedirect value is false,
   This situation should be considered. 

##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/member/DataGroupMember.java
##########
@@ -1115,7 +1118,11 @@ TSStatus executeNonQueryPlan(PhysicalPlan plan) {
         return status;
       }
     } else if (leader != null) {
-      return forwardPlan(plan, leader, getHeader());
+      TSStatus result = forwardPlan(plan, leader, getHeader());
+      if (!StatusUtils.NO_LEADER.equals(result)) {
+        result.setRedirectNode(new EndPoint(leader.getIp(), leader.getClientPort()));
+        return result;
+      }

Review comment:
       the _return result;_ statements should be outside of the _if (!StatusUtils.NO_LEADER.equals(result)) {_ statements? 
   Otherwise, the plan will be executed twice.
   
   ```suggestion
         if (!StatusUtils.NO_LEADER.equals(result)) {
           result.setRedirectNode(new EndPoint(leader.getIp(), leader.getClientPort()));
         }
         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.

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