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 2021/08/19 12:57:12 UTC
[GitHub] [iotdb] ericpai opened a new pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
ericpai opened a new pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797
See JIRA https://issues.apache.org/jira/browse/IOTDB-1564
BTW, did we need to change the default value? I think we should be compatible with v0.12.
--
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] ericpai edited a comment on pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
ericpai edited a comment on pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797#issuecomment-911259726
@jixuan1989 I make a brief test with 3 nodes cluster without any read/write pressure. And use Arthas tool to analyze the CPU usage. Here is my conclusion:
1. With heartbeat_interval_ms=100, the MetaHeartbeatThread and DataHeartbeatThread cost <0.1% CPU. And with heartbeat_interval_ms=1000, they cost <0.01% CPU. There's no significant impaction if we decrease the heartbeat interval.
2. Under no outer pressure environment, with heartbeat_interval_ms=100, the headertbeatThread CPU cost about 34.09% of the whole IoTDB program cost. And with heartbeat_interval_ms=1000, the percentage is 1.27%. Because CPU cost in total is very low, the percentage difference is huge.
I think the side effect of CPU is acceptable.
Attachments for reference:
1. hb-dashboard-100.png: The snapshot of dashboard of CPU of one leader node under heartbeat_interval_ms=100
2. hb-dashboard-1000.png: The snapshot of dashboard of CPU of one leader node under heartbeat_interval_ms=1000
3. hb -100.svg: The flame graph of CPU of one leader node by sampling at least 10 seconds under heartbeat_interval_ms=100
4. hb -1000.svg: The flame graph of CPU of one leader node by sampling at least 30 seconds under heartbeat_interval_ms=1000
[CPU-performance.zip](https://github.com/apache/iotdb/files/7096637/CPU-performance.zip)
--
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] ericpai commented on a change in pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797#discussion_r701042737
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/heartbeat/HeartbeatThread.java
##########
@@ -230,10 +235,15 @@ void sendHeartbeatSync(Node node) {
heartbeatHandler.onComplete(heartBeatResponse);
} catch (TTransportException e) {
logger.warn(
- "{}: Cannot send heart beat to node {} due to network", memberName, node, e);
+ memberName
Review comment:
Because the `logger.warn` has the following overload signatures with the `Marker` string:
```java
void warn(Marker var1, String var2);
void warn(Marker var1, String var2, Object var3);
void warn(Marker var1, String var2, Object var3, Object var4);
void warn(Marker var1, String var2, Object... var3);
void warn(Marker var1, String var2, Throwable var3);
```
We can see that only the last one can print the stacktrace from the last parameter. However, the original code here satisfies `void warn(Marker var1, String var2, Object var3, Object var4);` , 3 parameters with only two placeholders. The execption message and stacktrace are lost.
--
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] coveralls commented on pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797#issuecomment-901929369
[](https://coveralls.io/builds/42268822)
Coverage decreased (-0.008%) to 67.277% when pulling **3415fd0b2eceec90acebecac501c3979868834d3 on ericpai:improve/iotdb-1564** into **55d037fa69f77dc8df7f395186f0a702626216ba on apache:master**.
--
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] ericpai commented on a change in pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797#discussion_r701049849
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/config/ClusterConstant.java
##########
@@ -47,8 +48,9 @@ private ClusterConstant() {
}
/**
- * a failed election will restart in 2s~5s, this should be at least as long as a heartbeat
- * interval, or a stale node may frequently issue elections and thus makes the leader step down
+ * a failed election will restart in [2*heartbeatInterval, 2*heartbeatInterval + 50ms). If this
Review comment:
Yes, it's useless to set a minimum waiting time before starting election. After discussed with @jixuan1989 , we think that a constant maximum waiting time is not a good practice in real environment, but adding another configure parameter makes the cluster config more complicated. So we just set it as `1*heartbeatInterval`, which is lack of experiment now. The 50ms is just a protection if the heartbeatInterval is very small.
--
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] coveralls edited a comment on pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797#issuecomment-901929369
[](https://coveralls.io/builds/42270816)
Coverage decreased (-0.001%) to 67.248% when pulling **fb212e778592fd20d22c3cce8141901a759e0a88 on ericpai:improve/iotdb-1564** into **125f8cf649a16a62c1216b9e809c69fa5fddf7c9 on apache:master**.
--
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] ericpai commented on a change in pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797#discussion_r701042737
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/heartbeat/HeartbeatThread.java
##########
@@ -230,10 +235,15 @@ void sendHeartbeatSync(Node node) {
heartbeatHandler.onComplete(heartBeatResponse);
} catch (TTransportException e) {
logger.warn(
- "{}: Cannot send heart beat to node {} due to network", memberName, node, e);
+ memberName
Review comment:
Because the `logger.warn` has the following overload signatures:
```java
void warn(Marker var1, String var2);
void warn(Marker var1, String var2, Object var3);
void warn(Marker var1, String var2, Object var3, Object var4);
void warn(Marker var1, String var2, Object... var3);
void warn(String var1);
void warn(String var1, Object var2);
void warn(String var1, Object... var2);
void warn(String var1, Object var2, Object var3);
void warn(String var1, Throwable var2);
void warn(Marker var1, String var2, Throwable var3);
```
We can see that only the last two can print the stacktrace from the last parameter. However, the original code here satisfies `void warn(Marker var1, String var2, Object var3, Object var4);` , 3 parameters with only two placeholders. The execption message and stacktrace are lost.
--
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] ericpai commented on a change in pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797#discussion_r701053112
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/client/sync/SyncDataHeartbeatClient.java
##########
@@ -38,14 +38,19 @@
private SyncDataHeartbeatClient(TProtocolFactory protocolFactory, Node node, SyncClientPool pool)
throws TTransportException {
// the difference of the two clients lies in the port
+ // Heartbeat client connection timeout should not be larger than heartbeat interval, otherwise
+ // the thread pool of sending heartbeats or requesting votes may be used up by waiting for
+ // establishing connection with some slow or dead nodes.
super(
protocolFactory.getProtocol(
RpcTransportFactory.INSTANCE.getTransport(
new TSocket(
TConfigurationConst.defaultTConfiguration,
node.getInternalIp(),
node.getDataPort() + ClusterUtils.DATA_HEARTBEAT_PORT_OFFSET,
- RaftServer.getConnectionTimeoutInMS()))));
+ Math.min(
Review comment:
A new static field is added to RaftServer.
--
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] ericpai edited a comment on pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
ericpai edited a comment on pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797#issuecomment-911259726
@jixuan1989 I make a brief test with 3 nodes cluster without any read/write pressure. And use Arthas tool to analyze the CPU usage. Here is my conclusion:
1. With heartbeat_interval_ms=100, the MetaHeartbeatThread and DataHeartbeatThread costs <0.1% CPU. And with heartbeat_interval_ms=1000, they costs <0.01% CPU. There's no significant impaction if we decrease the heartbeat interval.
2. Under no outer pressure environment, with heartbeat_interval_ms=100, the headertbeatThread CPU costs about 34.09% of the whole IoTDB program cost. And with heartbeat_interval_ms=1000, the percentage is 1.27%. Because CPU cost in total is very low, the percentage difference is huge.
I think the side effect of CPU is acceptable.
Attachments for reference:
1. hb-dashboard-100.png: The snapshot of dashboard of CPU of one leader node under heartbeat_interval_ms=100
2. hb-dashboard-1000.png: The snapshot of dashboard of CPU of one leader node under heartbeat_interval_ms=1000
3. hb -100.svg: The flame graph of CPU of one leader node by sampling at least 10 seconds under heartbeat_interval_ms=100
4. hb -1000.svg: The flame graph of CPU of one leader node by sampling at least 30 seconds under heartbeat_interval_ms=1000
[CPU-performance.zip](https://github.com/apache/iotdb/files/7096637/CPU-performance.zip)
--
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] LebronAl commented on a change in pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
LebronAl commented on a change in pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797#discussion_r701527652
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/heartbeat/HeartbeatThread.java
##########
@@ -230,10 +235,15 @@ void sendHeartbeatSync(Node node) {
heartbeatHandler.onComplete(heartBeatResponse);
} catch (TTransportException e) {
logger.warn(
- "{}: Cannot send heart beat to node {} due to network", memberName, node, e);
+ memberName
Review comment:
got it
--
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] ericpai commented on a change in pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797#discussion_r701055332
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/client/sync/SyncMetaHeartbeatClient.java
##########
@@ -37,15 +37,20 @@
private SyncMetaHeartbeatClient(TProtocolFactory protocolFactory, Node node, SyncClientPool pool)
throws TTransportException {
- // the difference of the two clients lies in the port
+ // the difference of the two clients lies in the port.
+ // Heartbeat client connection timeout should not be larger than heartbeat interval, otherwise
+ // the thread pool of sending heartbeats or requesting votes may be used up by waiting for
+ // establishing connection with some slow or dead nodes.
super(
protocolFactory.getProtocol(
RpcTransportFactory.INSTANCE.getTransport(
new TSocket(
TConfigurationConst.defaultTConfiguration,
node.getInternalIp(),
node.getMetaPort() + ClusterUtils.META_HEARTBEAT_PORT_OFFSET,
- RaftServer.getConnectionTimeoutInMS()))));
+ Math.min(
Review comment:
A new static field is added to RaftServer.
--
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] jixuan1989 commented on a change in pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
jixuan1989 commented on a change in pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797#discussion_r700794103
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/client/sync/SyncDataHeartbeatClient.java
##########
@@ -38,14 +38,19 @@
private SyncDataHeartbeatClient(TProtocolFactory protocolFactory, Node node, SyncClientPool pool)
throws TTransportException {
// the difference of the two clients lies in the port
+ // Heartbeat client connection timeout should not be larger than heartbeat interval, otherwise
+ // the thread pool of sending heartbeats or requesting votes may be used up by waiting for
+ // establishing connection with some slow or dead nodes.
super(
protocolFactory.getProtocol(
RpcTransportFactory.INSTANCE.getTransport(
new TSocket(
TConfigurationConst.defaultTConfiguration,
node.getInternalIp(),
node.getDataPort() + ClusterUtils.DATA_HEARTBEAT_PORT_OFFSET,
- RaftServer.getConnectionTimeoutInMS()))));
+ Math.min(
Review comment:
```Math.min(
(int) RaftServer.getHeartbeatIntervalMs(),
RaftServer.getConnectionTimeoutInMS()))))```
can be saved in `RaftServer` as a new field.
--
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] ericpai commented on a change in pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797#discussion_r701043095
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/heartbeat/HeartbeatThread.java
##########
@@ -230,10 +235,15 @@ void sendHeartbeatSync(Node node) {
heartbeatHandler.onComplete(heartBeatResponse);
} catch (TTransportException e) {
logger.warn(
- "{}: Cannot send heart beat to node {} due to network", memberName, node, e);
+ memberName
+ + ": Cannot send heartbeat to node "
+ + node.toString()
+ + " due to network",
+ e);
client.getInputProtocol().getTransport().close();
} catch (Exception e) {
- logger.warn("{}: Cannot send heart beat to node {}", memberName, node, e);
+ logger.warn(
+ memberName + ": Cannot send heart beat to node " + node.toString(), e);
Review comment:
This has been explained
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/heartbeat/HeartbeatThread.java
##########
@@ -413,7 +422,11 @@ private void requestVoteSync(Node node, ElectionHandler handler, ElectionRequest
} catch (TException e) {
client.getInputProtocol().getTransport().close();
logger.warn(
- "{}: Cannot request a vote from {} due to network", memberName, node, e);
+ memberName
Review comment:
This has been explained
--
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] LebronAl commented on a change in pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
LebronAl commented on a change in pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797#discussion_r700842863
##########
File path: cluster/src/assembly/resources/conf/iotdb-cluster.properties
##########
@@ -79,7 +79,9 @@ multi_raft_factor=1
# all node's cluster_name in one cluster are the same
# cluster_name=default
-# connection time out (ms) among raft nodes
+# Thrift socket and connection timeout between raft nodes, in milliseconds.
+# NOTE: the timeout of connection used for sending heartbeats and requesting votes
+# will be adjust to min(heartbeat_interval_ms, connection_timeout_ms).
Review comment:
`adjusted`
##########
File path: cluster/src/assembly/resources/conf/iotdb-cluster.properties
##########
@@ -90,6 +92,13 @@ multi_raft_factor=1
# not for the whole operation.
# read_operation_timeout_ms=30000
+# the time interval (ms) between two rounds of heartbeat broadcast of one raft group leader.
+# Recommend to set it as 1/10 of election_timeout_ms, but larger than 1 RTT between each two nodes.
+# heartbeat_interval_ms=1000
+
+# The election timeout in follower, or the time waiting for request votes in elector, in milliseconds.
Review comment:
`for requesting votes`
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/heartbeat/HeartbeatThread.java
##########
@@ -413,7 +422,11 @@ private void requestVoteSync(Node node, ElectionHandler handler, ElectionRequest
} catch (TException e) {
client.getInputProtocol().getTransport().close();
logger.warn(
- "{}: Cannot request a vote from {} due to network", memberName, node, e);
+ memberName
Review comment:
same
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/client/sync/SyncDataHeartbeatClient.java
##########
@@ -38,14 +38,19 @@
private SyncDataHeartbeatClient(TProtocolFactory protocolFactory, Node node, SyncClientPool pool)
throws TTransportException {
// the difference of the two clients lies in the port
+ // Heartbeat client connection timeout should not be larger than heartbeat interval, otherwise
+ // the thread pool of sending heartbeats or requesting votes may be used up by waiting for
+ // establishing connection with some slow or dead nodes.
super(
protocolFactory.getProtocol(
RpcTransportFactory.INSTANCE.getTransport(
new TSocket(
TConfigurationConst.defaultTConfiguration,
node.getInternalIp(),
node.getDataPort() + ClusterUtils.DATA_HEARTBEAT_PORT_OFFSET,
- RaftServer.getConnectionTimeoutInMS()))));
+ Math.min(
Review comment:
+1
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/config/ClusterConstant.java
##########
@@ -47,8 +48,9 @@ private ClusterConstant() {
}
/**
- * a failed election will restart in 2s~5s, this should be at least as long as a heartbeat
- * interval, or a stale node may frequently issue elections and thus makes the leader step down
+ * a failed election will restart in [2*heartbeatInterval, 2*heartbeatInterval + 50ms). If this
Review comment:
Acutally, I think the lower bound should be 0.
With current logic, all candidates will wait this extra time for `2*heartbeatInterval`, which does not help to avoid live locks. It also does not help that stale nodes initiate elections first.
What do you think?
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/heartbeat/HeartbeatThread.java
##########
@@ -86,15 +84,17 @@ public void run() {
// send heartbeats to the followers
sendHeartbeats();
synchronized (localMember.getHeartBeatWaitObject()) {
- localMember.getHeartBeatWaitObject().wait(RaftServer.getHeartBeatIntervalMs());
+ localMember.getHeartBeatWaitObject().wait(RaftServer.getHeartbeatIntervalMs());
}
hasHadLeader = true;
break;
case FOLLOWER:
// check if heartbeat times out
- long heartBeatInterval =
+ long heartbeatInterval =
System.currentTimeMillis() - localMember.getLastHeartbeatReceivedTime();
- if (heartBeatInterval >= RaftServer.getConnectionTimeoutInMS()) {
+ long randomElectionTimeout =
+ RaftServer.getElectionTimeoutMs() + getElectionRandomTimeOutMs();
Review comment:
In fact, I don't think we need to calculate a randomElectionTimeout every time we receive a heartbeat, we just need to save a calculated randomElectionTimeout for every term.
But as it's trivil, I can accept this way for now.
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/heartbeat/HeartbeatThread.java
##########
@@ -230,10 +235,15 @@ void sendHeartbeatSync(Node node) {
heartbeatHandler.onComplete(heartBeatResponse);
} catch (TTransportException e) {
logger.warn(
- "{}: Cannot send heart beat to node {} due to network", memberName, node, e);
+ memberName
+ + ": Cannot send heartbeat to node "
+ + node.toString()
+ + " due to network",
+ e);
client.getInputProtocol().getTransport().close();
} catch (Exception e) {
- logger.warn("{}: Cannot send heart beat to node {}", memberName, node, e);
+ logger.warn(
+ memberName + ": Cannot send heart beat to node " + node.toString(), e);
Review comment:
same
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/heartbeat/HeartbeatThread.java
##########
@@ -230,10 +235,15 @@ void sendHeartbeatSync(Node node) {
heartbeatHandler.onComplete(heartBeatResponse);
} catch (TTransportException e) {
logger.warn(
- "{}: Cannot send heart beat to node {} due to network", memberName, node, e);
+ memberName
Review comment:
Although there's nothing wrong with this, I am curious for doing this.
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/client/sync/SyncMetaHeartbeatClient.java
##########
@@ -37,15 +37,20 @@
private SyncMetaHeartbeatClient(TProtocolFactory protocolFactory, Node node, SyncClientPool pool)
throws TTransportException {
- // the difference of the two clients lies in the port
+ // the difference of the two clients lies in the port.
+ // Heartbeat client connection timeout should not be larger than heartbeat interval, otherwise
+ // the thread pool of sending heartbeats or requesting votes may be used up by waiting for
+ // establishing connection with some slow or dead nodes.
super(
protocolFactory.getProtocol(
RpcTransportFactory.INSTANCE.getTransport(
new TSocket(
TConfigurationConst.defaultTConfiguration,
node.getInternalIp(),
node.getMetaPort() + ClusterUtils.META_HEARTBEAT_PORT_OFFSET,
- RaftServer.getConnectionTimeoutInMS()))));
+ Math.min(
Review comment:
same
--
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] ericpai commented on pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
ericpai commented on pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797#issuecomment-911259726
@jixuan1989 I make a brief test with 3 nodes cluster without any read/write pressure. And use Arthas tool to analyze the CPU usage. Here is my conclusion:
1. With heartbeat_interval_ms=100, the MetaHeartbeatThread and DataHeartbeatThread costs <0.1% CPU. And with heartbeat_interval_ms=1000, they costs <0.01% CPU. There's no significant impaction if we decrease the heartbeat interval.
2. Under no outer pressure environment, with heartbeat_interval_ms=100, the headertbeatThread CPU costs about 34.09% of the whole IoTDB program cost. And with heartbeat_interval_ms=1000, the percentage is 1.27%. Because CPU cost in total is very low, the percentage difference is huge.
I think the side effect of CPU is acceptable.
Attachments for reference:
1. hb-dashboard-100.png: The snapshot of dashboard of CPU of one leader node under heartbeat_interval_ms=100
2. hb-dashboard-1000.png: The snapshot of dashboard of CPU of one leader node under heartbeat_interval_ms=1000
3. 1. hb -100.svg: The flame graph of CPU of one leader node by sampling at least 10 seconds under heartbeat_interval_ms=100
4. 2. hb -1000.svg: The flame graph of CPU of one leader node by sampling at least 30 seconds under heartbeat_interval_ms=1000
[CPU-performance.zip](https://github.com/apache/iotdb/files/7096637/CPU-performance.zip)
--
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] ericpai commented on a change in pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797#discussion_r701052011
##########
File path: cluster/src/assembly/resources/conf/iotdb-cluster.properties
##########
@@ -79,7 +79,9 @@ multi_raft_factor=1
# all node's cluster_name in one cluster are the same
# cluster_name=default
-# connection time out (ms) among raft nodes
+# Thrift socket and connection timeout between raft nodes, in milliseconds.
+# NOTE: the timeout of connection used for sending heartbeats and requesting votes
+# will be adjust to min(heartbeat_interval_ms, connection_timeout_ms).
Review comment:
Thanks, this grammer issue has been fixed.
##########
File path: cluster/src/assembly/resources/conf/iotdb-cluster.properties
##########
@@ -90,6 +92,13 @@ multi_raft_factor=1
# not for the whole operation.
# read_operation_timeout_ms=30000
+# the time interval (ms) between two rounds of heartbeat broadcast of one raft group leader.
+# Recommend to set it as 1/10 of election_timeout_ms, but larger than 1 RTT between each two nodes.
+# heartbeat_interval_ms=1000
+
+# The election timeout in follower, or the time waiting for request votes in elector, in milliseconds.
Review comment:
Thanks, this grammer issue has been fixed.
--
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] LebronAl merged pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
LebronAl merged pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797
--
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] ericpai commented on a change in pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797#discussion_r701054060
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/heartbeat/HeartbeatThread.java
##########
@@ -86,15 +84,17 @@ public void run() {
// send heartbeats to the followers
sendHeartbeats();
synchronized (localMember.getHeartBeatWaitObject()) {
- localMember.getHeartBeatWaitObject().wait(RaftServer.getHeartBeatIntervalMs());
+ localMember.getHeartBeatWaitObject().wait(RaftServer.getHeartbeatIntervalMs());
}
hasHadLeader = true;
break;
case FOLLOWER:
// check if heartbeat times out
- long heartBeatInterval =
+ long heartbeatInterval =
System.currentTimeMillis() - localMember.getLastHeartbeatReceivedTime();
- if (heartBeatInterval >= RaftServer.getConnectionTimeoutInMS()) {
+ long randomElectionTimeout =
+ RaftServer.getElectionTimeoutMs() + getElectionRandomTimeOutMs();
Review comment:
OK, this is only a call of a random number.
--
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] ericpai edited a comment on pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
ericpai edited a comment on pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797#issuecomment-907759891
@jixuan1989 @LebronAl this pr is ready to be reviewed. I have tested in my local environment with a 3 nodes cluster. To set heartbeat_interval_ms=100, election_timeout_ms=1000, connection_timeout_ms=20000. The fastest successful election will be finished in 200ms, from the leader shutting down, and about 1000ms in average.
--
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] ericpai commented on pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
ericpai commented on pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797#issuecomment-907759891
@jixuan1989 @LebronAl this pr is ready to be reviewed. I have tested in my local environment with a 3 nodes cluster. To set heartbeat_interval_ms=100, election_timeout_ms=10000, connection_timeout_ms=20000. The fastest successful election will be finished in 200ms, from the leader shutting down, and about 1000ms in average.
--
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] jixuan1989 commented on a change in pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
jixuan1989 commented on a change in pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797#discussion_r700794103
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/client/sync/SyncDataHeartbeatClient.java
##########
@@ -38,14 +38,19 @@
private SyncDataHeartbeatClient(TProtocolFactory protocolFactory, Node node, SyncClientPool pool)
throws TTransportException {
// the difference of the two clients lies in the port
+ // Heartbeat client connection timeout should not be larger than heartbeat interval, otherwise
+ // the thread pool of sending heartbeats or requesting votes may be used up by waiting for
+ // establishing connection with some slow or dead nodes.
super(
protocolFactory.getProtocol(
RpcTransportFactory.INSTANCE.getTransport(
new TSocket(
TConfigurationConst.defaultTConfiguration,
node.getInternalIp(),
node.getDataPort() + ClusterUtils.DATA_HEARTBEAT_PORT_OFFSET,
- RaftServer.getConnectionTimeoutInMS()))));
+ Math.min(
Review comment:
```
Math.min(
(int) RaftServer.getHeartbeatIntervalMs(),
RaftServer.getConnectionTimeoutInMS()))))```
can be saved in `RaftServer` as a new field.
--
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] coveralls edited a comment on pull request #3797: [IOTDB-1564] Make hearbeat and election timeout parameters be configurable
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #3797:
URL: https://github.com/apache/iotdb/pull/3797#issuecomment-901929369
[](https://coveralls.io/builds/42270830)
Coverage decreased (-0.01%) to 67.238% when pulling **fb212e778592fd20d22c3cce8141901a759e0a88 on ericpai:improve/iotdb-1564** into **125f8cf649a16a62c1216b9e809c69fa5fddf7c9 on apache:master**.
--
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