From f89e846ce4b235e098cd22b002db032342d7cbb8 Mon Sep 17 00:00:00 2001 From: xushaohong Date: Wed, 8 Dec 2021 11:42:13 +0800 Subject: [PATCH 1/6] RATIS-1446. Avoid leader election for invalid conf --- .../java/org/apache/ratis/server/impl/RaftServerImpl.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java b/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java index eebcd15ece..5a770bd047 100644 --- a/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java +++ b/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java @@ -583,6 +583,12 @@ synchronized void changeToCandidate(boolean forceStartLeaderElection) { if (state.shouldNotifyExtendedNoLeader()) { stateMachine.followerEvent().notifyExtendedNoLeader(getRoleInfoProto()); } + // Candidate shall not start leader election in these cases in case of + // possible NPE caused by conf.getPeer().getPriority() + if (!getRaftConf().containsInBothConfs(getId())) { + LOG.warn("{} find invalid configuration {}, skip start leader election", this, getRaftConf()); + return; + } // start election role.startLeaderElection(this, forceStartLeaderElection); } From 07e0a11be20bc83edd1cdea9fdabe49c4532b549 Mon Sep 17 00:00:00 2001 From: xushaohong Date: Wed, 8 Dec 2021 11:42:13 +0800 Subject: [PATCH 2/6] RATIS-1446. Avoid leader election for invalid conf(v2) --- .../ratis/server/impl/LeaderElection.java | 24 +++++++++---------- .../ratis/server/impl/LeaderStateImpl.java | 13 ++++++---- .../ratis/server/impl/RaftServerImpl.java | 6 ----- .../apache/ratis/server/impl/VoteContext.java | 6 ++++- 4 files changed, 26 insertions(+), 23 deletions(-) diff --git a/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java b/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java index 747607f633..7ce2e8889e 100644 --- a/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java +++ b/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java @@ -43,6 +43,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorCompletionService; @@ -52,6 +53,7 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.HashSet; import java.util.Set; +import java.util.stream.Collectors; import static org.apache.ratis.util.LifeCycle.State.NEW; import static org.apache.ratis.util.LifeCycle.State.RUNNING; @@ -273,6 +275,10 @@ private ResultAndTerm submitRequestAndWaitResult(Phase phase, RaftConfigurationI throws InterruptedException { final ResultAndTerm r; final Collection others = conf.getOtherPeers(server.getId()); + if (!conf.containsInBothConfs(server.getId())) { + r = new ResultAndTerm(Result.REJECTED, electionTerm); + return r; + } if (others.isEmpty()) { r = new ResultAndTerm(Result.PASSED, electionTerm); } else { @@ -345,18 +351,12 @@ private int submitRequests(Phase phase, long electionTerm, TermIndex lastEntry, } private Set getHigherPriorityPeers(RaftConfiguration conf) { - Set higherPriorityPeers = new HashSet<>(); - - int currPriority = conf.getPeer(server.getId()).getPriority(); - final Collection peers = conf.getAllPeers(); - - for (RaftPeer peer : peers) { - if (peer.getPriority() > currPriority) { - higherPriorityPeers.add(peer.getId()); - } - } - - return higherPriorityPeers; + final Optional priority = Optional.ofNullable(conf.getPeer(server.getId())) + .map(RaftPeer::getPriority); + return conf.getAllPeers().stream() + .filter(peer -> priority.filter(p -> peer.getPriority() > p).isPresent()) + .map(RaftPeer::getId) + .collect(Collectors.toSet()); } private ResultAndTerm waitForResults(Phase phase, long electionTerm, int submitted, diff --git a/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java b/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java index 03850114eb..bd1e9673c5 100644 --- a/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java +++ b/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(); for (LogAppender logAppender : senders.getSenders()) { FollowerInfo followerInfo = logAppender.getFollower(); - RaftPeerId followerID = followerInfo.getPeer().getId(); - int followerPriority = conf.getPeer(followerID).getPriority(); - + final RaftPeer follower = followerInfo.getPeer(); + final int followerPriority = follower.getPriority(); if (followerPriority <= leaderPriority) { continue; } - + final RaftPeerId followerID = follower.getId(); final TermIndex leaderLastEntry = server.getState().getLastEntry(); if (leaderLastEntry == null) { LOG.info("{} send StartLeaderElectionRequest to follower:{} on term:{} because follower's priority:{} " + diff --git a/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java b/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java index 5a770bd047..eebcd15ece 100644 --- a/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java +++ b/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java @@ -583,12 +583,6 @@ synchronized void changeToCandidate(boolean forceStartLeaderElection) { if (state.shouldNotifyExtendedNoLeader()) { stateMachine.followerEvent().notifyExtendedNoLeader(getRoleInfoProto()); } - // Candidate shall not start leader election in these cases in case of - // possible NPE caused by conf.getPeer().getPriority() - if (!getRaftConf().containsInBothConfs(getId())) { - LOG.warn("{} find invalid configuration {}, skip start leader election", this, getRaftConf()); - return; - } // start election role.startLeaderElection(this, forceStartLeaderElection); } diff --git a/ratis-server/src/main/java/org/apache/ratis/server/impl/VoteContext.java b/ratis-server/src/main/java/org/apache/ratis/server/impl/VoteContext.java index 1ef721aadc..2b7d96975f 100644 --- a/ratis-server/src/main/java/org/apache/ratis/server/impl/VoteContext.java +++ b/ratis-server/src/main/java/org/apache/ratis/server/impl/VoteContext.java @@ -144,7 +144,11 @@ boolean decideVote(RaftPeer candidate, TermIndex candidateLastEntry) { } // Check priority - final int priority = impl.getRaftConf().getPeer(impl.getId()).getPriority(); + final RaftPeer peer = conf.getPeer(impl.getId()); + if (peer == null) { + return reject("our server " + impl.getId() + " is not in the conf " + conf); + } + final int priority = peer.getPriority(); if (priority <= candidate.getPriority()) { return log(true, "our priority " + priority + " <= candidate's priority " + candidate.getPriority()); } else { From 743271245e1ddde574ed8590a70c678a14c93a4e Mon Sep 17 00:00:00 2001 From: xushaohong Date: Fri, 10 Dec 2021 16:32:27 +0800 Subject: [PATCH 3/6] fix some comments --- .../org/apache/ratis/server/impl/LeaderElection.java | 11 +++++------ .../org/apache/ratis/server/impl/LeaderStateImpl.java | 5 ++--- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java b/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java index 7ce2e8889e..7ef0e22997 100644 --- a/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java +++ b/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java @@ -51,7 +51,6 @@ import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicInteger; -import java.util.HashSet; import java.util.Set; import java.util.stream.Collectors; @@ -106,7 +105,7 @@ enum Phase { 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; @@ -273,12 +272,11 @@ private boolean shouldRun(long electionTerm) { private ResultAndTerm submitRequestAndWaitResult(Phase phase, RaftConfigurationImpl conf, long electionTerm) throws InterruptedException { + if (!conf.containsInConf(server.getId()) && phase == Phase.ELECTION) { + return new ResultAndTerm(Result.NOT_IN_CONF, electionTerm); + } final ResultAndTerm r; final Collection others = conf.getOtherPeers(server.getId()); - if (!conf.containsInBothConfs(server.getId())) { - r = new ResultAndTerm(Result.REJECTED, electionTerm); - return r; - } if (others.isEmpty()) { r = new ResultAndTerm(Result.PASSED, electionTerm); } else { @@ -328,6 +326,7 @@ private boolean askForVotes(Phase phase) throws InterruptedException, IOExceptio 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; diff --git a/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java b/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java index bd1e9673c5..bf6677bbb6 100644 --- a/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java +++ b/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java @@ -914,11 +914,10 @@ 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); + LOG.error("{} the leader {} is not in the conf {}", this, server.getId(), conf); return; } - int leaderPriority = conf.getPeer(server.getId()).getPriority(); + int leaderPriority = leader.getPriority(); for (LogAppender logAppender : senders.getSenders()) { FollowerInfo followerInfo = logAppender.getFollower(); From 846a3c590c22f5a0e6469d0c17904f8bfd2df389 Mon Sep 17 00:00:00 2001 From: xushaohong Date: Mon, 13 Dec 2021 14:39:07 +0800 Subject: [PATCH 4/6] fix yieldLeaderToHigherPriorityPeer --- .../org/apache/ratis/server/impl/LeaderStateImpl.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java b/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java index bf6677bbb6..3181625c97 100644 --- a/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java +++ b/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java @@ -920,13 +920,17 @@ private void yieldLeaderToHigherPriorityPeer() { int leaderPriority = leader.getPriority(); for (LogAppender logAppender : senders.getSenders()) { - FollowerInfo followerInfo = logAppender.getFollower(); - final RaftPeer follower = followerInfo.getPeer(); + final FollowerInfo followerInfo = logAppender.getFollower(); + final RaftPeerId followerID = followerInfo.getPeer().getId(); + final RaftPeer follower = conf.getPeer(followerID); + if (follower == null) { + LOG.error("{} the follower {} is not in the conf {}", this, server.getId(), conf); + continue; + } final int followerPriority = follower.getPriority(); if (followerPriority <= leaderPriority) { continue; } - final RaftPeerId followerID = follower.getId(); final TermIndex leaderLastEntry = server.getState().getLastEntry(); if (leaderLastEntry == null) { LOG.info("{} send StartLeaderElectionRequest to follower:{} on term:{} because follower's priority:{} " + From 409c5571462aaa9e1e557f722b9d7ba3513c3ead Mon Sep 17 00:00:00 2001 From: xushaohong Date: Mon, 13 Dec 2021 16:17:11 +0800 Subject: [PATCH 5/6] remove phase check --- .../main/java/org/apache/ratis/server/impl/LeaderElection.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java b/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java index 7ef0e22997..cf21acc211 100644 --- a/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java +++ b/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java @@ -272,7 +272,7 @@ private boolean shouldRun(long electionTerm) { private ResultAndTerm submitRequestAndWaitResult(Phase phase, RaftConfigurationImpl conf, long electionTerm) throws InterruptedException { - if (!conf.containsInConf(server.getId()) && phase == Phase.ELECTION) { + if (!conf.containsInConf(server.getId())) { return new ResultAndTerm(Result.NOT_IN_CONF, electionTerm); } final ResultAndTerm r; From 5af366d6b54890971824c770ed205208e0422991 Mon Sep 17 00:00:00 2001 From: xushaohong Date: Wed, 15 Dec 2021 19:17:55 +0800 Subject: [PATCH 6/6] NOT_IN_CONF case should close server --- .../main/java/org/apache/ratis/server/impl/LeaderElection.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java b/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java index cf21acc211..0f2515cbc9 100644 --- a/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java +++ b/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java @@ -319,6 +319,7 @@ private boolean askForVotes(Phase phase) throws InterruptedException, IOExceptio switch (r.getResult()) { case PASSED: return true; + case NOT_IN_CONF: case SHUTDOWN: server.getRaftServer().close(); return false; @@ -326,7 +327,6 @@ private boolean askForVotes(Phase phase) throws InterruptedException, IOExceptio 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;