You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ratis.apache.org by GitBox <gi...@apache.org> on 2020/07/13 08:51:27 UTC
[GitHub] [incubator-ratis] GlenGeng opened a new pull request #150: RATIS-981: Step-down stale leader in case of split-brain
GlenGeng opened a new pull request #150:
URL: https://github.com/apache/incubator-ratis/pull/150
## What changes were proposed in this pull request?
/**
* See the thesis section 6.2: A leader in Raft steps down
* if an election timeout elapses without a successful
* round of heartbeats to a majority of its cluster.
*/
## What is the link to the Apache JIRA
https://issues.apache.org/jira/projects/RATIS/issues/RATIS-981?filter=allissues
## How was this patch tested?
CI
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] GlenGeng commented on a change in pull request #150: RATIS-981: Step-down stale leader in case of split-brain
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #150:
URL: https://github.com/apache/incubator-ratis/pull/150#discussion_r454128140
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/PeerConfiguration.java
##########
@@ -90,11 +90,8 @@ boolean hasMajority(Collection<RaftPeerId> others, RaftPeerId selfId) {
if (contains(other)) {
num++;
}
- if (num > size() / 2) {
- return true;
- }
}
- return false;
+ return num > size() / 2;
Review comment:
After this change, it can handle single-server cluster.
Previous one can not handle such case, since its `others` is empty.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #150: RATIS-981: Step-down stale leader in case of split-brain
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #150:
URL: https://github.com/apache/incubator-ratis/pull/150#discussion_r454121712
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/PeerConfiguration.java
##########
@@ -90,11 +90,8 @@ boolean hasMajority(Collection<RaftPeerId> others, RaftPeerId selfId) {
if (contains(other)) {
num++;
}
- if (num > size() / 2) {
- return true;
- }
}
- return false;
+ return num > size() / 2;
Review comment:
why change this ? looks like equivalent with origin code.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] GlenGeng commented on a change in pull request #150: RATIS-981: Step-down stale leader in case of split-brain
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #150:
URL: https://github.com/apache/incubator-ratis/pull/150#discussion_r457018981
##########
File path: ratis-test/src/test/java/org/apache/ratis/grpc/TestRetryCacheWithGrpc.java
##########
@@ -45,6 +47,11 @@
public void testRetryOnResourceUnavailableException()
throws InterruptedException, IOException {
RaftProperties properties = new RaftProperties();
+
+ // make sure leadership check won't affect the test
Review comment:
test cases invoking `SimpleStateMachine4Testing::blockWriteStateMachineData()` or `DelayLocalExecutionInjection::setDelayMs()` will be affected, which is meet expectation. We need enlarge election timeout to make sure leader won't step down before hearing append entries response from majority followers.
Another case is related to the membership change, especially the one with 9-servers. To avoid flaky behavior in CI environment, we need enlarge election timeout a little bit, say from 300s to 1s, since leader is vulnerable during such situation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] GlenGeng commented on a change in pull request #150: RATIS-981: Step-down stale leader in case of split-brain
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #150:
URL: https://github.com/apache/incubator-ratis/pull/150#discussion_r456377784
##########
File path: ratis-examples/src/test/java/org/apache/ratis/examples/ParameterizedBaseTest.java
##########
@@ -123,6 +126,11 @@ public boolean hasNext() {
final RaftProperties prop = new RaftProperties();
prop.setClass(MiniRaftCluster.STATEMACHINE_CLASS_KEY,
stateMachineClass, StateMachine.class);
+
+ // make sure leadership check won't affect the test
+ RaftServerConfigKeys.Rpc.setTimeoutMin(prop, TimeDuration.valueOf(2, TimeUnit.SECONDS));
+ RaftServerConfigKeys.Rpc.setTimeoutMax(prop, TimeDuration.valueOf(4, TimeUnit.SECONDS));
Review comment:
Yes, some test assume that leader will not step down even it can not hear from followers.
Such as `runTestUpdateViaHeartbeat` in `TestRaftWithGrpc`, after leader is elected, it just block followers's append requests for 5 seconds, then unblock them
```
waitForLeader(cluster);
try (final RaftClient client = cluster.createClient()) {
// block append requests
cluster.getServerAliveStream()
.filter(impl -> !impl.isLeader())
.map(SimpleStateMachine4Testing::get)
.forEach(SimpleStateMachine4Testing::blockWriteStateMachineData);
CompletableFuture<RaftClientReply>
replyFuture = client.sendAsync(new RaftTestUtil.SimpleMessage("abc"));
TimeDuration.valueOf(5 , TimeUnit.SECONDS).sleep();
// replyFuture should not be completed until append request is unblocked.
Assert.assertTrue(!replyFuture.isDone());
// unblock append request.
cluster.getServerAliveStream()
.filter(impl -> !impl.isLeader())
.map(SimpleStateMachine4Testing::get)
.forEach(SimpleStateMachine4Testing::unblockWriteStateMachineData);
```
Such case will failed due to `checkLeadership`, thus I have to enlarge election timeout to avoid leader step down during such case.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] GlenGeng commented on a change in pull request #150: RATIS-981: Step-down stale leader in case of split-brain
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #150:
URL: https://github.com/apache/incubator-ratis/pull/150#discussion_r457903716
##########
File path: ratis-examples/src/test/java/org/apache/ratis/examples/ParameterizedBaseTest.java
##########
@@ -123,6 +126,11 @@ public boolean hasNext() {
final RaftProperties prop = new RaftProperties();
prop.setClass(MiniRaftCluster.STATEMACHINE_CLASS_KEY,
stateMachineClass, StateMachine.class);
+
+ // make sure leadership check won't affect the test
+ RaftServerConfigKeys.Rpc.setTimeoutMin(prop, TimeDuration.valueOf(2, TimeUnit.SECONDS));
+ RaftServerConfigKeys.Rpc.setTimeoutMax(prop, TimeDuration.valueOf(4, TimeUnit.SECONDS));
Review comment:
Fix above comments:
After enable and adding some trace, Leader do send heartbeats after sending the append entries request which will be blocked by StateMachine$DataApi#write, but `GrpcServerProtocolService::ServerRequestStreamObserver::onNext()` on raft server side was not called,
thus
```
if (!replyInOrder(request)) {
try {
process(request).thenAccept(this::handleReply);
} catch (Throwable e) {
handleError(e, request);
}
return;
}
```
does not take effect for heartbeats.
Currently, not sure if this is the desired behavior of grpc bi-direction stream or not.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] GlenGeng commented on a change in pull request #150: RATIS-981: Step-down stale leader in case of split-brain
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #150:
URL: https://github.com/apache/incubator-ratis/pull/150#discussion_r457021240
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderState.java
##########
@@ -787,6 +789,46 @@ private void replicateNewConf() {
return lists;
}
+ /**
+ * See the thesis section 6.2: A leader in Raft steps down
+ * if an election timeout elapses without a successful
+ * round of heartbeats to a majority of its cluster.
+ */
+ private void checkLeadership() {
+ // The initial value of lastRpcResponseTime in FollowerInfo is set by
+ // LeaderState::addSenders(), which is fake and used to trigger an
+ // immediate round of AppendEntries request. To ensure that leader
+ // is stable, just ignore the check if become leader soon.
+ if (server.getRole().getRoleElapsedTimeMs() < 3 * server.getMaxTimeoutMs()) {
Review comment:
Thanks for pointing this. I will remove this magic number 3.
Basically, candidates collect votes from majority before becoming leader. Since response time of request votes is recoded, If leader just take charge, without seeing higher term, ideally it should be legal for election timeout.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] GlenGeng commented on a change in pull request #150: RATIS-981: Step-down stale leader in case of split-brain
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #150:
URL: https://github.com/apache/incubator-ratis/pull/150#discussion_r462238086
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderState.java
##########
@@ -792,6 +794,47 @@ private void replicateNewConf() {
return lists;
}
+ /**
+ * See the thesis section 6.2: A leader in Raft steps down
+ * if an election timeout elapses without a successful
+ * round of heartbeats to a majority of its cluster.
+ */
+ private void checkLeadership() {
+ // The initial value of lastRpcResponseTime in FollowerInfo is set by
+ // LeaderState::addSenders(), which is fake and used to trigger an
+ // immediate round of AppendEntries request. Since candidates collect
+ // votes from majority before becoming leader, without seeing higher term,
+ // ideally, A leader is legal for election timeout if become leader soon.
+ if (server.getRole().getRoleElapsedTimeMs() < server.getMaxTimeoutMs()) {
+ return;
+ }
+
+ final List<RaftPeerId> activePeers = senders.stream()
+ .filter(sender -> sender.getFollower()
+ .getLastRpcResponseTime()
+ .elapsedTimeMs() <= server.getMaxTimeoutMs())
+ .map(sender -> sender.getFollower().getPeer().getId())
+ .collect(Collectors.toList());
+
+ final RaftConfiguration conf = server.getRaftConf();
+
+ if (conf.hasMajority(activePeers, server.getId())) {
+ // leadership check passed
+ return;
+ }
+
+ List<FollowerInfo> followers = senders.stream()
+ .map(LogAppender::getFollower).collect(Collectors.toList());
+
+ LOG.warn(this + ": Lost leadership on term: " + currentTerm
+ + ". Election timeout: " + server.getMaxTimeoutMs() + "ms"
+ + ". In charge for: " + server.getRole().getRoleElapsedTimeMs() + "ms"
+ + ". Conf: " + conf + ". Followers: " + followers);
+
+ // become follower of next term.
+ stepDown(currentTerm + 1);
Review comment:
ok
##########
File path: ratis-server/src/test/java/org/apache/ratis/RaftAsyncTests.java
##########
@@ -402,6 +407,46 @@ void runTestAppendEntriesTimeout(CLUSTER cluster) throws Exception {
RaftServerConfigKeys.RetryCache.setExpiryTime(getProperties(), oldExpiryTime);
}
+ @Test
+ public void testCheckLeadershipFailure() throws Exception {
+ runWithNewCluster(NUM_SERVERS, this::runTestCheckLeadershipFailure);
+ }
+
+ void runTestCheckLeadershipFailure(CLUSTER cluster) throws Exception {
Review comment:
"This test can also pass when followers turn to candidate state and trigger an election."
We don't need to worry about this, the `outstandingOp` will prevent follower to timeout.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] GlenGeng commented on a change in pull request #150: RATIS-981: Step-down stale leader in case of split-brain
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #150:
URL: https://github.com/apache/incubator-ratis/pull/150#discussion_r456742216
##########
File path: ratis-examples/src/test/java/org/apache/ratis/examples/ParameterizedBaseTest.java
##########
@@ -123,6 +126,11 @@ public boolean hasNext() {
final RaftProperties prop = new RaftProperties();
prop.setClass(MiniRaftCluster.STATEMACHINE_CLASS_KEY,
stateMachineClass, StateMachine.class);
+
+ // make sure leadership check won't affect the test
+ RaftServerConfigKeys.Rpc.setTimeoutMin(prop, TimeDuration.valueOf(2, TimeUnit.SECONDS));
+ RaftServerConfigKeys.Rpc.setTimeoutMax(prop, TimeDuration.valueOf(4, TimeUnit.SECONDS));
Review comment:
Hi @lokeshj1703
raft use bi-direction streaming grpc, it will maintain sequence between requests and sequence between responses.
Leader do send heartbeats after sending the append entries request which will be blocked by StateMachine$DataApi#write, but it won't receive replies of these heartbeats until receiving reply of that append entries response.
This sequence assurance is important for maintain log match property of raft.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] lokeshj1703 commented on a change in pull request #150: RATIS-981: Step-down stale leader in case of split-brain
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #150:
URL: https://github.com/apache/incubator-ratis/pull/150#discussion_r457276401
##########
File path: ratis-examples/src/test/java/org/apache/ratis/examples/ParameterizedBaseTest.java
##########
@@ -123,6 +126,11 @@ public boolean hasNext() {
final RaftProperties prop = new RaftProperties();
prop.setClass(MiniRaftCluster.STATEMACHINE_CLASS_KEY,
stateMachineClass, StateMachine.class);
+
+ // make sure leadership check won't affect the test
+ RaftServerConfigKeys.Rpc.setTimeoutMin(prop, TimeDuration.valueOf(2, TimeUnit.SECONDS));
+ RaftServerConfigKeys.Rpc.setTimeoutMax(prop, TimeDuration.valueOf(4, TimeUnit.SECONDS));
Review comment:
@GlenGeng Heartbeats are out of band in ratis. It does not violate the log matching property as leader still waits for reply from appendEntries. Heartbeats are required to maintain liveness.
Please check GrpcServerProtocolService#appendEntries.
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderState.java
##########
@@ -792,6 +794,47 @@ private void replicateNewConf() {
return lists;
}
+ /**
+ * See the thesis section 6.2: A leader in Raft steps down
+ * if an election timeout elapses without a successful
+ * round of heartbeats to a majority of its cluster.
+ */
+ private void checkLeadership() {
+ // The initial value of lastRpcResponseTime in FollowerInfo is set by
+ // LeaderState::addSenders(), which is fake and used to trigger an
+ // immediate round of AppendEntries request. Since candidates collect
+ // votes from majority before becoming leader, without seeing higher term,
+ // ideally, A leader is legal for election timeout if become leader soon.
+ if (server.getRole().getRoleElapsedTimeMs() < server.getMaxTimeoutMs()) {
+ return;
+ }
+
+ final List<RaftPeerId> activePeers = senders.stream()
+ .filter(sender -> sender.getFollower()
+ .getLastRpcResponseTime()
+ .elapsedTimeMs() <= server.getMaxTimeoutMs())
+ .map(sender -> sender.getFollower().getPeer().getId())
+ .collect(Collectors.toList());
+
+ final RaftConfiguration conf = server.getRaftConf();
+
+ if (conf.hasMajority(activePeers, server.getId())) {
+ // leadership check passed
+ return;
+ }
+
+ List<FollowerInfo> followers = senders.stream()
+ .map(LogAppender::getFollower).collect(Collectors.toList());
+
+ LOG.warn(this + ": Lost leadership on term: " + currentTerm
+ + ". Election timeout: " + server.getMaxTimeoutMs() + "ms"
+ + ". In charge for: " + server.getRole().getRoleElapsedTimeMs() + "ms"
+ + ". Conf: " + conf + ". Followers: " + followers);
+
+ // become follower of next term.
+ stepDown(currentTerm + 1);
Review comment:
I think this should be currentTerm.
##########
File path: ratis-server/src/test/java/org/apache/ratis/RaftAsyncTests.java
##########
@@ -402,6 +407,46 @@ void runTestAppendEntriesTimeout(CLUSTER cluster) throws Exception {
RaftServerConfigKeys.RetryCache.setExpiryTime(getProperties(), oldExpiryTime);
}
+ @Test
+ public void testCheckLeadershipFailure() throws Exception {
+ runWithNewCluster(NUM_SERVERS, this::runTestCheckLeadershipFailure);
+ }
+
+ void runTestCheckLeadershipFailure(CLUSTER cluster) throws Exception {
Review comment:
This test can also pass when followers turn to candidate state and trigger an election. How about we delay REQUEST_VOTE in the test? And then check the state of current leader before and after timeout?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] GlenGeng commented on a change in pull request #150: RATIS-981: Step-down stale leader in case of split-brain
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #150:
URL: https://github.com/apache/incubator-ratis/pull/150#discussion_r457019384
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderState.java
##########
@@ -787,6 +789,46 @@ private void replicateNewConf() {
return lists;
}
+ /**
+ * See the thesis section 6.2: A leader in Raft steps down
+ * if an election timeout elapses without a successful
+ * round of heartbeats to a majority of its cluster.
+ */
+ private void checkLeadership() {
+ // The initial value of lastRpcResponseTime in FollowerInfo is set by
+ // LeaderState::addSenders(), which is fake and used to trigger an
+ // immediate round of AppendEntries request. To ensure that leader
+ // is stable, just ignore the check if become leader soon.
+ if (server.getRole().getRoleElapsedTimeMs() < 3 * server.getMaxTimeoutMs()) {
+ return;
+ }
+
+ final List<RaftPeerId> activePeers = senders.stream()
+ .filter(sender -> sender.getFollower()
+ .getLastRpcResponseTime()
+ .elapsedTimeMs() <= server.getMaxTimeoutMs())
+ .map(sender -> sender.getFollower().getPeer().getId())
+ .collect(Collectors.toList());
+
+ final RaftConfiguration conf = server.getRaftConf();
+
+ if (conf.hasMajority(activePeers, server.getId())) {
+ // leadership check passed
+ return;
+ }
+
+ List<FollowerInfo> followers = senders.stream()
+ .map(LogAppender::getFollower).collect(Collectors.toList());
+
+ LOG.warn(this + ": Lost leadership on term: " + currentTerm
+ + ". Election timeout: " + server.getMaxTimeoutMs() + "ms"
+ + ". In charge for: " + server.getRole().getRoleElapsedTimeMs() + "ms"
+ + ". Conf: " + conf + ". Followers: " + followers);
Review comment:
Yes, it works well. Relies on `List::toString` and `FollowerInfo::toString`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] GlenGeng commented on a change in pull request #150: RATIS-981: Step-down stale leader in case of split-brain
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #150:
URL: https://github.com/apache/incubator-ratis/pull/150#discussion_r456374982
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderState.java
##########
@@ -787,6 +789,46 @@ private void replicateNewConf() {
return lists;
}
+ /**
+ * See the thesis section 6.2: A leader in Raft steps down
+ * if an election timeout elapses without a successful
+ * round of heartbeats to a majority of its cluster.
+ */
+ private void checkLeadership() {
+ // The initial value of lastRpcResponseTime in FollowerInfo is set by
+ // LeaderState::addSenders(), which is fake and used to trigger an
+ // immediate round of AppendEntries request. To ensure that leader
+ // is stable, just ignore the check if become leader soon.
+ if (server.getRole().getRoleElapsedTimeMs() < 3 * server.getMaxTimeoutMs()) {
+ return;
+ }
+
+ final List<RaftPeerId> activePeers = senders.stream()
+ .filter(sender -> sender.getFollower()
+ .getLastRpcResponseTime()
+ .elapsedTimeMs() <= server.getMaxTimeoutMs())
Review comment:
I am afraid not. Since the criterial is "A leader in Raft steps down if an election timeout elapses without a successful round of heartbeats to a majority of its cluster"
##########
File path: ratis-examples/src/test/java/org/apache/ratis/examples/ParameterizedBaseTest.java
##########
@@ -123,6 +126,11 @@ public boolean hasNext() {
final RaftProperties prop = new RaftProperties();
prop.setClass(MiniRaftCluster.STATEMACHINE_CLASS_KEY,
stateMachineClass, StateMachine.class);
+
+ // make sure leadership check won't affect the test
+ RaftServerConfigKeys.Rpc.setTimeoutMin(prop, TimeDuration.valueOf(2, TimeUnit.SECONDS));
+ RaftServerConfigKeys.Rpc.setTimeoutMax(prop, TimeDuration.valueOf(4, TimeUnit.SECONDS));
Review comment:
Yes, some test assume that leader will not step down even it can not hear from followers.
Such as `runTestUpdateViaHeartbeat` in `TestRaftWithGrpc`, after leader is elected, it just block followers's append requests for 5 seconds, then unblock them
```
waitForLeader(cluster);
try (final RaftClient client = cluster.createClient()) {
// block append requests
cluster.getServerAliveStream()
.filter(impl -> !impl.isLeader())
.map(SimpleStateMachine4Testing::get)
.forEach(SimpleStateMachine4Testing::blockWriteStateMachineData);
CompletableFuture<RaftClientReply>
replyFuture = client.sendAsync(new RaftTestUtil.SimpleMessage("abc"));
TimeDuration.valueOf(5 , TimeUnit.SECONDS).sleep();
// replyFuture should not be completed until append request is unblocked.
Assert.assertTrue(!replyFuture.isDone());
// unblock append request.
cluster.getServerAliveStream()
.filter(impl -> !impl.isLeader())
.map(SimpleStateMachine4Testing::get)
.forEach(SimpleStateMachine4Testing::unblockWriteStateMachineData);
```
Such case will failed due to `checkLeadership`
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/PeerConfiguration.java
##########
@@ -90,11 +90,8 @@ boolean hasMajority(Collection<RaftPeerId> others, RaftPeerId selfId) {
if (contains(other)) {
num++;
}
- if (num > size() / 2) {
- return true;
- }
}
- return false;
+ return num > size() / 2;
Review comment:
For single-server cluster, `others` will be empty, `for` loop will not be entered,
```
if (num > size() / 2) {
return true;
}
```
will be un-reachable.
Even `num = 1` will return `false`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] lokeshj1703 closed pull request #150: RATIS-981: Step-down stale leader in case of split-brain
Posted by GitBox <gi...@apache.org>.
lokeshj1703 closed pull request #150:
URL: https://github.com/apache/incubator-ratis/pull/150
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] lokeshj1703 commented on a change in pull request #150: RATIS-981: Step-down stale leader in case of split-brain
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #150:
URL: https://github.com/apache/incubator-ratis/pull/150#discussion_r456362723
##########
File path: ratis-examples/src/test/java/org/apache/ratis/examples/ParameterizedBaseTest.java
##########
@@ -123,6 +126,11 @@ public boolean hasNext() {
final RaftProperties prop = new RaftProperties();
prop.setClass(MiniRaftCluster.STATEMACHINE_CLASS_KEY,
stateMachineClass, StateMachine.class);
+
+ // make sure leadership check won't affect the test
+ RaftServerConfigKeys.Rpc.setTimeoutMin(prop, TimeDuration.valueOf(2, TimeUnit.SECONDS));
+ RaftServerConfigKeys.Rpc.setTimeoutMax(prop, TimeDuration.valueOf(4, TimeUnit.SECONDS));
Review comment:
We should not ideally require setting timeout in the tests. Are we experiencing flakiness in the tests due to the change?
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderState.java
##########
@@ -787,6 +789,46 @@ private void replicateNewConf() {
return lists;
}
+ /**
+ * See the thesis section 6.2: A leader in Raft steps down
+ * if an election timeout elapses without a successful
+ * round of heartbeats to a majority of its cluster.
+ */
+ private void checkLeadership() {
+ // The initial value of lastRpcResponseTime in FollowerInfo is set by
+ // LeaderState::addSenders(), which is fake and used to trigger an
+ // immediate round of AppendEntries request. To ensure that leader
+ // is stable, just ignore the check if become leader soon.
+ if (server.getRole().getRoleElapsedTimeMs() < 3 * server.getMaxTimeoutMs()) {
+ return;
+ }
+
+ final List<RaftPeerId> activePeers = senders.stream()
+ .filter(sender -> sender.getFollower()
+ .getLastRpcResponseTime()
+ .elapsedTimeMs() <= server.getMaxTimeoutMs())
Review comment:
Should we have a separate config for the timeout value? I think it might be better to have a separate config.
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/PeerConfiguration.java
##########
@@ -90,11 +90,8 @@ boolean hasMajority(Collection<RaftPeerId> others, RaftPeerId selfId) {
if (contains(other)) {
num++;
}
- if (num > size() / 2) {
- return true;
- }
}
- return false;
+ return num > size() / 2;
Review comment:
`if (contains(selfId)) {
num++;
}`
This check should handle single server case.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] GlenGeng commented on a change in pull request #150: RATIS-981: Step-down stale leader in case of split-brain
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #150:
URL: https://github.com/apache/incubator-ratis/pull/150#discussion_r456431437
##########
File path: ratis-examples/src/test/java/org/apache/ratis/examples/ParameterizedBaseTest.java
##########
@@ -123,6 +126,11 @@ public boolean hasNext() {
final RaftProperties prop = new RaftProperties();
prop.setClass(MiniRaftCluster.STATEMACHINE_CLASS_KEY,
stateMachineClass, StateMachine.class);
+
+ // make sure leadership check won't affect the test
+ RaftServerConfigKeys.Rpc.setTimeoutMin(prop, TimeDuration.valueOf(2, TimeUnit.SECONDS));
+ RaftServerConfigKeys.Rpc.setTimeoutMax(prop, TimeDuration.valueOf(4, TimeUnit.SECONDS));
Review comment:
appendEntries in Follower is blocked by StateMachine$DataApi#write, thus won't reply append entries response, which will cause leader to step down.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #150: RATIS-981: Step-down stale leader in case of split-brain
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #150:
URL: https://github.com/apache/incubator-ratis/pull/150#discussion_r454127796
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderState.java
##########
@@ -787,6 +789,46 @@ private void replicateNewConf() {
return lists;
}
+ /**
+ * See the thesis section 6.2: A leader in Raft steps down
+ * if an election timeout elapses without a successful
+ * round of heartbeats to a majority of its cluster.
+ */
+ private void checkLeadership() {
+ // The initial value of lastRpcResponseTime in FollowerInfo is set by
+ // LeaderState::addSenders(), which is fake and used to trigger an
+ // immediate round of AppendEntries request. To ensure that leader
+ // is stable, just ignore the check if become leader soon.
+ if (server.getRole().getRoleElapsedTimeMs() < 3 * server.getMaxTimeoutMs()) {
Review comment:
why 3 is the best choice ? Besides if this is only for ensure leader stable when LeaderState::addSenders(), is there better solution ?
##########
File path: ratis-test/src/test/java/org/apache/ratis/grpc/TestRetryCacheWithGrpc.java
##########
@@ -45,6 +47,11 @@
public void testRetryOnResourceUnavailableException()
throws InterruptedException, IOException {
RaftProperties properties = new RaftProperties();
+
+ // make sure leadership check won't affect the test
Review comment:
We need change a lot of places to make sure leadership check won't affect the test. Is there a better solution ?
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderState.java
##########
@@ -787,6 +789,46 @@ private void replicateNewConf() {
return lists;
}
+ /**
+ * See the thesis section 6.2: A leader in Raft steps down
+ * if an election timeout elapses without a successful
+ * round of heartbeats to a majority of its cluster.
+ */
+ private void checkLeadership() {
+ // The initial value of lastRpcResponseTime in FollowerInfo is set by
+ // LeaderState::addSenders(), which is fake and used to trigger an
+ // immediate round of AppendEntries request. To ensure that leader
+ // is stable, just ignore the check if become leader soon.
+ if (server.getRole().getRoleElapsedTimeMs() < 3 * server.getMaxTimeoutMs()) {
+ return;
+ }
+
+ final List<RaftPeerId> activePeers = senders.stream()
+ .filter(sender -> sender.getFollower()
+ .getLastRpcResponseTime()
+ .elapsedTimeMs() <= server.getMaxTimeoutMs())
+ .map(sender -> sender.getFollower().getPeer().getId())
+ .collect(Collectors.toList());
+
+ final RaftConfiguration conf = server.getRaftConf();
+
+ if (conf.hasMajority(activePeers, server.getId())) {
+ // leadership check passed
+ return;
+ }
+
+ List<FollowerInfo> followers = senders.stream()
+ .map(LogAppender::getFollower).collect(Collectors.toList());
+
+ LOG.warn(this + ": Lost leadership on term: " + currentTerm
+ + ". Election timeout: " + server.getMaxTimeoutMs() + "ms"
+ + ". In charge for: " + server.getRole().getRoleElapsedTimeMs() + "ms"
+ + ". Conf: " + conf + ". Followers: " + followers);
Review comment:
Can log print the item of followers ? I'm not sure about 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] lokeshj1703 commented on pull request #150: RATIS-981: Step-down stale leader in case of split-brain
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #150:
URL: https://github.com/apache/incubator-ratis/pull/150#issuecomment-666938196
@GlenGeng Thanks for the contribution! @runzhiwang Thanks for the review! I have committed the PR to master branch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] GlenGeng commented on a change in pull request #150: RATIS-981: Step-down stale leader in case of split-brain
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #150:
URL: https://github.com/apache/incubator-ratis/pull/150#discussion_r457021240
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderState.java
##########
@@ -787,6 +789,46 @@ private void replicateNewConf() {
return lists;
}
+ /**
+ * See the thesis section 6.2: A leader in Raft steps down
+ * if an election timeout elapses without a successful
+ * round of heartbeats to a majority of its cluster.
+ */
+ private void checkLeadership() {
+ // The initial value of lastRpcResponseTime in FollowerInfo is set by
+ // LeaderState::addSenders(), which is fake and used to trigger an
+ // immediate round of AppendEntries request. To ensure that leader
+ // is stable, just ignore the check if become leader soon.
+ if (server.getRole().getRoleElapsedTimeMs() < 3 * server.getMaxTimeoutMs()) {
Review comment:
Thanks for pointing this. I will remove this magic number 3.
Basically, candidates collect votes from majority before becoming leader. Since response time of request votes is not recoded, If leader just take charge, without seeing higher term, ideally it should be legal for election timeout.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] lokeshj1703 commented on a change in pull request #150: RATIS-981: Step-down stale leader in case of split-brain
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #150:
URL: https://github.com/apache/incubator-ratis/pull/150#discussion_r456392331
##########
File path: ratis-examples/src/test/java/org/apache/ratis/examples/ParameterizedBaseTest.java
##########
@@ -123,6 +126,11 @@ public boolean hasNext() {
final RaftProperties prop = new RaftProperties();
prop.setClass(MiniRaftCluster.STATEMACHINE_CLASS_KEY,
stateMachineClass, StateMachine.class);
+
+ // make sure leadership check won't affect the test
+ RaftServerConfigKeys.Rpc.setTimeoutMin(prop, TimeDuration.valueOf(2, TimeUnit.SECONDS));
+ RaftServerConfigKeys.Rpc.setTimeoutMax(prop, TimeDuration.valueOf(4, TimeUnit.SECONDS));
Review comment:
blockWriteStateMachineData - it only blocks StateMachine$DataApi#write call. Heartbeats should still work.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] GlenGeng commented on a change in pull request #150: RATIS-981: Step-down stale leader in case of split-brain
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #150:
URL: https://github.com/apache/incubator-ratis/pull/150#discussion_r457018981
##########
File path: ratis-test/src/test/java/org/apache/ratis/grpc/TestRetryCacheWithGrpc.java
##########
@@ -45,6 +47,11 @@
public void testRetryOnResourceUnavailableException()
throws InterruptedException, IOException {
RaftProperties properties = new RaftProperties();
+
+ // make sure leadership check won't affect the test
Review comment:
test cases invoking `SimpleStateMachine4Testing::blockWriteStateMachineData()` or `DelayLocalExecutionInjection::setDelayMs()` will be affected, which is meet expectation. We need enlarge election timeout to make sure leader won't step down before hearing append entries response from majority followers.
Another case is related to the membership change, especially the one with 9-servers. To avoid flaky behavior in CI environment, we need enlarge election timeout a little bit, say from 300ms to 1s, since leader is vulnerable during such situation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org