[GitHub] [iotdb] CRZbulabula commented on a diff in pull request #8654: [IOTDB-5312] Consolidate ClientManagers in Datanodes for unified management

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

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


##########
node-commons/src/main/java/org/apache/iotdb/commons/client/async/AsyncDataNodeHeartbeatServiceClient.java:
##########
@@ -53,41 +55,44 @@ public AsyncDataNodeHeartbeatServiceClient(
     this.clientManager = clientManager;
   }
 
-  public void close() {
-    ___transport.close();
-    ___currentMethod = null;
-  }
-
-  /**
-   * return self if clientManager is not null, the method doesn't need to call by user, it will
-   * trigger once client transport complete.
-   */
-  private void returnSelf() {
-    if (clientManager != null) {
-      clientManager.returnClient(endpoint, this);
-    }
-  }
-
-  /**
-   * This method will be automatically called by the thrift selector thread, and we'll just simulate
-   * the behavior in our test
-   */
   @Override
   public void onComplete() {
     super.onComplete();
     returnSelf();
   }
 
-  /**
-   * This method will be automatically called by the thrift selector thread, and we'll just simulate
-   * the behavior in our test
-   */
   @Override
   public void onError(Exception e) {
     super.onError(e);
+    ThriftClient.resolveException(e, this);

Review Comment:
   It's better not to log any exception when cluster heartbeat fails down. Otherwise the user might think it's the ConfigNode-leader who has errors.



##########
node-commons/src/main/java/org/apache/iotdb/commons/conf/CommonConfig.java:
##########
@@ -99,13 +99,14 @@ public class CommonConfig {
    * ClientManager will have so many selector threads (TAsyncClientManager) to distribute to its
    * clients.
    */
-  private int selectorNumOfClientManager =
-      Runtime.getRuntime().availableProcessors() / 4 > 0
-          ? Runtime.getRuntime().availableProcessors() / 4
-          : 1;
+  private int selectorNumOfClientManager = 1;
 
   /** whether to use thrift compression. */
-  private boolean isCnRpcThriftCompressionEnabled = false;
+  private boolean isRpcThriftCompressionEnabled = false;
+
+  private int maxTotalClientForEachNode = 300;
+
+  private int maxIdleClientForEachNode = 200;

Review Comment:
   These updated parameters should be also removed in ConfigNodeConfig, IoTDBConfig, iotdb-confignode.properties and iotdb-datanode.properties. And should be added in iotdb-commons.properties.



##########
node-commons/src/main/java/org/apache/iotdb/commons/client/async/AsyncConfigNodeHeartbeatServiceClient.java:
##########
@@ -53,41 +55,44 @@ public AsyncConfigNodeHeartbeatServiceClient(
     this.clientManager = clientManager;
   }
 
-  public void close() {
-    ___transport.close();
-    ___currentMethod = null;
-  }
-
-  /**
-   * return self if clientManager is not null, the method doesn't need to call by user, it will
-   * trigger once client transport complete.
-   */
-  private void returnSelf() {
-    if (clientManager != null) {
-      clientManager.returnClient(endpoint, this);
-    }
-  }
-
-  /**
-   * This method will be automatically called by the thrift selector thread, and we'll just simulate
-   * the behavior in our test
-   */
   @Override
   public void onComplete() {
     super.onComplete();
     returnSelf();
   }
 
-  /**
-   * This method will be automatically called by the thrift selector thread, and we'll just simulate
-   * the behavior in our test
-   */
   @Override
   public void onError(Exception e) {
     super.onError(e);
+    ThriftClient.resolveException(e, this);

Review Comment:
   It's better not to log any exception when cluster heartbeat fails down. Otherwise the user might think it's the ConfigNode-leader who has errors.



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