You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2022/12/29 06:02:56 UTC
[GitHub] [iotdb] OneSizeFitsQuorum opened a new pull request, #8654: [IOTDB-5312] Consolidate ClientManagers in Datanodes for unified management
OneSizeFitsQuorum opened a new pull request, #8654:
URL: https://github.com/apache/iotdb/pull/8654
see [jira5312](https://issues.apache.org/jira/browse/IOTDB-5312)
--
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
[GitHub] [iotdb] sonarcloud[bot] commented on pull request #8654: [IOTDB-5312] Consolidate ClientManagers in Datanodes for unified management
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #8654:
URL: https://github.com/apache/iotdb/pull/8654#issuecomment-1367102209
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_incubator-iotdb&pullRequest=8654)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL) [7 Code Smells](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_duplicated_lines_density&view=list) [15.2% Duplication](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [iotdb] sonarcloud[bot] commented on pull request #8654: [IOTDB-5312] Consolidate ClientManagers in Datanodes for unified management
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #8654:
URL: https://github.com/apache/iotdb/pull/8654#issuecomment-1373111475
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_incubator-iotdb&pullRequest=8654)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL) [5 Code Smells](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_duplicated_lines_density&view=list) [16.2% Duplication](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [iotdb] OneSizeFitsQuorum commented on a diff in pull request #8654: [IOTDB-5312] Consolidate ClientManagers in Datanodes for unified management
Posted by GitBox <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #8654:
URL: https://github.com/apache/iotdb/pull/8654#discussion_r1062086547
##########
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:
So far this is just a debug log, but we still need to clear all related invalid clients when datanode fails down, what do you think?
##########
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:
So far this is just a debug log, but we still need to clear all related invalid clients when datanode fails down, what do you think?
--
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
[GitHub] [iotdb] sonarcloud[bot] commented on pull request #8654: [IOTDB-5312] Consolidate ClientManagers in Datanodes for unified management
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #8654:
URL: https://github.com/apache/iotdb/pull/8654#issuecomment-1367094587
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_incubator-iotdb&pullRequest=8654)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL) [22 Code Smells](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_duplicated_lines_density&view=list) [15.3% Duplication](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [iotdb] sonarcloud[bot] commented on pull request #8654: [IOTDB-5312] Consolidate ClientManagers in Datanodes for unified management
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #8654:
URL: https://github.com/apache/iotdb/pull/8654#issuecomment-1367140376
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_incubator-iotdb&pullRequest=8654)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL) [7 Code Smells](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_duplicated_lines_density&view=list) [15.2% Duplication](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [iotdb] OneSizeFitsQuorum merged pull request #8654: [IOTDB-5312] Consolidate ClientManagers in Datanodes for unified management
Posted by GitBox <gi...@apache.org>.
OneSizeFitsQuorum merged PR #8654:
URL: https://github.com/apache/iotdb/pull/8654
--
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
[GitHub] [iotdb] OneSizeFitsQuorum commented on a diff in pull request #8654: [IOTDB-5312] Consolidate ClientManagers in Datanodes for unified management
Posted by GitBox <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #8654:
URL: https://github.com/apache/iotdb/pull/8654#discussion_r1062114440
##########
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:
Current thrift related parameters are placed in a separate configuration file for confignode/datanode. This ensures that confignode and datanode can use different thrift parameters in a distribution package.
To keep up with the current status, I load different parameters for the same variable in CommonDescriptor, which is ugly but ensures that any process can set the parameters correctly to initialize the clientPool with the correct configuration.
--
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
[GitHub] [iotdb] sonarcloud[bot] commented on pull request #8654: [IOTDB-5312] Consolidate ClientManagers in Datanodes for unified management
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #8654:
URL: https://github.com/apache/iotdb/pull/8654#issuecomment-1371796250
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_incubator-iotdb&pullRequest=8654)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL) [5 Code Smells](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_duplicated_lines_density&view=list) [16.2% Duplication](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [iotdb] CRZbulabula commented on a diff in pull request #8654: [IOTDB-5312] Consolidate ClientManagers in Datanodes for unified management
Posted by GitBox <gi...@apache.org>.
CRZbulabula commented on code in PR #8654:
URL: https://github.com/apache/iotdb/pull/8654#discussion_r1062180997
##########
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:
Debug log is suitable for this~
##########
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:
Debug log is suitable for this~
--
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
[GitHub] [iotdb] sonarcloud[bot] commented on pull request #8654: [IOTDB-5312] Consolidate ClientManagers in Datanodes for unified management
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #8654:
URL: https://github.com/apache/iotdb/pull/8654#issuecomment-1371024272
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_incubator-iotdb&pullRequest=8654)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL) [5 Code Smells](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_duplicated_lines_density&view=list) [15.7% Duplication](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [iotdb] OneSizeFitsQuorum commented on a diff in pull request #8654: [IOTDB-5312] Consolidate ClientManagers in Datanodes for unified management
Posted by GitBox <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #8654:
URL: https://github.com/apache/iotdb/pull/8654#discussion_r1062086547
##########
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:
So far this is just debug log, what do you think?
##########
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:
So far this is just debug log, what do you think?
--
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
[GitHub] [iotdb] sonarcloud[bot] commented on pull request #8654: [IOTDB-5312] Consolidate ClientManagers in Datanodes for unified management
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #8654:
URL: https://github.com/apache/iotdb/pull/8654#issuecomment-1367107102
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_incubator-iotdb&pullRequest=8654)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL) [7 Code Smells](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=8654&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_duplicated_lines_density&view=list) [15.2% Duplication](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=8654&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [iotdb] CRZbulabula commented on a diff in pull request #8654: [IOTDB-5312] Consolidate ClientManagers in Datanodes for unified management
Posted by 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