-
Notifications
You must be signed in to change notification settings - Fork 438
RATIS-2378. fix listener role transition #1331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
szetszwo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JoeCqupt , thanks a lot for working on this! Please see the comments inlined and below
+++ b/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
@@ -578,6 +578,14 @@ class RaftServerImpl implements RaftServer.Division,
}
}
+ private boolean shouldSetFollower(RaftPeerRole old, boolean force) {
+ if (old == RaftPeerRole.LISTENER) {
+ final RaftConfigurationImpl conf = state.getRaftConf();
+ return conf.isStable() && conf.containsInConf(getId());
+ }
+ return old != RaftPeerRole.FOLLOWER || force;
+ }
+
/**
* Change the server state to Follower if this server is in a different role or force is true.
* @param newTerm The new term.
@@ -591,7 +599,7 @@ class RaftServerImpl implements RaftServer.Division,
throw new IllegalStateException("Unexpected role " + old);
}
CompletableFuture<Void> future = CompletableFuture.completedFuture(null);
- if ((old != RaftPeerRole.FOLLOWER || force) && old != RaftPeerRole.LISTENER) {
+ if (shouldSetFollower(old, force)) {
setRole(RaftPeerRole.FOLLOWER, reason);
if (old == RaftPeerRole.LEADER) {
future = role.shutdownLeaderState(false)
@@ -607,7 +615,7 @@ class RaftServerImpl implements RaftServer.Division,
state.setLeader(null, reason);
} else if (old == RaftPeerRole.CANDIDATE) {
future = role.shutdownLeaderElection();
- } else if (old == RaftPeerRole.FOLLOWER) {
+ } else if (old == RaftPeerRole.FOLLOWER || old == RaftPeerRole.LISTENER) {
future = role.shutdownFollowerState();
}
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
Outdated
Show resolved
Hide resolved
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
Outdated
Show resolved
Hide resolved
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
Outdated
Show resolved
Hide resolved
|
I have actually considered moving the logic to in when Never mind, it works correctly now. let the logic in |
|
now unit tests fail because the listener do not change to follower immediately after received https://github.com/apache/ratis/actions/runs/20610847852/job/59198634271#step:11:1020 |
|
i add logic to @szetszwo |
szetszwo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JoeCqupt , thanks for the update! Please see the comment inlined and also https://issues.apache.org/jira/secure/attachment/13080065/1331_review.patch
ratis-server/src/main/java/org/apache/ratis/server/impl/ServerState.java
Outdated
Show resolved
Hide resolved
szetszwo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 the change looks good.

What changes were proposed in this pull request?
fix https://issues.apache.org/jira/browse/RATIS-2378
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-2378
How was this patch tested?
unit tests