szetszwo commented on a change in pull request #560:
URL: https://github.com/apache/ratis/pull/560#discussion_r766451724
##########
File path:
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java
##########
@@ -912,17 +912,22 @@ private void yieldLeaderToHigherPriorityPeer() {
}
final RaftConfigurationImpl conf = server.getRaftConf();
+ final RaftPeer leader = conf.getPeer(server.getId());
+ if (leader == null) {
+ LOG.error("{} find leader {} not in the conf {}",
+ this, server.getId(), conf);
Review comment:
Let's change the message to below:
```
LOG.error("{} the leader {} is not in the conf {}", this,
server.getId(), conf);
```
##########
File path:
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java
##########
@@ -912,17 +912,22 @@ private void yieldLeaderToHigherPriorityPeer() {
}
final RaftConfigurationImpl conf = server.getRaftConf();
+ final RaftPeer leader = conf.getPeer(server.getId());
+ if (leader == null) {
+ LOG.error("{} find leader {} not in the conf {}",
+ this, server.getId(), conf);
+ return;
+ }
int leaderPriority = conf.getPeer(server.getId()).getPriority();
Review comment:
Let's reuse the "leader" variable here.
```
final int leaderPriority = leader.getPriority();
```
##########
File path:
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java
##########
@@ -273,6 +275,10 @@ private ResultAndTerm submitRequestAndWaitResult(Phase
phase, RaftConfigurationI
throws InterruptedException {
final ResultAndTerm r;
final Collection<RaftPeer> others = conf.getOtherPeers(server.getId());
+ if (!conf.containsInBothConfs(server.getId())) {
+ r = new ResultAndTerm(Result.REJECTED, electionTerm);
Review comment:
Let's add a new Result.NOT_IN_CONF but not using Result.REJECTED. Also,
the if-statement should be moved up.
```
@@ -104,7 +105,7 @@ class LeaderElection implements Runnable {
ELECTION
}
- enum Result {PASSED, REJECTED, TIMEOUT, DISCOVERED_A_NEW_TERM, SHUTDOWN}
+ enum Result {PASSED, REJECTED, TIMEOUT, DISCOVERED_A_NEW_TERM, SHUTDOWN,
NOT_IN_CONF}
private static class ResultAndTerm {
private final Result result;
@@ -271,6 +272,9 @@ class LeaderElection implements Runnable {
private ResultAndTerm submitRequestAndWaitResult(Phase phase,
RaftConfigurationImpl conf, long electionTerm)
throws InterruptedException {
+ if (!conf.containsInConf(server.getId())) {
+ return new ResultAndTerm(Result.NOT_IN_CONF, electionTerm);
+ }
final ResultAndTerm r;
final Collection<RaftPeer> others = conf.getOtherPeers(server.getId());
if (others.isEmpty()) {
@@ -322,6 +326,7 @@ class LeaderElection implements Runnable {
continue; // should retry
case REJECTED:
case DISCOVERED_A_NEW_TERM:
+ case NOT_IN_CONF:
final long term = r.maxTerm(server.getState().getCurrentTerm());
server.changeToFollowerAndPersistMetadata(term, r);
return false;
```
##########
File path:
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java
##########
@@ -273,6 +275,10 @@ private ResultAndTerm submitRequestAndWaitResult(Phase
phase, RaftConfigurationI
throws InterruptedException {
final ResultAndTerm r;
final Collection<RaftPeer> others = conf.getOtherPeers(server.getId());
+ if (!conf.containsInBothConfs(server.getId())) {
Review comment:
It should use containsInConf(..) instead of containsInBothConfs(..)
since we don't care about the previous conf.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
