szetszwo commented on code in PR #954:
URL: https://github.com/apache/ratis/pull/954#discussion_r1378340353
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java:
##########
@@ -387,6 +388,9 @@ private ResultAndTerm waitForResults(Phase phase, long
electionTerm, int submitt
// if some higher priority peer did not response when timeout, but
candidate get majority,
// candidate pass vote
return logAndReturn(phase, Result.PASSED, responses, exceptions);
+ } else if (singleMode) {
+ // if candidate is in single mode, candidate pass vote.
+ return logAndReturn(phase, Result.PASSED, responses, exceptions);
Review Comment:
Let's add a `Result.SINGLE_MODE_PASSED`.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -1311,6 +1311,11 @@ public CompletableFuture<RaftClientReply>
setConfigurationAsync(SetConfiguration
pending.setReply(newSuccessReply(request));
return pending.getFuture();
}
+ if (arguments.getMode() !=
SetConfigurationRequest.Mode.SET_UNCONDITIONALLY
Review Comment:
Since it is not safe, let's don't allow it even for `SET_UNCONDITIONALLY`.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerProxy.java:
##########
@@ -368,7 +369,7 @@ private RaftServerImpl getImpl(RaftRpcRequestProto proto)
throws IOException {
return getImpl(ProtoUtils.toRaftGroupId(proto.getRaftGroupId()));
}
- private RaftServerImpl getImpl(RaftGroupId groupId) throws IOException {
+ public RaftServerImpl getImpl(RaftGroupId groupId) throws IOException {
Review Comment:
The method is not used in this change. Let's keep it private.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java:
##########
@@ -447,6 +452,8 @@ private ResultAndTerm waitForResults(Phase phase, long
electionTerm, int submitt
// received all the responses
if (conf.hasMajority(votedPeers, server.getId())) {
return logAndReturn(phase, Result.PASSED, responses, exceptions);
+ } else if (singleMode) {
+ return logAndReturn(phase, Result.PASSED, responses, exceptions);
Review Comment:
Use `SINGLE_MODE_PASSED`.
##########
ratis-server/src/test/java/org/apache/ratis/server/impl/RaftReconfigurationBaseTest.java:
##########
@@ -145,6 +146,54 @@ public void testAddPeers() throws Exception {
});
}
+ /**
+ * Test leader election when changing cluster from single mode to HA mode.
+ */
+ @Test
+ public void testLeaderElectionWhenChangeFromSingleToHA() throws Exception {
+ runWithNewCluster(1, cluster -> {
+ RaftTestUtil.waitForLeader(cluster);
+
+ RaftGroupId groupId = cluster.getGroup().getGroupId();
+ RaftPeer curPeer = cluster.getGroup().getPeers().iterator().next();
+ RaftPeer newPeer = cluster.addNewPeers(1, true, true).newPeers[0];
+
+ RaftServerProxy leaderServer = cluster.getServer(curPeer.getId());
+
+ // Update leader conf to transitional single mode.
+ RaftConfigurationImpl oldNewConf = RaftConfigurationImpl.newBuilder()
+ .setOldConf(new PeerConfiguration(Arrays.asList(curPeer)))
+ .setConf(new PeerConfiguration(Arrays.asList(curPeer, newPeer)))
+ .setLogEntryIndex(Long.MAX_VALUE / 2)
+ .build();
+ Assert.assertTrue(oldNewConf.isSingleMode(curPeer.getId()));
+ leaderServer.setRaftConf(groupId, oldNewConf);
+ leaderServer.triggerElection(groupId);
+
+ RaftTestUtil.waitForLeader(cluster);
Review Comment:
Check new leader and new conf:
```java
final RaftServer.Division newLeader =
RaftTestUtil.waitForLeader(cluster);
Assert.assertEquals(leaderServer.getId(), newLeader.getId());
Assert.assertEquals(oldNewConf, newLeader.getRaftConf());
```
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerProxy.java:
##########
@@ -391,6 +392,21 @@ public LifeCycle.State getLifeCycleState() {
return lifeCycle.getCurrentState();
}
+ @VisibleForTesting
+ void setRaftConf(RaftGroupId groupId, RaftConfigurationImpl conf) throws
IOException {
+ getImpl(groupId).getState().setRaftConf(conf);
+ }
Review Comment:
Move it to `RaftServerTestUtil`, i.e.
```java
//RaftServerTestUtil
public static void setRaftConf(RaftServer proxy, RaftGroupId groupId,
RaftConfiguration conf) {
((RaftServerImpl)getDivision(proxy,
groupId)).getState().setRaftConf(conf);
}
```
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftConfigurationImpl.java:
##########
@@ -232,6 +232,39 @@ Collection<RaftPeer> getOtherPeers(RaftPeerId selfId) {
return others;
}
+ /**
+ * @return true if the new peers number reaches half of new conf peers
number or the group is
+ * changing from single mode to HA mode.
+ */
+ boolean changeMajority(Collection<RaftPeer> newMembers) {
+ Preconditions.assertNull(oldConf, "Conf must be stable.");
Review Comment:
It expect to pass the name, i.e. `Preconditions.assertNull(oldConf,
"oldConf")`
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftConfigurationImpl.java:
##########
@@ -232,6 +232,39 @@ Collection<RaftPeer> getOtherPeers(RaftPeerId selfId) {
return others;
}
+ /**
+ * @return true if the new peers number reaches half of new conf peers
number or the group is
+ * changing from single mode to HA mode.
+ */
+ boolean changeMajority(Collection<RaftPeer> newMembers) {
+ Preconditions.assertNull(oldConf, "Conf must be stable.");
+ int newPeersCount = 0;
+ for (RaftPeer peer : newMembers) {
+ final RaftPeer inConf = conf.getPeer(peer.getId());
+ if (inConf == null) {
+ newPeersCount++;
+ }
+ }
+
+ if (conf.size() == 1 && newMembers.size() == 2 && newPeersCount == 1) {
+ // Change from single peer to HA mode. This is a special case, skip
majority verification.
+ return false;
+ }
+ // If newPeersCount reaches majority number of new conf size, the cluster
may end with infinity
+ // election. See https://issues.apache.org/jira/browse/RATIS-1912 for more
details.
+ return newPeersCount >= newMembers.size() / 2 + newMembers.size() % 2;
+ }
+
+ /** @return True if the selfId is in single mode. */
+ boolean isSingleMode(RaftPeerId selfId) {
+ if (isStable()) {
+ return conf.size() == 1;
+ } else {
+ return oldConf.size() == 1 && oldConf.contains(selfId) && conf.size() ==
2 && conf.contains(
+ selfId);
Review Comment:
Make it a single line (ratis uses 120 line width).
##########
ratis-server/src/test/java/org/apache/ratis/server/impl/RaftReconfigurationBaseTest.java:
##########
@@ -145,6 +146,54 @@ public void testAddPeers() throws Exception {
});
}
+ /**
+ * Test leader election when changing cluster from single mode to HA mode.
+ */
+ @Test
+ public void testLeaderElectionWhenChangeFromSingleToHA() throws Exception {
+ runWithNewCluster(1, cluster -> {
+ RaftTestUtil.waitForLeader(cluster);
+
+ RaftGroupId groupId = cluster.getGroup().getGroupId();
+ RaftPeer curPeer = cluster.getGroup().getPeers().iterator().next();
+ RaftPeer newPeer = cluster.addNewPeers(1, true, true).newPeers[0];
+
+ RaftServerProxy leaderServer = cluster.getServer(curPeer.getId());
+
+ // Update leader conf to transitional single mode.
+ RaftConfigurationImpl oldNewConf = RaftConfigurationImpl.newBuilder()
+ .setOldConf(new PeerConfiguration(Arrays.asList(curPeer)))
+ .setConf(new PeerConfiguration(Arrays.asList(curPeer, newPeer)))
+ .setLogEntryIndex(Long.MAX_VALUE / 2)
+ .build();
+ Assert.assertTrue(oldNewConf.isSingleMode(curPeer.getId()));
+ leaderServer.setRaftConf(groupId, oldNewConf);
+ leaderServer.triggerElection(groupId);
Review Comment:
Use `transferLeadership` with null new leader and then remove the new
`triggerElection` method.
```java
try(RaftClient client = cluster.createClient()) {
client.admin().transferLeadership(null, leaderServer.getId(),
10_000);
}
```
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftConfigurationImpl.java:
##########
@@ -232,6 +232,39 @@ Collection<RaftPeer> getOtherPeers(RaftPeerId selfId) {
return others;
}
+ /**
+ * @return true if the new peers number reaches half of new conf peers
number or the group is
+ * changing from single mode to HA mode.
+ */
+ boolean changeMajority(Collection<RaftPeer> newMembers) {
+ Preconditions.assertNull(oldConf, "Conf must be stable.");
+ int newPeersCount = 0;
+ for (RaftPeer peer : newMembers) {
+ final RaftPeer inConf = conf.getPeer(peer.getId());
+ if (inConf == null) {
+ newPeersCount++;
+ }
+ }
+
+ if (conf.size() == 1 && newMembers.size() == 2 && newPeersCount == 1) {
+ // Change from single peer to HA mode. This is a special case, skip
majority verification.
+ return false;
+ }
+ // If newPeersCount reaches majority number of new conf size, the cluster
may end with infinity
+ // election. See https://issues.apache.org/jira/browse/RATIS-1912 for more
details.
+ return newPeersCount >= newMembers.size() / 2 + newMembers.size() % 2;
Review Comment:
It may be easier to understand to `return newPeersCount >= oldPeersCount`,
i.e.
```java
final long oldPeersCount = newMembers.size() - newPeersCount;
return newPeersCount >= oldPeersCount;
```
--
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]
