You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by "133tosakarin (via GitHub)" <gi...@apache.org> on 2024/08/15 02:57:50 UTC
[PR] RegionMigrate: Fix choosing the transferLeadership on migrateRegion [iotdb]
133tosakarin opened a new pull request, #13178:
URL: https://github.com/apache/iotdb/pull/13178
In the previous region migration implementation, we randomly selected an available node a and relinquished the existing leader privileges to this available node a. This may result in selecting a new node whose data has not yet caught up with the previous leader. This useless election is wasted because other nodes will never vote for node a.
In this PR, we fixed this issue, and if the migrated node is not the leader, then we do not perform the leader transfer
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1721201162
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -864,14 +868,30 @@ private RatisClient getRaftClient(RaftGroup group) throws ClientManagerException
}
}
+ private RatisClient getConfigurationRaftClient(RaftGroup group) throws ClientManagerException {
+ try {
+ return reconfigurationClientManager.borrowClient(group);
+ } catch (ClientManagerException e) {
+ logger.error("Borrow client from pool for group {} failed.", group, e);
+ // rethrow the exception
+ throw e;
+ }
+ }
+
private RaftClientReply sendReconfiguration(RaftGroup newGroupConf)
throws RatisRequestFailedException {
// notify the group leader of configuration change
RaftClientReply reply;
- try (RatisClient client = getRaftClient(newGroupConf)) {
- reply =
- client.getRaftClient().admin().setConfiguration(new ArrayList<>(newGroupConf.getPeers()));
- if (!reply.isSuccess()) {
+ try (RatisClient client = getConfigurationRaftClient(newGroupConf)) {
+ while (true) {
Review Comment:
I think it's no problem, but i can remove 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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719322745
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisClient.java:
##########
@@ -167,4 +210,48 @@ public Action handleAttemptFailure(Event event) {
return defaultPolicy.handleAttemptFailure(event);
}
}
+
+ // This policy is used to raft configuration change
+ private static class RatisEndlessRetryPolicy implements RetryPolicy {
+
+ private static final Logger logger = LoggerFactory.getLogger(RatisEndlessRetryPolicy.class);
+ private static final RetryPolicy defaultPolicy;
+
+ static {
+ String str = "";
+ // 50, 500ms, 40, 1000ms, 30, 1500ms, 20, 2000ms, 10, 2500ms
Review Comment:
it's a pairArray, pair such as (n, t), use the i-th pair, then client will retry n times, each time sleep t ms.
I cancat these pair as str, The MultipleLinearRandomRetry will parse it.
I want to calculate only once, so I think static chunk will only be executed once
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1718093684
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -693,11 +703,22 @@ public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation o
(CONF.getSchemaRegionRatisRpcLeaderElectionTimeoutMaxMs()
+ CONF.getSchemaRegionRatisRpcLeaderElectionTimeoutMinMs())
/ 2;
+ Integer leaderId = configManager.getLoadManager().getRegionLeaderMap().get(regionId);
+
+ if (leaderId != -1) {
+ // The migrated node is not leader, so we specify the transfer leader to current leader node
Review Comment:
I think so, we can report success
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1721199870
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -864,14 +868,30 @@ private RatisClient getRaftClient(RaftGroup group) throws ClientManagerException
}
}
+ private RatisClient getConfigurationRaftClient(RaftGroup group) throws ClientManagerException {
+ try {
+ return reconfigurationClientManager.borrowClient(group);
+ } catch (ClientManagerException e) {
+ logger.error("Borrow client from pool for group {} failed.", group, e);
+ // rethrow the exception
+ throw e;
+ }
+ }
+
private RaftClientReply sendReconfiguration(RaftGroup newGroupConf)
throws RatisRequestFailedException {
// notify the group leader of configuration change
RaftClientReply reply;
- try (RatisClient client = getRaftClient(newGroupConf)) {
- reply =
- client.getRaftClient().admin().setConfiguration(new ArrayList<>(newGroupConf.getPeers()));
- if (!reply.isSuccess()) {
+ try (RatisClient client = getConfigurationRaftClient(newGroupConf)) {
+ while (true) {
Review Comment:
do we need this while(true) now? as we have already done endless retry in raftclient
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1718208592
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -869,9 +873,34 @@ private RaftClientReply sendReconfiguration(RaftGroup newGroupConf)
// notify the group leader of configuration change
RaftClientReply reply;
try (RatisClient client = getRaftClient(newGroupConf)) {
- reply =
- client.getRaftClient().admin().setConfiguration(new ArrayList<>(newGroupConf.getPeers()));
- if (!reply.isSuccess()) {
+ int basicWaitTime = 500;
+ int maxWaitTime = 10000;
+ int retryTimes = 0;
+ while (true) {
+ logger.info("retry sendConfiguration req {} times", retryTimes);
+ reply =
+ client
+ .getRaftClient()
+ .admin()
+ .setConfiguration(new ArrayList<>(newGroupConf.getPeers()));
+ if (reply.isSuccess()) {
+ logger.info("reConfiguration success");
+ break;
+ }
+ if (reply.getException() instanceof ReconfigurationInProgressException
+ || reply.getException() instanceof LeaderSteppingDownException
Review Comment:
it will loop until reply return success or throw other exception. In fact, I want to use RaftServer.LifeCycle as loop factor, if majority RaftServer.LifeCycle is RUNNING, then we can return, but I can't get raftServers in newGroupConf's peers
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "liyuheng55555 (via GitHub)" <gi...@apache.org>.
liyuheng55555 commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719411834
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisClient.java:
##########
@@ -167,4 +210,48 @@ public Action handleAttemptFailure(Event event) {
return defaultPolicy.handleAttemptFailure(event);
}
}
+
+ // This policy is used to raft configuration change
+ private static class RatisEndlessRetryPolicy implements RetryPolicy {
+
+ private static final Logger logger = LoggerFactory.getLogger(RatisEndlessRetryPolicy.class);
+ private static final RetryPolicy defaultPolicy;
+
+ static {
+ String str = "";
+ // 50, 500ms, 40, 1000ms, 30, 1500ms, 20, 2000ms, 10, 2500ms
Review Comment:
I think just using "50, 500ms, 40, 1000ms, 30, 1500ms, 20, 2000ms, 10, 2500ms" will be more clear than building a String by loop.
But, this strategy seems not actually **endless** ?
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "liyuheng55555 (via GitHub)" <gi...@apache.org>.
liyuheng55555 commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719270294
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/impl/region/RemoveRegionPeerProcedure.java:
##########
@@ -68,12 +69,25 @@ public RemoveRegionPeerProcedure(
TConsensusGroupId consensusGroupId,
TDataNodeLocation coordinator,
TDataNodeLocation targetDataNode) {
- super();
this.consensusGroupId = consensusGroupId;
this.coordinator = coordinator;
this.targetDataNode = targetDataNode;
}
+ private void handleTransferLeader(RegionMaintainHandler handler)
+ throws ProcedureException, InterruptedException {
+ LOGGER.info(
+ "[pid{}][RemoveRegion] started, region {} will be removed from DataNode {}.",
+ getProcId(),
+ consensusGroupId.getId(),
+ targetDataNode.getDataNodeId());
+ handler.forceUpdateRegionCache(consensusGroupId, targetDataNode, RegionStatus.Removing);
+ List<TDataNodeLocation> excludeDataNode = new ArrayList<>();
+ excludeDataNode.add(targetDataNode);
+ excludeDataNode.add(coordinator);
+ handler.transferRegionLeader(consensusGroupId, targetDataNode, excludeDataNode);
Review Comment:
If replica = 1, we have to choose coordinator as new leader. Maybe not use `excludedDataNode` only, but use `excluded` and `notSuggested`
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -693,6 +702,14 @@ public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation o
(CONF.getSchemaRegionRatisRpcLeaderElectionTimeoutMaxMs()
+ CONF.getSchemaRegionRatisRpcLeaderElectionTimeoutMinMs())
/ 2;
+ Integer leaderId = configManager.getLoadManager().getRegionLeaderMap().get(regionId);
+
+ if (leaderId != -1) {
+ // The migrated node is not leader, so we don't need to transfer temporarily
+ if (originalDataNode.getDataNodeId() != leaderId) {
+ return;
+ }
+ }
Review Comment:
1. `if (leaderId != -1)` useless check
2. inline leaderId
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisClient.java:
##########
@@ -167,4 +210,48 @@ public Action handleAttemptFailure(Event event) {
return defaultPolicy.handleAttemptFailure(event);
}
}
+
+ // This policy is used to raft configuration change
+ private static class RatisEndlessRetryPolicy implements RetryPolicy {
+
+ private static final Logger logger = LoggerFactory.getLogger(RatisEndlessRetryPolicy.class);
+ private static final RetryPolicy defaultPolicy;
+
+ static {
+ String str = "";
+ // 50, 500ms, 40, 1000ms, 30, 1500ms, 20, 2000ms, 10, 2500ms
Review Comment:
What is this comment's meaning? Describe your algorithm more clearly.
And, why define an algorithm in static chunk?
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -864,14 +869,30 @@ private RatisClient getRaftClient(RaftGroup group) throws ClientManagerException
}
}
+ private RatisClient getConfRaftClient(RaftGroup group) throws ClientManagerException {
Review Comment:
get**Reconfiguration**RaftClient. Keep name consistent.
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisClient.java:
##########
@@ -167,4 +210,48 @@ public Action handleAttemptFailure(Event event) {
return defaultPolicy.handleAttemptFailure(event);
}
}
+
+ // This policy is used to raft configuration change
+ private static class RatisEndlessRetryPolicy implements RetryPolicy {
+
+ private static final Logger logger = LoggerFactory.getLogger(RatisEndlessRetryPolicy.class);
+ private static final RetryPolicy defaultPolicy;
+
+ static {
+ String str = "";
+ // 50, 500ms, 40, 1000ms, 30, 1500ms, 20, 2000ms, 10, 2500ms
+ int basicRetry = 50;
+ int basicSleep = 500;
+ for (int i = 0; i < 5; i++) {
+ str += basicRetry + "," + basicSleep + ",";
+ basicRetry -= 10;
+ basicSleep += 500;
+ }
+
+ defaultPolicy =
+ MultipleLinearRandomRetry.parseCommaSeparated(str.substring(0, str.length() - 1));
+ }
+
+ RatisEndlessRetryPolicy() {}
+
+ @Override
+ public Action handleAttemptFailure(Event event) {
+ // Ratis guarantees that event.getCause() is instance of IOException.
+ // We should allow RaftException or IOException(StatusRuntimeException, thrown by gRPC) to be
+ // retried.
+ Optional<Throwable> unexpectedCause =
+ Optional.ofNullable(event.getCause())
+ .filter(RaftException.class::isInstance)
+ .map(Throwable::getCause)
+ .filter(StatusRuntimeException.class::isInstance);
Review Comment:
What is `RaftException or IOException(StatusRuntimeException, thrown by gRPC)`? Words between () is confusing, StatusRuntimeException is not a kind of IOException.
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -901,12 +922,21 @@ public void allowStaleRead(ConsensusGroupId consensusGroupId) {
private class RatisClientPoolFactory implements IClientPoolFactory<RaftGroup, RatisClient> {
+ private boolean isReConfiguration;
+
+ RatisClientPoolFactory(boolean isReConfiguration) {
+ this.isReConfiguration = isReConfiguration;
+ }
Review Comment:
is**Reconfiguration**
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -122,6 +122,7 @@ class RatisConsensus implements IConsensus {
private final RaftClientRpc clientRpc;
private final IClientManager<RaftGroup, RatisClient> clientManager;
+ private final IClientManager<RaftGroup, RatisClient> reconfClientManager;
Review Comment:
**reconfiguration**ClientManager. We usually not use abbreviation in iotdb.
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1718096623
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -667,12 +667,22 @@ public void removeDataNodePersistence(TDataNodeLocation dataNodeLocation) {
*/
public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation originalDataNode)
throws ProcedureException, InterruptedException {
+ List<TDataNodeLocation> excludeDataNode = new ArrayList<>();
Review Comment:
:), ok
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719315176
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -901,12 +922,21 @@ public void allowStaleRead(ConsensusGroupId consensusGroupId) {
private class RatisClientPoolFactory implements IClientPoolFactory<RaftGroup, RatisClient> {
+ private boolean isReConfiguration;
+
+ RatisClientPoolFactory(boolean isReConfiguration) {
+ this.isReConfiguration = isReConfiguration;
+ }
Review Comment:
what's mean?
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -864,14 +869,30 @@ private RatisClient getRaftClient(RaftGroup group) throws ClientManagerException
}
}
+ private RatisClient getConfRaftClient(RaftGroup group) throws ClientManagerException {
Review Comment:
ok:)
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719590577
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisClient.java:
##########
@@ -167,4 +216,41 @@ public Action handleAttemptFailure(Event event) {
return defaultPolicy.handleAttemptFailure(event);
}
}
+
+ // This policy is used to raft configuration change
+ //
+ private static class RatisEndlessRetryPolicy implements RetryPolicy {
+
+ private static final Logger logger = LoggerFactory.getLogger(RatisEndlessRetryPolicy.class);
+ private final RetryPolicy defaultPolicy;
+
+ RatisEndlessRetryPolicy() {
+ // about 1 hour wait Time.
+ defaultPolicy =
+ RetryPolicies.retryForeverWithSleep(TimeDuration.valueOf(2, TimeUnit.SECONDS));
+ }
+
+ @Override
+ public Action handleAttemptFailure(Event event) {
+ if (event.getCause() instanceof ReconfigurationInProgressException) {
Review Comment:
why not use if（A || B || C)
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719692849
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -742,12 +759,27 @@ public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation o
*/
public Optional<TDataNodeLocation> filterDataNodeWithOtherRegionReplica(
TConsensusGroupId regionId, TDataNodeLocation filterLocation) {
+ List<TDataNodeLocation> filterLocations = new ArrayList<>();
Review Comment:
why not change?
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719692042
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisClient.java:
##########
@@ -167,4 +216,41 @@ public Action handleAttemptFailure(Event event) {
return defaultPolicy.handleAttemptFailure(event);
}
}
+
+ // This policy is used to raft configuration change
+ //
+ private static class RatisEndlessRetryPolicy implements RetryPolicy {
+
+ private static final Logger logger = LoggerFactory.getLogger(RatisEndlessRetryPolicy.class);
+ private final RetryPolicy defaultPolicy;
+
+ RatisEndlessRetryPolicy() {
+ // about 1 hour wait Time.
+ defaultPolicy =
+ RetryPolicies.retryForeverWithSleep(TimeDuration.valueOf(2, TimeUnit.SECONDS));
+ }
+
+ @Override
+ public Action handleAttemptFailure(Event event) {
+ if (event.getCause() instanceof ReconfigurationInProgressException) {
Review Comment:
Why not change 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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719710789
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -742,12 +759,27 @@ public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation o
*/
public Optional<TDataNodeLocation> filterDataNodeWithOtherRegionReplica(
TConsensusGroupId regionId, TDataNodeLocation filterLocation) {
+ List<TDataNodeLocation> filterLocations = new ArrayList<>();
Review Comment:
I didn't see 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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1718189670
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/impl/region/RemoveRegionPeerProcedure.java:
##########
@@ -68,12 +69,25 @@ public RemoveRegionPeerProcedure(
TConsensusGroupId consensusGroupId,
TDataNodeLocation coordinator,
TDataNodeLocation targetDataNode) {
- super();
this.consensusGroupId = consensusGroupId;
this.coordinator = coordinator;
this.targetDataNode = targetDataNode;
}
+ private void handleTransferLeader(RegionMaintainHandler handler)
+ throws ProcedureException, InterruptedException {
+ LOGGER.info(
+ "[pid{}][RemoveRegion] started, region {} will be removed from DataNode {}.",
+ getProcId(),
+ consensusGroupId.getId(),
+ targetDataNode.getDataNodeId());
+ handler.forceUpdateRegionCache(consensusGroupId, targetDataNode, RegionStatus.Removing);
+ List<TDataNodeLocation> excludeDataNode = new ArrayList<>();
+ excludeDataNode.add(targetDataNode);
+ excludeDataNode.add(coordinator);
Review Comment:
It feels like the semantics here should be allowed, and in the end, if the judgment is the same, do not send rpc?
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -869,9 +873,34 @@ private RaftClientReply sendReconfiguration(RaftGroup newGroupConf)
// notify the group leader of configuration change
RaftClientReply reply;
try (RatisClient client = getRaftClient(newGroupConf)) {
- reply =
- client.getRaftClient().admin().setConfiguration(new ArrayList<>(newGroupConf.getPeers()));
- if (!reply.isSuccess()) {
+ int basicWaitTime = 500;
+ int maxWaitTime = 10000;
Review Comment:
It feels like we can just check regularly here, without adding idempotent logic, because it doesn't make a big difference.
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -742,12 +759,27 @@ public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation o
*/
public Optional<TDataNodeLocation> filterDataNodeWithOtherRegionReplica(
TConsensusGroupId regionId, TDataNodeLocation filterLocation) {
+ List<TDataNodeLocation> filterLocations = new ArrayList<>();
Review Comment:
Collections.singletonList
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -869,9 +873,34 @@ private RaftClientReply sendReconfiguration(RaftGroup newGroupConf)
// notify the group leader of configuration change
RaftClientReply reply;
try (RatisClient client = getRaftClient(newGroupConf)) {
- reply =
- client.getRaftClient().admin().setConfiguration(new ArrayList<>(newGroupConf.getPeers()));
- if (!reply.isSuccess()) {
+ int basicWaitTime = 500;
+ int maxWaitTime = 10000;
+ int retryTimes = 0;
+ while (true) {
+ logger.info("retry sendConfiguration req {} times", retryTimes);
+ reply =
+ client
+ .getRaftClient()
+ .admin()
+ .setConfiguration(new ArrayList<>(newGroupConf.getPeers()));
+ if (reply.isSuccess()) {
+ logger.info("reConfiguration success");
+ break;
+ }
+ if (reply.getException() instanceof ReconfigurationInProgressException
+ || reply.getException() instanceof LeaderSteppingDownException
Review Comment:
when we need thses two exceptions? will this cause forever loop?
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -578,15 +597,26 @@ public void resetPeerList(ConsensusGroupId groupId, List<Peer> peers) throws Con
@Override
public void transferLeader(ConsensusGroupId groupId, Peer newLeader) throws ConsensusException {
// first fetch the newest information
+ logger.info("transfer consensusGroup {} leader to {}", groupId, newLeader);
final RaftGroupId raftGroupId = Utils.fromConsensusGroupIdToRaftGroupId(groupId);
final RaftGroup raftGroup =
Optional.ofNullable(getGroupInfo(raftGroupId))
.orElseThrow(() -> new ConsensusGroupNotExistException(groupId));
-
+ if (raftGroup.getPeers() == null) {
Review Comment:
OK, waiting for yuheng's pr
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -667,12 +667,21 @@ public void removeDataNodePersistence(TDataNodeLocation dataNodeLocation) {
*/
public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation originalDataNode)
throws ProcedureException, InterruptedException {
+ List<TDataNodeLocation> excludeDataNode = Collections.singletonList(originalDataNode);
+ transferRegionLeader(regionId, originalDataNode, excludeDataNode);
+ }
+
+ public void transferRegionLeader(
+ TConsensusGroupId regionId,
+ TDataNodeLocation originalDataNode,
+ List<TDataNodeLocation> excludeDataNode)
Review Comment:
seems we only need one node which will be deleted in the future? not a 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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719327236
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -693,6 +702,14 @@ public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation o
(CONF.getSchemaRegionRatisRpcLeaderElectionTimeoutMaxMs()
+ CONF.getSchemaRegionRatisRpcLeaderElectionTimeoutMinMs())
/ 2;
+ Integer leaderId = configManager.getLoadManager().getRegionLeaderMap().get(regionId);
+
+ if (leaderId != -1) {
+ // The migrated node is not leader, so we don't need to transfer temporarily
+ if (originalDataNode.getDataNodeId() != leaderId) {
+ return;
+ }
+ }
Review Comment:
leaderId may be -1, I think we should check 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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1718224867
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/impl/region/RemoveRegionPeerProcedure.java:
##########
@@ -68,12 +69,25 @@ public RemoveRegionPeerProcedure(
TConsensusGroupId consensusGroupId,
TDataNodeLocation coordinator,
TDataNodeLocation targetDataNode) {
- super();
this.consensusGroupId = consensusGroupId;
this.coordinator = coordinator;
this.targetDataNode = targetDataNode;
}
+ private void handleTransferLeader(RegionMaintainHandler handler)
+ throws ProcedureException, InterruptedException {
+ LOGGER.info(
+ "[pid{}][RemoveRegion] started, region {} will be removed from DataNode {}.",
+ getProcId(),
+ consensusGroupId.getId(),
+ targetDataNode.getDataNodeId());
+ handler.forceUpdateRegionCache(consensusGroupId, targetDataNode, RegionStatus.Removing);
+ List<TDataNodeLocation> excludeDataNode = new ArrayList<>();
+ excludeDataNode.add(targetDataNode);
+ excludeDataNode.add(coordinator);
Review Comment:
This just exclude origin node and target node, we don't need to transferLeadership to a new member, and the originNode will be deleted, we also filter it out. Considering that there may be other nodes that need to be filtered, I simply used List<T> to avoid adding additional parameters to the function
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "liyuheng55555 (via GitHub)" <gi...@apache.org>.
liyuheng55555 commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719289278
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -693,6 +702,14 @@ public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation o
(CONF.getSchemaRegionRatisRpcLeaderElectionTimeoutMaxMs()
+ CONF.getSchemaRegionRatisRpcLeaderElectionTimeoutMinMs())
/ 2;
+ Integer leaderId = configManager.getLoadManager().getRegionLeaderMap().get(regionId);
+
+ if (leaderId != -1) {
+ // The migrated node is not leader, so we don't need to transfer temporarily
+ if (originalDataNode.getDataNodeId() != leaderId) {
+ return;
+ }
+ }
Review Comment:
I reconsidered here, maybe the check is necessary?
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719316183
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisClient.java:
##########
@@ -167,4 +210,48 @@ public Action handleAttemptFailure(Event event) {
return defaultPolicy.handleAttemptFailure(event);
}
}
+
+ // This policy is used to raft configuration change
+ private static class RatisEndlessRetryPolicy implements RetryPolicy {
+
+ private static final Logger logger = LoggerFactory.getLogger(RatisEndlessRetryPolicy.class);
+ private static final RetryPolicy defaultPolicy;
+
+ static {
+ String str = "";
+ // 50, 500ms, 40, 1000ms, 30, 1500ms, 20, 2000ms, 10, 2500ms
+ int basicRetry = 50;
+ int basicSleep = 500;
+ for (int i = 0; i < 5; i++) {
+ str += basicRetry + "," + basicSleep + ",";
+ basicRetry -= 10;
+ basicSleep += 500;
+ }
+
+ defaultPolicy =
+ MultipleLinearRandomRetry.parseCommaSeparated(str.substring(0, str.length() - 1));
+ }
+
+ RatisEndlessRetryPolicy() {}
+
+ @Override
+ public Action handleAttemptFailure(Event event) {
+ // Ratis guarantees that event.getCause() is instance of IOException.
+ // We should allow RaftException or IOException(StatusRuntimeException, thrown by gRPC) to be
+ // retried.
+ Optional<Throwable> unexpectedCause =
+ Optional.ofNullable(event.getCause())
+ .filter(RaftException.class::isInstance)
+ .map(Throwable::getCause)
+ .filter(StatusRuntimeException.class::isInstance);
Review Comment:
I copy from RatisRetryPolicy
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719712258
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -742,12 +763,29 @@ public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation o
*/
public Optional<TDataNodeLocation> filterDataNodeWithOtherRegionReplica(
TConsensusGroupId regionId, TDataNodeLocation filterLocation) {
+ List<TDataNodeLocation> filterLocations = new ArrayList<>();
+ filterLocations.add(filterLocation);
+ return filterDataNodeWithOtherRegionReplica(regionId, filterLocations);
+ }
+
+ public Optional<TDataNodeLocation> filterDataNodeWithOtherRegionReplica(
+ TConsensusGroupId regionId, List<TDataNodeLocation> filterLocations) {
return filterDataNodeWithOtherRegionReplica(
- regionId, filterLocation, NodeStatus.Running, NodeStatus.ReadOnly);
+ regionId, filterLocations, NodeStatus.Running, NodeStatus.ReadOnly);
}
public Optional<TDataNodeLocation> filterDataNodeWithOtherRegionReplica(
TConsensusGroupId regionId, TDataNodeLocation filterLocation, NodeStatus... allowingStatus) {
+ List<TDataNodeLocation> excludeLocations = new ArrayList<>();
+ excludeLocations.add(filterLocation);
Review Comment:
I have 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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719325857
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/impl/region/RemoveRegionPeerProcedure.java:
##########
@@ -68,12 +69,25 @@ public RemoveRegionPeerProcedure(
TConsensusGroupId consensusGroupId,
TDataNodeLocation coordinator,
TDataNodeLocation targetDataNode) {
- super();
this.consensusGroupId = consensusGroupId;
this.coordinator = coordinator;
this.targetDataNode = targetDataNode;
}
+ private void handleTransferLeader(RegionMaintainHandler handler)
+ throws ProcedureException, InterruptedException {
+ LOGGER.info(
+ "[pid{}][RemoveRegion] started, region {} will be removed from DataNode {}.",
+ getProcId(),
+ consensusGroupId.getId(),
+ targetDataNode.getDataNodeId());
+ handler.forceUpdateRegionCache(consensusGroupId, targetDataNode, RegionStatus.Removing);
+ List<TDataNodeLocation> excludeDataNode = new ArrayList<>();
+ excludeDataNode.add(targetDataNode);
+ excludeDataNode.add(coordinator);
+ handler.transferRegionLeader(consensusGroupId, targetDataNode, excludeDataNode);
Review Comment:
Then I'll change it again.
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "liyuheng55555 (via GitHub)" <gi...@apache.org>.
liyuheng55555 commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719407452
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -901,12 +922,21 @@ public void allowStaleRead(ConsensusGroupId consensusGroupId) {
private class RatisClientPoolFactory implements IClientPoolFactory<RaftGroup, RatisClient> {
+ private boolean isReConfiguration;
+
+ RatisClientPoolFactory(boolean isReConfiguration) {
+ this.isReConfiguration = isReConfiguration;
+ }
Review Comment:
C->c
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719712083
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -667,12 +667,22 @@ public void removeDataNodePersistence(TDataNodeLocation dataNodeLocation) {
*/
public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation originalDataNode)
throws ProcedureException, InterruptedException {
+ List<TDataNodeLocation> excludeDataNode = new ArrayList<>();
+ excludeDataNode.add(originalDataNode);
Review Comment:
I have 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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719738052
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -578,15 +597,26 @@ public void resetPeerList(ConsensusGroupId groupId, List<Peer> peers) throws Con
@Override
public void transferLeader(ConsensusGroupId groupId, Peer newLeader) throws ConsensusException {
// first fetch the newest information
+ logger.info("transfer consensusGroup {} leader to {}", groupId, newLeader);
final RaftGroupId raftGroupId = Utils.fromConsensusGroupIdToRaftGroupId(groupId);
final RaftGroup raftGroup =
Optional.ofNullable(getGroupInfo(raftGroupId))
.orElseThrow(() -> new ConsensusGroupNotExistException(groupId));
-
+ if (raftGroup.getPeers() == null) {
Review Comment:
ok
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1718093672
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -453,7 +453,7 @@ public void removeRegionLocation(
getIdWithRpcEndpoint(deprecatedLocation),
status);
configManager.getLoadManager().removeRegionCache(regionId, deprecatedLocation.getDataNodeId());
- configManager.getLoadManager().getRouteBalancer().balanceRegionLeaderAndPriority();
+ // configManager.getLoadManager().getRouteBalancer().balanceRegionLeaderAndPriority();
Review Comment:
ok, i have changed it
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -693,11 +703,22 @@ public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation o
(CONF.getSchemaRegionRatisRpcLeaderElectionTimeoutMaxMs()
+ CONF.getSchemaRegionRatisRpcLeaderElectionTimeoutMinMs())
/ 2;
+ Integer leaderId = configManager.getLoadManager().getRegionLeaderMap().get(regionId);
+
+ if (leaderId != -1) {
+ // The migrated node is not leader, so we specify the transfer leader to current leader node
Review Comment:
I think so, we can't report success
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum merged PR #13178:
URL: https://github.com/apache/iotdb/pull/13178
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1718100475
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -578,15 +597,26 @@ public void resetPeerList(ConsensusGroupId groupId, List<Peer> peers) throws Con
@Override
public void transferLeader(ConsensusGroupId groupId, Peer newLeader) throws ConsensusException {
// first fetch the newest information
+ logger.info("transfer consensusGroup {} leader to {}", groupId, newLeader);
final RaftGroupId raftGroupId = Utils.fromConsensusGroupIdToRaftGroupId(groupId);
final RaftGroup raftGroup =
Optional.ofNullable(getGroupInfo(raftGroupId))
.orElseThrow(() -> new ConsensusGroupNotExistException(groupId));
-
+ if (raftGroup.getPeers() == null) {
Review Comment:
This commit is draft version, I'll delete these code in future commit
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1718226180
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -869,9 +873,34 @@ private RaftClientReply sendReconfiguration(RaftGroup newGroupConf)
// notify the group leader of configuration change
RaftClientReply reply;
try (RatisClient client = getRaftClient(newGroupConf)) {
- reply =
- client.getRaftClient().admin().setConfiguration(new ArrayList<>(newGroupConf.getPeers()));
- if (!reply.isSuccess()) {
+ int basicWaitTime = 500;
+ int maxWaitTime = 10000;
Review Comment:
This is also okay.
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "liyuheng55555 (via GitHub)" <gi...@apache.org>.
liyuheng55555 commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719604597
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -922,18 +922,18 @@ public void allowStaleRead(ConsensusGroupId consensusGroupId) {
private class RatisClientPoolFactory implements IClientPoolFactory<RaftGroup, RatisClient> {
- private boolean isReConfiguration;
+ private final boolean isReconfiguration;
RatisClientPoolFactory(boolean isReConfiguration) {
- this.isReConfiguration = isReConfiguration;
+ this.isReconfiguration = isReConfiguration;
Review Comment:
you forget something
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisClient.java:
##########
@@ -212,46 +218,39 @@ public Action handleAttemptFailure(Event event) {
}
// This policy is used to raft configuration change
+ //
Review Comment:
Use javadoc /** */ instead
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -665,27 +665,26 @@ public void removeDataNodePersistence(TDataNodeLocation dataNodeLocation) {
* @param regionId The region to be migrated
* @param originalDataNode The DataNode where the region locates
*/
- public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation originalDataNode)
- throws ProcedureException, InterruptedException {
- List<TDataNodeLocation> excludeDataNode = Collections.singletonList(originalDataNode);
- transferRegionLeader(regionId, originalDataNode, excludeDataNode);
- }
-
public void transferRegionLeader(
- TConsensusGroupId regionId,
- TDataNodeLocation originalDataNode,
- List<TDataNodeLocation> excludeDataNode)
+ TConsensusGroupId regionId, TDataNodeLocation originalDataNode, TDataNodeLocation coodinator)
throws ProcedureException, InterruptedException {
// find new leader
final int findNewLeaderTimeLimitSecond = 10;
long startTime = System.nanoTime();
Optional<TDataNodeLocation> newLeaderNode = Optional.empty();
+ List<TDataNodeLocation> excludeDataNode = new ArrayList<>();
+ excludeDataNode.add(originalDataNode);
+ excludeDataNode.add(coodinator);
while (System.nanoTime() - startTime < TimeUnit.SECONDS.toNanos(findNewLeaderTimeLimitSecond)) {
newLeaderNode = filterDataNodeWithOtherRegionReplica(regionId, excludeDataNode);
if (newLeaderNode.isPresent()) {
break;
}
}
+ if (!newLeaderNode.isPresent()) {
+ // If we have no choice, we use it
+ newLeaderNode = Optional.of(coodinator);
+ }
Review Comment:
Better than my thought, nice and clean code!
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisClient.java:
##########
@@ -212,46 +218,39 @@ public Action handleAttemptFailure(Event event) {
}
// This policy is used to raft configuration change
+ //
private static class RatisEndlessRetryPolicy implements RetryPolicy {
private static final Logger logger = LoggerFactory.getLogger(RatisEndlessRetryPolicy.class);
- private static final RetryPolicy defaultPolicy;
-
- static {
- String str = "";
- // 50, 500ms, 40, 1000ms, 30, 1500ms, 20, 2000ms, 10, 2500ms
- int basicRetry = 50;
- int basicSleep = 500;
- for (int i = 0; i < 5; i++) {
- str += basicRetry + "," + basicSleep + ",";
- basicRetry -= 10;
- basicSleep += 500;
- }
+ private final RetryPolicy defaultPolicy;
+ RatisEndlessRetryPolicy() {
+ // about 1 hour wait Time.
defaultPolicy =
- MultipleLinearRandomRetry.parseCommaSeparated(str.substring(0, str.length() - 1));
+ RetryPolicies.retryForeverWithSleep(TimeDuration.valueOf(2, TimeUnit.SECONDS));
}
Review Comment:
1. Isn't retry every 2 seconds too frequently ?
2. Why still // about 1 hour wait Time.
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719271371
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -112,6 +115,11 @@ class RatisConsensus implements IConsensus {
private static final Logger logger = LoggerFactory.getLogger(RatisConsensus.class);
+ // The two fields are used to control the retry times and wait time for setConfiguration
+ // reconfiguration may take many time, so we need to set a long wait time and retry times
+ private final long setConfigurationWaitTime = 10000;
+ private final int setConfigurationRetryTimes = 50;
Review Comment:
these codes have been deleted
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719710498
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -667,12 +667,21 @@ public void removeDataNodePersistence(TDataNodeLocation dataNodeLocation) {
*/
public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation originalDataNode)
throws ProcedureException, InterruptedException {
+ List<TDataNodeLocation> excludeDataNode = Collections.singletonList(originalDataNode);
+ transferRegionLeader(regionId, originalDataNode, excludeDataNode);
+ }
+
+ public void transferRegionLeader(
+ TConsensusGroupId regionId,
+ TDataNodeLocation originalDataNode,
+ List<TDataNodeLocation> excludeDataNode)
Review Comment:
It's convenient to use to determine whether an object is included
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719432197
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisClient.java:
##########
@@ -167,4 +210,48 @@ public Action handleAttemptFailure(Event event) {
return defaultPolicy.handleAttemptFailure(event);
}
}
+
+ // This policy is used to raft configuration change
+ private static class RatisEndlessRetryPolicy implements RetryPolicy {
+
+ private static final Logger logger = LoggerFactory.getLogger(RatisEndlessRetryPolicy.class);
+ private static final RetryPolicy defaultPolicy;
+
+ static {
+ String str = "";
+ // 50, 500ms, 40, 1000ms, 30, 1500ms, 20, 2000ms, 10, 2500ms
Review Comment:
yes, the retry time should give a large number.
if the number is enough large, then the loop is ok
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719707094
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisClient.java:
##########
@@ -167,4 +216,41 @@ public Action handleAttemptFailure(Event event) {
return defaultPolicy.handleAttemptFailure(event);
}
}
+
+ // This policy is used to raft configuration change
+ //
+ private static class RatisEndlessRetryPolicy implements RetryPolicy {
+
+ private static final Logger logger = LoggerFactory.getLogger(RatisEndlessRetryPolicy.class);
+ private final RetryPolicy defaultPolicy;
+
+ RatisEndlessRetryPolicy() {
+ // about 1 hour wait Time.
+ defaultPolicy =
+ RetryPolicies.retryForeverWithSleep(TimeDuration.valueOf(2, TimeUnit.SECONDS));
+ }
+
+ @Override
+ public Action handleAttemptFailure(Event event) {
+ if (event.getCause() instanceof ReconfigurationInProgressException) {
Review Comment:
I have 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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719711794
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -718,7 +739,7 @@ public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation o
Collections.singletonMap(
regionId,
new ConsensusGroupHeartbeatSample(timestamp, newLeaderNode.get().getDataNodeId())));
- configManager.getLoadManager().getRouteBalancer().balanceRegionLeaderAndPriority();
+ // configManager.getLoadManager().getRouteBalancer().balanceRegionLeaderAndPriority();
Review Comment:
ok
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719725223
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -864,14 +869,30 @@ private RatisClient getRaftClient(RaftGroup group) throws ClientManagerException
}
}
+ private RatisClient getConfigurationRaftClient(RaftGroup group) throws ClientManagerException {
+ try {
+ return reconfigurationClientManager.borrowClient(group);
Review Comment:
consistent with clientManager
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -578,15 +597,26 @@ public void resetPeerList(ConsensusGroupId groupId, List<Peer> peers) throws Con
@Override
public void transferLeader(ConsensusGroupId groupId, Peer newLeader) throws ConsensusException {
// first fetch the newest information
+ logger.info("transfer consensusGroup {} leader to {}", groupId, newLeader);
final RaftGroupId raftGroupId = Utils.fromConsensusGroupIdToRaftGroupId(groupId);
final RaftGroup raftGroup =
Optional.ofNullable(getGroupInfo(raftGroupId))
.orElseThrow(() -> new ConsensusGroupNotExistException(groupId));
-
+ if (raftGroup.getPeers() == null) {
Review Comment:
remove it
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -665,18 +665,26 @@ public void removeDataNodePersistence(TDataNodeLocation dataNodeLocation) {
* @param regionId The region to be migrated
* @param originalDataNode The DataNode where the region locates
*/
- public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation originalDataNode)
+ public void transferRegionLeader(
+ TConsensusGroupId regionId, TDataNodeLocation originalDataNode, TDataNodeLocation coodinator)
throws ProcedureException, InterruptedException {
// find new leader
final int findNewLeaderTimeLimitSecond = 10;
long startTime = System.nanoTime();
Optional<TDataNodeLocation> newLeaderNode = Optional.empty();
+ List<TDataNodeLocation> excludeDataNode = new ArrayList<>();
+ excludeDataNode.add(originalDataNode);
+ excludeDataNode.add(coodinator);
while (System.nanoTime() - startTime < TimeUnit.SECONDS.toNanos(findNewLeaderTimeLimitSecond)) {
- newLeaderNode = filterDataNodeWithOtherRegionReplica(regionId, originalDataNode);
+ newLeaderNode = filterDataNodeWithOtherRegionReplica(regionId, excludeDataNode);
Review Comment:
cause redundant sleep when replica is 1?
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1718083390
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -693,11 +703,22 @@ public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation o
(CONF.getSchemaRegionRatisRpcLeaderElectionTimeoutMaxMs()
+ CONF.getSchemaRegionRatisRpcLeaderElectionTimeoutMinMs())
/ 2;
+ Integer leaderId = configManager.getLoadManager().getRegionLeaderMap().get(regionId);
+
+ if (leaderId != -1) {
+ // The migrated node is not leader, so we specify the transfer leader to current leader node
Review Comment:
then we do not need to send rpc to datanode?
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -667,12 +667,22 @@ public void removeDataNodePersistence(TDataNodeLocation dataNodeLocation) {
*/
public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation originalDataNode)
throws ProcedureException, InterruptedException {
+ List<TDataNodeLocation> excludeDataNode = new ArrayList<>();
Review Comment:
Collections.singletonList()
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -453,7 +453,7 @@ public void removeRegionLocation(
getIdWithRpcEndpoint(deprecatedLocation),
status);
configManager.getLoadManager().removeRegionCache(regionId, deprecatedLocation.getDataNodeId());
- configManager.getLoadManager().getRouteBalancer().balanceRegionLeaderAndPriority();
+ // configManager.getLoadManager().getRouteBalancer().balanceRegionLeaderAndPriority();
Review Comment:
do not comment this
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -578,15 +597,26 @@ public void resetPeerList(ConsensusGroupId groupId, List<Peer> peers) throws Con
@Override
public void transferLeader(ConsensusGroupId groupId, Peer newLeader) throws ConsensusException {
// first fetch the newest information
+ logger.info("transfer consensusGroup {} leader to {}", groupId, newLeader);
final RaftGroupId raftGroupId = Utils.fromConsensusGroupIdToRaftGroupId(groupId);
final RaftGroup raftGroup =
Optional.ofNullable(getGroupInfo(raftGroupId))
.orElseThrow(() -> new ConsensusGroupNotExistException(groupId));
-
+ if (raftGroup.getPeers() == null) {
Review Comment:
if we forbid leader trasnfer during regionMigration, thus we do not need this judgement?
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -667,12 +667,22 @@ public void removeDataNodePersistence(TDataNodeLocation dataNodeLocation) {
*/
public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation originalDataNode)
throws ProcedureException, InterruptedException {
+ List<TDataNodeLocation> excludeDataNode = new ArrayList<>();
+ excludeDataNode.add(originalDataNode);
Review Comment:
why not use originDataNode directly?
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -578,15 +597,26 @@ public void resetPeerList(ConsensusGroupId groupId, List<Peer> peers) throws Con
@Override
public void transferLeader(ConsensusGroupId groupId, Peer newLeader) throws ConsensusException {
// first fetch the newest information
+ logger.info("transfer consensusGroup {} leader to {}", groupId, newLeader);
final RaftGroupId raftGroupId = Utils.fromConsensusGroupIdToRaftGroupId(groupId);
final RaftGroup raftGroup =
Optional.ofNullable(getGroupInfo(raftGroupId))
.orElseThrow(() -> new ConsensusGroupNotExistException(groupId));
-
+ if (raftGroup.getPeers() == null) {
+ throw new ConsensusException("group " + groupId + " has no peers");
+ }
final RaftPeer newRaftLeader = Utils.fromPeerAndPriorityToRaftPeer(newLeader, DEFAULT_PRIORITY);
-
final RaftClientReply reply;
try {
+ Peer leader = getLeader(groupId);
Review Comment:
same as above, not necessary if we consider this in condignode
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -718,7 +739,7 @@ public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation o
Collections.singletonMap(
regionId,
new ConsensusGroupHeartbeatSample(timestamp, newLeaderNode.get().getDataNodeId())));
- configManager.getLoadManager().getRouteBalancer().balanceRegionLeaderAndPriority();
+ // configManager.getLoadManager().getRouteBalancer().balanceRegionLeaderAndPriority();
Review Comment:
do not comment
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -112,6 +115,11 @@ class RatisConsensus implements IConsensus {
private static final Logger logger = LoggerFactory.getLogger(RatisConsensus.class);
+ // The two fields are used to control the retry times and wait time for setConfiguration
+ // reconfiguration may take many time, so we need to set a long wait time and retry times
+ private final long setConfigurationWaitTime = 10000;
+ private final int setConfigurationRetryTimes = 50;
Review Comment:
not used?
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -880,6 +936,30 @@ private RaftClientReply sendReconfiguration(RaftGroup newGroupConf)
return reply;
}
+ private RaftClientReply sendReconfigurationWithRetry(RaftGroup newGroupConf)
Review Comment:
not used?
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -186,6 +195,16 @@ public RatisConsensus(ConsensusConfig config, IStateMachine.Registry registry) {
this.config.getImpl().getRetryWaitMillis(), TimeUnit.MILLISECONDS))
.build();
+ regionMigrateRetryPolicy =
Review Comment:
not used?
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RegionMaintainHandler.java:
##########
@@ -742,12 +763,29 @@ public void transferRegionLeader(TConsensusGroupId regionId, TDataNodeLocation o
*/
public Optional<TDataNodeLocation> filterDataNodeWithOtherRegionReplica(
TConsensusGroupId regionId, TDataNodeLocation filterLocation) {
+ List<TDataNodeLocation> filterLocations = new ArrayList<>();
+ filterLocations.add(filterLocation);
+ return filterDataNodeWithOtherRegionReplica(regionId, filterLocations);
+ }
+
+ public Optional<TDataNodeLocation> filterDataNodeWithOtherRegionReplica(
+ TConsensusGroupId regionId, List<TDataNodeLocation> filterLocations) {
return filterDataNodeWithOtherRegionReplica(
- regionId, filterLocation, NodeStatus.Running, NodeStatus.ReadOnly);
+ regionId, filterLocations, NodeStatus.Running, NodeStatus.ReadOnly);
}
public Optional<TDataNodeLocation> filterDataNodeWithOtherRegionReplica(
TConsensusGroupId regionId, TDataNodeLocation filterLocation, NodeStatus... allowingStatus) {
+ List<TDataNodeLocation> excludeLocations = new ArrayList<>();
+ excludeLocations.add(filterLocation);
Review Comment:
same as above
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1718098970
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -186,6 +195,16 @@ public RatisConsensus(ConsensusConfig config, IStateMachine.Registry registry) {
this.config.getImpl().getRetryWaitMillis(), TimeUnit.MILLISECONDS))
.build();
+ regionMigrateRetryPolicy =
Review Comment:
I'm considering which one to use
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719315668
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -122,6 +122,7 @@ class RatisConsensus implements IConsensus {
private final RaftClientRpc clientRpc;
private final IClientManager<RaftGroup, RatisClient> clientManager;
+ private final IClientManager<RaftGroup, RatisClient> reconfClientManager;
Review Comment:
ok:)
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719618935
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisClient.java:
##########
@@ -212,46 +218,39 @@ public Action handleAttemptFailure(Event event) {
}
// This policy is used to raft configuration change
+ //
private static class RatisEndlessRetryPolicy implements RetryPolicy {
private static final Logger logger = LoggerFactory.getLogger(RatisEndlessRetryPolicy.class);
- private static final RetryPolicy defaultPolicy;
-
- static {
- String str = "";
- // 50, 500ms, 40, 1000ms, 30, 1500ms, 20, 2000ms, 10, 2500ms
- int basicRetry = 50;
- int basicSleep = 500;
- for (int i = 0; i < 5; i++) {
- str += basicRetry + "," + basicSleep + ",";
- basicRetry -= 10;
- basicSleep += 500;
- }
+ private final RetryPolicy defaultPolicy;
+ RatisEndlessRetryPolicy() {
+ // about 1 hour wait Time.
defaultPolicy =
- MultipleLinearRandomRetry.parseCommaSeparated(str.substring(0, str.length() - 1));
+ RetryPolicies.retryForeverWithSleep(TimeDuration.valueOf(2, TimeUnit.SECONDS));
}
Review Comment:
1. if reply return false, then resend request after 2s. Instead of sending it every 2 seconds
sorry, I forget delete comment
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisClient.java:
##########
@@ -212,46 +218,39 @@ public Action handleAttemptFailure(Event event) {
}
// This policy is used to raft configuration change
+ //
private static class RatisEndlessRetryPolicy implements RetryPolicy {
private static final Logger logger = LoggerFactory.getLogger(RatisEndlessRetryPolicy.class);
- private static final RetryPolicy defaultPolicy;
-
- static {
- String str = "";
- // 50, 500ms, 40, 1000ms, 30, 1500ms, 20, 2000ms, 10, 2500ms
- int basicRetry = 50;
- int basicSleep = 500;
- for (int i = 0; i < 5; i++) {
- str += basicRetry + "," + basicSleep + ",";
- basicRetry -= 10;
- basicSleep += 500;
- }
+ private final RetryPolicy defaultPolicy;
+ RatisEndlessRetryPolicy() {
+ // about 1 hour wait Time.
defaultPolicy =
- MultipleLinearRandomRetry.parseCommaSeparated(str.substring(0, str.length() - 1));
+ RetryPolicies.retryForeverWithSleep(TimeDuration.valueOf(2, TimeUnit.SECONDS));
}
Review Comment:
1. if reply return false, then resend request after 2s. Instead of sending it every 2 seconds
2. sorry, I forget delete comment
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719711450
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -880,6 +936,30 @@ private RaftClientReply sendReconfiguration(RaftGroup newGroupConf)
return reply;
}
+ private RaftClientReply sendReconfigurationWithRetry(RaftGroup newGroupConf)
Review Comment:
I have used another policy
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719604769
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisClient.java:
##########
@@ -167,4 +216,41 @@ public Action handleAttemptFailure(Event event) {
return defaultPolicy.handleAttemptFailure(event);
}
}
+
+ // This policy is used to raft configuration change
+ //
+ private static class RatisEndlessRetryPolicy implements RetryPolicy {
+
+ private static final Logger logger = LoggerFactory.getLogger(RatisEndlessRetryPolicy.class);
+ private final RetryPolicy defaultPolicy;
+
+ RatisEndlessRetryPolicy() {
+ // about 1 hour wait Time.
+ defaultPolicy =
+ RetryPolicies.retryForeverWithSleep(TimeDuration.valueOf(2, TimeUnit.SECONDS));
+ }
+
+ @Override
+ public Action handleAttemptFailure(Event event) {
+ if (event.getCause() instanceof ReconfigurationInProgressException) {
Review Comment:
haha! I'm lazy here :) . This part is the copilot prompt, and the tab is generated with one click.
--
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
Re: [PR] RegionMigrate: Fix migrating region with ratisConsensus cause the region is unavailable. [iotdb]
Posted by "133tosakarin (via GitHub)" <gi...@apache.org>.
133tosakarin commented on code in PR #13178:
URL: https://github.com/apache/iotdb/pull/13178#discussion_r1719609305
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisClient.java:
##########
@@ -212,46 +218,39 @@ public Action handleAttemptFailure(Event event) {
}
// This policy is used to raft configuration change
+ //
Review Comment:
ok
--
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