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/08/12 06:42:15 UTC
[GitHub] [iotdb] CRZbulabula opened a new pull request, #6979: [IOTDB-4112] Decoupling heartbeat scheduled executor service from load manager
CRZbulabula opened a new pull request, #6979:
URL: https://github.com/apache/iotdb/pull/6979
The following changes will bring NodeManager, PartitionManager and LoadManager more in line with their respective functional definitions:
1. Let NodeManager maintains the heartbeat ScheduledExecutorService
2. Let NodeManager maintains the NodeCache
3. Let PartitionManager maintains the RegionGroupCache
Such that the LoadManager can focus more on the load balancing policies.
You can see the [ConfigNode framwork](https://apache-iotdb.feishu.cn/docs/doccnUzGLXivqOoNjAo8Ole3cQe#) doc for more details
--
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 #6979: [IOTDB-4112] Decoupling heartbeat scheduled executor service from LoadManager
Posted by GitBox <gi...@apache.org>.
CRZbulabula commented on code in PR #6979:
URL: https://github.com/apache/iotdb/pull/6979#discussion_r944303127
##########
confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/PartitionRegionStateMachine.java:
##########
@@ -141,13 +141,15 @@ public void notifyLeaderChanged(ConsensusGroupId groupId, TEndPoint newLeader) {
if (currentNode.equals(newLeader)) {
LOGGER.info("Current node {} becomes Leader", newLeader);
configManager.getProcedureManager().shiftExecutor(true);
- configManager.getLoadManager().start();
+ configManager.getLoadManager().startLoadBalancingService();
+ configManager.getNodeManager().startHeartbeatService();
Review Comment:
I'm not sure either...
--
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 #6979: [IOTDB-4112] Decoupling heartbeat scheduled executor service from LoadManager
Posted by GitBox <gi...@apache.org>.
CRZbulabula commented on code in PR #6979:
URL: https://github.com/apache/iotdb/pull/6979#discussion_r944301945
##########
confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/ConfigNodeProcedureEnv.java:
##########
@@ -203,9 +203,6 @@ public void removeConfigNodePeer(TConfigNodeLocation tConfigNodeLocation)
try {
// Execute removePeer
if (getConsensusManager().removeConfigNodePeer(tConfigNodeLocation)) {
- configManager
- .getLoadManager()
- .removeNodeHeartbeatHandCache(tConfigNodeLocation.getConfigNodeId());
Review Comment:
I suppose that there is not need to make the cache management more intricate. Because the NodeCache and RegionGroupCache are limited and they will be discarded as long as the ConfigNode-leader changed.
--
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] Beyyes commented on a diff in pull request #6979: [IOTDB-4112] Decoupling heartbeat scheduled executor service from LoadManager
Posted by GitBox <gi...@apache.org>.
Beyyes commented on code in PR #6979:
URL: https://github.com/apache/iotdb/pull/6979#discussion_r944187789
##########
confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/PartitionRegionStateMachine.java:
##########
@@ -141,13 +141,15 @@ public void notifyLeaderChanged(ConsensusGroupId groupId, TEndPoint newLeader) {
if (currentNode.equals(newLeader)) {
LOGGER.info("Current node {} becomes Leader", newLeader);
configManager.getProcedureManager().shiftExecutor(true);
- configManager.getLoadManager().start();
+ configManager.getLoadManager().startLoadBalancingService();
+ configManager.getNodeManager().startHeartbeatService();
Review Comment:
What about moving `startHeartbeatService` in front of `startLoadBalancingService`
##########
confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/ConfigNodeProcedureEnv.java:
##########
@@ -203,9 +203,6 @@ public void removeConfigNodePeer(TConfigNodeLocation tConfigNodeLocation)
try {
// Execute removePeer
if (getConsensusManager().removeConfigNodePeer(tConfigNodeLocation)) {
- configManager
- .getLoadManager()
- .removeNodeHeartbeatHandCache(tConfigNodeLocation.getConfigNodeId());
Review Comment:
Why remove 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] Beyyes commented on a diff in pull request #6979: [IOTDB-4112] Decoupling heartbeat scheduled executor service from LoadManager
Posted by GitBox <gi...@apache.org>.
Beyyes commented on code in PR #6979:
URL: https://github.com/apache/iotdb/pull/6979#discussion_r944187789
##########
confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/PartitionRegionStateMachine.java:
##########
@@ -141,13 +141,15 @@ public void notifyLeaderChanged(ConsensusGroupId groupId, TEndPoint newLeader) {
if (currentNode.equals(newLeader)) {
LOGGER.info("Current node {} becomes Leader", newLeader);
configManager.getProcedureManager().shiftExecutor(true);
- configManager.getLoadManager().start();
+ configManager.getLoadManager().startLoadBalancingService();
+ configManager.getNodeManager().startHeartbeatService();
Review Comment:
What about moving `startHeartbeatService` in front of `startLoadBalancingService` [not sure]
--
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] qiaojialin merged pull request #6979: [IOTDB-4112] Decoupling heartbeat scheduled executor service from LoadManager
Posted by GitBox <gi...@apache.org>.
qiaojialin merged PR #6979:
URL: https://github.com/apache/iotdb/pull/6979
--
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