You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by "slfan1989 (via GitHub)" <gi...@apache.org> on 2026/08/29 01:45:32 UTC
[PR] RATIS-2662. Make testKillLeaderDuringReconf deterministic. [ratis]
slfan1989 opened a new pull request, #1571:
URL: https://github.com/apache/ratis/pull/1571
## What changes were proposed in this pull request?
This pull request makes `RaftReconfigurationBaseTest#testKillLeaderDuringReconf` deterministic and removes its stale `@Flaky("RATIS-2251")` annotation.
## What is the link to the Apache JIRA
RATIS-2662. Make testKillLeaderDuringReconf deterministic.
## How was this patch tested?
(Please explain how this patch was tested. Ex: unit tests, manual tests)
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2662. Make testKillLeaderDuringReconf deterministic. [ratis]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #1571:
URL: https://github.com/apache/ratis/pull/1571#discussion_r3888611301
##########
ratis-server/src/test/java/org/apache/ratis/server/impl/RaftReconfigurationBaseTest.java:
##########
@@ -507,86 +513,119 @@ void runTestBootstrapReconf(int numNewPeer, boolean startNewPeer, CLUSTER cluste
* retrying.
*/
@Test
- @Flaky("RATIS-2251")
+ @Timeout(120)
public void testKillLeaderDuringReconf() throws Exception {
// originally 3 peers
runWithNewCluster(3, this::runTestKillLeaderDuringReconf);
}
+ /**
+ * Checks whether the server is bootstrapping the given peer.
+ *
+ * @param server the server division to check
+ * @param peerId the ID of the peer to check
+ * @return true if the peer is being bootstrapped in the leader staging state; otherwise, false
+ */
+ private static boolean isBootstrappingPeer(RaftServer.Division server, RaftPeerId peerId) {
+ return ((RaftServerImpl) server).getRole().getLeaderState()
+ .filter(LeaderStateImpl::inStagingState)
+ .map(state -> state.isBootStrappingPeer(peerId))
+ .orElse(false);
+ }
+
void runTestKillLeaderDuringReconf(CLUSTER cluster) throws Exception {
- final AtomicBoolean clientRunning = new AtomicBoolean(true);
- Thread clientThread = null;
+ final ExecutorService executor = ConcurrentUtils.newSingleThreadExecutor(
+ JavaUtils.getClassSimpleName(getClass()) + "-testKillLeaderDuringReconf-client");
try {
final RaftPeerId leaderId = RaftTestUtil.waitForLeader(cluster).getId();
- PeerChanges c1 = cluster.addNewPeers(1, false);
- PeerChanges c2 = cluster.removePeers(1, false, c1.getAddedPeers());
+ final PeerChanges c1 = cluster.addNewPeers(1, false);
+ final PeerChanges c2 = cluster.removePeers(1, false, c1.getAddedPeers());
+ final RaftPeerId newPeerId = c1.getAddedPeers().get(0).getId();
LOG.info("Start setConf: {}", c2.getPeersInNewConf());
LOG.info(cluster.printServers());
- final CompletableFuture<Void> setConf = new CompletableFuture<>();
- clientThread = new Thread(() -> {
- try(final RaftClient client = cluster.createClient(leaderId)) {
- for(int i = 0; clientRunning.get() && !setConf.isDone(); i++) {
- final RaftClientReply reply = client.admin().setConfiguration(c2.getPeersInNewConf());
- if (reply.isSuccess()) {
- setConf.complete(null);
- return;
- }
- LOG.info("setConf attempt #{} failed, {}", i, cluster.printServers());
- }
- } catch(Exception e) {
- LOG.error("Failed to setConf", e);
- setConf.completeExceptionally(e);
+ final Future<RaftClientReply> setConfTask = executor.submit(() -> {
+ try (RaftClient client = cluster.createClient(leaderId)) {
+ return client.admin().setConfiguration(c2.getPeersInNewConf());
}
});
- clientThread.start();
-
- // the leader cannot generate the (old, new) conf, and it will keep
- // bootstrapping the 1 new peer since it has not started yet.
- Assertions.assertFalse(((RaftConfigurationImpl)cluster.getLeader().getRaftConf()).isTransitional());
- // (0) the first conf entry, (1) the 1st setConf entry, (2) a metadata entry
- // (3) new current conf entry (4) a metadata entry
- {
- final RaftLog leaderLog = cluster.getLeader().getRaftLog();
- for(LogEntryProto e : RaftTestUtil.getLogEntryProtos(leaderLog)) {
- LOG.info("{}", LogProtoUtils.toLogEntryString(e));
+ try {
+ JavaUtils.attempt(() -> {
+ assertFalse(setConfTask.isDone(),
+ () -> "setConfiguration completed before the leader was killed; " + cluster.printServers());
+ assertTrue(isBootstrappingPeer(cluster.getDivision(leaderId), newPeerId),
+ () -> "Leader " + leaderId + " is not bootstrapping peer " + newPeerId
+ + "; " + cluster.printServers());
+ }, 10, cluster.getTimeoutMax(), "wait for the original leader to bootstrap " + newPeerId, LOG);
+
+ // The leader cannot generate the (old, new) conf, and it will keep
+ // bootstrapping the new peer since it has not started yet.
+ assertFalse(((RaftConfigurationImpl)cluster.getLeader().getRaftConf()).isTransitional());
+
+ // (0) the first conf entry, (1) the 1st setConf entry, (2) a metadata entry
+ // (3) new current conf entry (4) a metadata entry
+ {
+ final RaftLog leaderLog = cluster.getLeader().getRaftLog();
+ for(LogEntryProto e : RaftTestUtil.getLogEntryProtos(leaderLog)) {
+ LOG.info("{}", LogProtoUtils.toLogEntryString(e));
+ }
+ final long commitIndex = leaderLog.getLastCommittedIndex();
+ assertTrue(commitIndex <= 2,
+ () -> "commitIndex = " + commitIndex + " > 2; " + cluster.printServers());
}
- final long commitIndex = leaderLog.getLastCommittedIndex();
- Assertions.assertTrue(commitIndex <= 2, "commitIndex = " + commitIndex + " > 2");
- }
-
- final RaftPeerId killed = RaftTestUtil.waitAndKillLeader(cluster);
- Assertions.assertEquals(leaderId, killed);
- final RaftPeerId newLeaderId = RaftTestUtil.waitForLeader(cluster).getId();
- LOG.info("newLeaderId: {}", newLeaderId);
- TimeDuration.valueOf(1500, TimeUnit.MILLISECONDS).sleep();
Review Comment:
A fixed `1500 ms` sleep does not guarantee that the new leader has taken over the reconfiguration and started bootstrapping the new peer.
Replace it with a state-based wait that confirms the new leader recognizes `newPeerId` as a bootstrapping peer before the peer is started.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2662. Make testKillLeaderDuringReconf deterministic. [ratis]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #1571:
URL: https://github.com/apache/ratis/pull/1571#discussion_r3888604963
##########
ratis-server/src/test/java/org/apache/ratis/server/impl/RaftReconfigurationBaseTest.java:
##########
@@ -507,86 +513,119 @@ void runTestBootstrapReconf(int numNewPeer, boolean startNewPeer, CLUSTER cluste
* retrying.
*/
@Test
- @Flaky("RATIS-2251")
+ @Timeout(120)
public void testKillLeaderDuringReconf() throws Exception {
// originally 3 peers
runWithNewCluster(3, this::runTestKillLeaderDuringReconf);
}
+ /**
+ * Checks whether the server is bootstrapping the given peer.
+ *
+ * @param server the server division to check
+ * @param peerId the ID of the peer to check
+ * @return true if the peer is being bootstrapped in the leader staging state; otherwise, false
+ */
+ private static boolean isBootstrappingPeer(RaftServer.Division server, RaftPeerId peerId) {
+ return ((RaftServerImpl) server).getRole().getLeaderState()
+ .filter(LeaderStateImpl::inStagingState)
+ .map(state -> state.isBootStrappingPeer(peerId))
+ .orElse(false);
+ }
+
void runTestKillLeaderDuringReconf(CLUSTER cluster) throws Exception {
- final AtomicBoolean clientRunning = new AtomicBoolean(true);
- Thread clientThread = null;
+ final ExecutorService executor = ConcurrentUtils.newSingleThreadExecutor(
+ JavaUtils.getClassSimpleName(getClass()) + "-testKillLeaderDuringReconf-client");
try {
final RaftPeerId leaderId = RaftTestUtil.waitForLeader(cluster).getId();
- PeerChanges c1 = cluster.addNewPeers(1, false);
- PeerChanges c2 = cluster.removePeers(1, false, c1.getAddedPeers());
+ final PeerChanges c1 = cluster.addNewPeers(1, false);
+ final PeerChanges c2 = cluster.removePeers(1, false, c1.getAddedPeers());
+ final RaftPeerId newPeerId = c1.getAddedPeers().get(0).getId();
LOG.info("Start setConf: {}", c2.getPeersInNewConf());
LOG.info(cluster.printServers());
- final CompletableFuture<Void> setConf = new CompletableFuture<>();
- clientThread = new Thread(() -> {
- try(final RaftClient client = cluster.createClient(leaderId)) {
- for(int i = 0; clientRunning.get() && !setConf.isDone(); i++) {
- final RaftClientReply reply = client.admin().setConfiguration(c2.getPeersInNewConf());
- if (reply.isSuccess()) {
- setConf.complete(null);
- return;
- }
- LOG.info("setConf attempt #{} failed, {}", i, cluster.printServers());
- }
- } catch(Exception e) {
- LOG.error("Failed to setConf", e);
- setConf.completeExceptionally(e);
+ final Future<RaftClientReply> setConfTask = executor.submit(() -> {
+ try (RaftClient client = cluster.createClient(leaderId)) {
+ return client.admin().setConfiguration(c2.getPeersInNewConf());
}
});
- clientThread.start();
-
- // the leader cannot generate the (old, new) conf, and it will keep
- // bootstrapping the 1 new peer since it has not started yet.
- Assertions.assertFalse(((RaftConfigurationImpl)cluster.getLeader().getRaftConf()).isTransitional());
- // (0) the first conf entry, (1) the 1st setConf entry, (2) a metadata entry
- // (3) new current conf entry (4) a metadata entry
- {
- final RaftLog leaderLog = cluster.getLeader().getRaftLog();
- for(LogEntryProto e : RaftTestUtil.getLogEntryProtos(leaderLog)) {
- LOG.info("{}", LogProtoUtils.toLogEntryString(e));
+ try {
+ JavaUtils.attempt(() -> {
+ assertFalse(setConfTask.isDone(),
+ () -> "setConfiguration completed before the leader was killed; " + cluster.printServers());
+ assertTrue(isBootstrappingPeer(cluster.getDivision(leaderId), newPeerId),
+ () -> "Leader " + leaderId + " is not bootstrapping peer " + newPeerId
+ + "; " + cluster.printServers());
+ }, 10, cluster.getTimeoutMax(), "wait for the original leader to bootstrap " + newPeerId, LOG);
+
+ // The leader cannot generate the (old, new) conf, and it will keep
+ // bootstrapping the new peer since it has not started yet.
+ assertFalse(((RaftConfigurationImpl)cluster.getLeader().getRaftConf()).isTransitional());
+
+ // (0) the first conf entry, (1) the 1st setConf entry, (2) a metadata entry
+ // (3) new current conf entry (4) a metadata entry
+ {
+ final RaftLog leaderLog = cluster.getLeader().getRaftLog();
+ for(LogEntryProto e : RaftTestUtil.getLogEntryProtos(leaderLog)) {
+ LOG.info("{}", LogProtoUtils.toLogEntryString(e));
+ }
+ final long commitIndex = leaderLog.getLastCommittedIndex();
+ assertTrue(commitIndex <= 2,
+ () -> "commitIndex = " + commitIndex + " > 2; " + cluster.printServers());
}
- final long commitIndex = leaderLog.getLastCommittedIndex();
- Assertions.assertTrue(commitIndex <= 2, "commitIndex = " + commitIndex + " > 2");
- }
-
- final RaftPeerId killed = RaftTestUtil.waitAndKillLeader(cluster);
- Assertions.assertEquals(leaderId, killed);
- final RaftPeerId newLeaderId = RaftTestUtil.waitForLeader(cluster).getId();
- LOG.info("newLeaderId: {}", newLeaderId);
- TimeDuration.valueOf(1500, TimeUnit.MILLISECONDS).sleep();
- LOG.info("start new peers: {}", c1.getAddedPeers());
- for (RaftPeer np : c1.getAddedPeers()) {
- cluster.restartServer(np.getId(), false);
Review Comment:
Since the test adds exactly one peer, the loop is unnecessary. Starting the existing peer directly preserves its empty initial configuration and prevents it from prematurely participating in `PRE_VOTE` before being bootstrapped, eliminating the `PRE_VOTE` -> `SHUTDOWN` -> `CLOSED` race.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2662. Make testKillLeaderDuringReconf deterministic. [ratis]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #1571:
URL: https://github.com/apache/ratis/pull/1571#discussion_r3888635992
##########
ratis-server/src/test/java/org/apache/ratis/server/impl/RaftReconfigurationBaseTest.java:
##########
@@ -507,86 +513,119 @@ void runTestBootstrapReconf(int numNewPeer, boolean startNewPeer, CLUSTER cluste
* retrying.
*/
@Test
- @Flaky("RATIS-2251")
+ @Timeout(120)
public void testKillLeaderDuringReconf() throws Exception {
// originally 3 peers
runWithNewCluster(3, this::runTestKillLeaderDuringReconf);
}
+ /**
+ * Checks whether the server is bootstrapping the given peer.
+ *
+ * @param server the server division to check
+ * @param peerId the ID of the peer to check
+ * @return true if the peer is being bootstrapped in the leader staging state; otherwise, false
+ */
+ private static boolean isBootstrappingPeer(RaftServer.Division server, RaftPeerId peerId) {
+ return ((RaftServerImpl) server).getRole().getLeaderState()
+ .filter(LeaderStateImpl::inStagingState)
+ .map(state -> state.isBootStrappingPeer(peerId))
+ .orElse(false);
+ }
+
void runTestKillLeaderDuringReconf(CLUSTER cluster) throws Exception {
- final AtomicBoolean clientRunning = new AtomicBoolean(true);
- Thread clientThread = null;
+ final ExecutorService executor = ConcurrentUtils.newSingleThreadExecutor(
Review Comment:
Run `setConfiguration` asynchronously in a named single-thread executor so that the test can kill the leader while reconfiguration is in progress. The returned `Future` allows the test to validate the reply, propagate failures, and clean up the background task reliably.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2662. Make testKillLeaderDuringReconf deterministic. [ratis]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #1571:
URL: https://github.com/apache/ratis/pull/1571#discussion_r3888614640
##########
ratis-server/src/test/java/org/apache/ratis/server/impl/RaftReconfigurationBaseTest.java:
##########
@@ -507,86 +513,119 @@ void runTestBootstrapReconf(int numNewPeer, boolean startNewPeer, CLUSTER cluste
* retrying.
*/
@Test
- @Flaky("RATIS-2251")
+ @Timeout(120)
public void testKillLeaderDuringReconf() throws Exception {
// originally 3 peers
runWithNewCluster(3, this::runTestKillLeaderDuringReconf);
}
+ /**
+ * Checks whether the server is bootstrapping the given peer.
+ *
+ * @param server the server division to check
+ * @param peerId the ID of the peer to check
+ * @return true if the peer is being bootstrapped in the leader staging state; otherwise, false
+ */
+ private static boolean isBootstrappingPeer(RaftServer.Division server, RaftPeerId peerId) {
+ return ((RaftServerImpl) server).getRole().getLeaderState()
+ .filter(LeaderStateImpl::inStagingState)
+ .map(state -> state.isBootStrappingPeer(peerId))
+ .orElse(false);
+ }
+
void runTestKillLeaderDuringReconf(CLUSTER cluster) throws Exception {
- final AtomicBoolean clientRunning = new AtomicBoolean(true);
- Thread clientThread = null;
+ final ExecutorService executor = ConcurrentUtils.newSingleThreadExecutor(
+ JavaUtils.getClassSimpleName(getClass()) + "-testKillLeaderDuringReconf-client");
try {
final RaftPeerId leaderId = RaftTestUtil.waitForLeader(cluster).getId();
- PeerChanges c1 = cluster.addNewPeers(1, false);
- PeerChanges c2 = cluster.removePeers(1, false, c1.getAddedPeers());
+ final PeerChanges c1 = cluster.addNewPeers(1, false);
+ final PeerChanges c2 = cluster.removePeers(1, false, c1.getAddedPeers());
+ final RaftPeerId newPeerId = c1.getAddedPeers().get(0).getId();
LOG.info("Start setConf: {}", c2.getPeersInNewConf());
LOG.info(cluster.printServers());
- final CompletableFuture<Void> setConf = new CompletableFuture<>();
- clientThread = new Thread(() -> {
- try(final RaftClient client = cluster.createClient(leaderId)) {
- for(int i = 0; clientRunning.get() && !setConf.isDone(); i++) {
- final RaftClientReply reply = client.admin().setConfiguration(c2.getPeersInNewConf());
- if (reply.isSuccess()) {
- setConf.complete(null);
- return;
- }
- LOG.info("setConf attempt #{} failed, {}", i, cluster.printServers());
- }
- } catch(Exception e) {
- LOG.error("Failed to setConf", e);
- setConf.completeExceptionally(e);
+ final Future<RaftClientReply> setConfTask = executor.submit(() -> {
+ try (RaftClient client = cluster.createClient(leaderId)) {
+ return client.admin().setConfiguration(c2.getPeersInNewConf());
}
});
- clientThread.start();
-
- // the leader cannot generate the (old, new) conf, and it will keep
- // bootstrapping the 1 new peer since it has not started yet.
- Assertions.assertFalse(((RaftConfigurationImpl)cluster.getLeader().getRaftConf()).isTransitional());
- // (0) the first conf entry, (1) the 1st setConf entry, (2) a metadata entry
- // (3) new current conf entry (4) a metadata entry
- {
- final RaftLog leaderLog = cluster.getLeader().getRaftLog();
- for(LogEntryProto e : RaftTestUtil.getLogEntryProtos(leaderLog)) {
- LOG.info("{}", LogProtoUtils.toLogEntryString(e));
+ try {
+ JavaUtils.attempt(() -> {
+ assertFalse(setConfTask.isDone(),
+ () -> "setConfiguration completed before the leader was killed; " + cluster.printServers());
+ assertTrue(isBootstrappingPeer(cluster.getDivision(leaderId), newPeerId),
+ () -> "Leader " + leaderId + " is not bootstrapping peer " + newPeerId
+ + "; " + cluster.printServers());
+ }, 10, cluster.getTimeoutMax(), "wait for the original leader to bootstrap " + newPeerId, LOG);
+
+ // The leader cannot generate the (old, new) conf, and it will keep
+ // bootstrapping the new peer since it has not started yet.
+ assertFalse(((RaftConfigurationImpl)cluster.getLeader().getRaftConf()).isTransitional());
+
+ // (0) the first conf entry, (1) the 1st setConf entry, (2) a metadata entry
+ // (3) new current conf entry (4) a metadata entry
+ {
+ final RaftLog leaderLog = cluster.getLeader().getRaftLog();
+ for(LogEntryProto e : RaftTestUtil.getLogEntryProtos(leaderLog)) {
+ LOG.info("{}", LogProtoUtils.toLogEntryString(e));
+ }
+ final long commitIndex = leaderLog.getLastCommittedIndex();
+ assertTrue(commitIndex <= 2,
+ () -> "commitIndex = " + commitIndex + " > 2; " + cluster.printServers());
}
- final long commitIndex = leaderLog.getLastCommittedIndex();
- Assertions.assertTrue(commitIndex <= 2, "commitIndex = " + commitIndex + " > 2");
- }
-
- final RaftPeerId killed = RaftTestUtil.waitAndKillLeader(cluster);
- Assertions.assertEquals(leaderId, killed);
- final RaftPeerId newLeaderId = RaftTestUtil.waitForLeader(cluster).getId();
- LOG.info("newLeaderId: {}", newLeaderId);
- TimeDuration.valueOf(1500, TimeUnit.MILLISECONDS).sleep();
- LOG.info("start new peers: {}", c1.getAddedPeers());
- for (RaftPeer np : c1.getAddedPeers()) {
- cluster.restartServer(np.getId(), false);
- }
+ final RaftPeerId killed = RaftTestUtil.waitAndKillLeader(cluster);
+ assertEquals(leaderId, killed);
+ final RaftServer.Division newLeader = RaftTestUtil.waitForLeader(cluster);
+ final RaftPeerId newLeaderId = newLeader.getId();
+ assertNotEquals(leaderId, newLeaderId);
+ LOG.info("newLeaderId: {}", newLeaderId);
+
+ JavaUtils.attempt(() -> {
+ assertFalse(setConfTask.isDone(),
+ () -> "setConfiguration completed before the new peer was started; " + cluster.printServers());
+ assertTrue(isBootstrappingPeer(newLeader, newPeerId),
+ () -> "New leader " + newLeaderId + " is not bootstrapping peer " + newPeerId
+ + "; " + cluster.printServers());
+ }, 30, cluster.getTimeoutMax(), "wait for the new leader to bootstrap " + newPeerId, LOG);
+
+ LOG.info("start new peer: {}", newPeerId);
+ cluster.getServer(newPeerId).start();
+ final RaftServer.Division newDivision = cluster.getDivision(newPeerId);
+
+ final RaftClientReply reply;
+ try {
+ reply = setConfTask.get(30, TimeUnit.SECONDS);
+ } catch (TimeoutException e) {
+ throw new AssertionError("Timed out waiting for reconfiguration; newPeer=" + newPeerId
+ + ", newPeerState=" + newDivision.getInfo().getLifeCycleState() + ", setConfTask[done="
+ + setConfTask.isDone() + ", cancelled=" + setConfTask.isCancelled() + "], "
+ + cluster.printServers(), e);
+ } catch (ExecutionException e) {
+ throw new AssertionError("setConfiguration failed; newPeer=" + newPeerId
+ + ", newPeerState=" + newDivision.getInfo().getLifeCycleState() + ", "
+ + cluster.printServers(), e.getCause());
+ }
- try {
- setConf.get(10, TimeUnit.SECONDS);
Review Comment:
A timeout means that the expected reconfiguration has not completed. Ignoring it allows the test to continue with an unknown state and may produce a false success, especially when the final checks are skipped for a closed peer. Treat the timeout as a test failure and include the
task, peer, and cluster states in the diagnostic message.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2662. Make testKillLeaderDuringReconf deterministic. [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #1571:
URL: https://github.com/apache/ratis/pull/1571
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2662. Make testKillLeaderDuringReconf deterministic. [ratis]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #1571:
URL: https://github.com/apache/ratis/pull/1571#issuecomment-5460703431
I’m working on a detailed explanation of the unit test workflow and will update the PR shortly.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2662. Make testKillLeaderDuringReconf deterministic. [ratis]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #1571:
URL: https://github.com/apache/ratis/pull/1571#discussion_r3888638226
##########
ratis-server/src/test/java/org/apache/ratis/server/impl/RaftReconfigurationBaseTest.java:
##########
@@ -507,86 +513,119 @@ void runTestBootstrapReconf(int numNewPeer, boolean startNewPeer, CLUSTER cluste
* retrying.
*/
@Test
- @Flaky("RATIS-2251")
+ @Timeout(120)
public void testKillLeaderDuringReconf() throws Exception {
// originally 3 peers
runWithNewCluster(3, this::runTestKillLeaderDuringReconf);
}
+ /**
+ * Checks whether the server is bootstrapping the given peer.
+ *
+ * @param server the server division to check
+ * @param peerId the ID of the peer to check
+ * @return true if the peer is being bootstrapped in the leader staging state; otherwise, false
+ */
+ private static boolean isBootstrappingPeer(RaftServer.Division server, RaftPeerId peerId) {
+ return ((RaftServerImpl) server).getRole().getLeaderState()
+ .filter(LeaderStateImpl::inStagingState)
+ .map(state -> state.isBootStrappingPeer(peerId))
+ .orElse(false);
+ }
+
void runTestKillLeaderDuringReconf(CLUSTER cluster) throws Exception {
- final AtomicBoolean clientRunning = new AtomicBoolean(true);
- Thread clientThread = null;
+ final ExecutorService executor = ConcurrentUtils.newSingleThreadExecutor(
Review Comment:
Replace the manually managed client thread, control flag, and `CompletableFuture` with an `ExecutorService` and `Future`.
This simplifies the asynchronous workflow and provides explicit result handling, exception propagation, cancellation, timeout control, and deterministic cleanup.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2662. Make testKillLeaderDuringReconf deterministic. [ratis]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #1571:
URL: https://github.com/apache/ratis/pull/1571#issuecomment-5483877988
> +1 the change looks good.
@szetszwo Thanks for the review and the +1! I really appreciate it.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2662. Make testKillLeaderDuringReconf deterministic. [ratis]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #1571:
URL: https://github.com/apache/ratis/pull/1571#issuecomment-5467150330
@szetszwo @adoroszlai Could you please take a look at this PR when you have time?
I believe we have identified the root cause of the flakiness in `testKillLeaderDuringReconf`.
I have documented the failure sequence, supporting logs, root-cause analysis, and the rationale for the changes in the PR description.
Any feedback would be greatly appreciated. Thanks!
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2662. Make testKillLeaderDuringReconf deterministic. [ratis]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #1571:
URL: https://github.com/apache/ratis/pull/1571#discussion_r3888614640
##########
ratis-server/src/test/java/org/apache/ratis/server/impl/RaftReconfigurationBaseTest.java:
##########
@@ -507,86 +513,119 @@ void runTestBootstrapReconf(int numNewPeer, boolean startNewPeer, CLUSTER cluste
* retrying.
*/
@Test
- @Flaky("RATIS-2251")
+ @Timeout(120)
public void testKillLeaderDuringReconf() throws Exception {
// originally 3 peers
runWithNewCluster(3, this::runTestKillLeaderDuringReconf);
}
+ /**
+ * Checks whether the server is bootstrapping the given peer.
+ *
+ * @param server the server division to check
+ * @param peerId the ID of the peer to check
+ * @return true if the peer is being bootstrapped in the leader staging state; otherwise, false
+ */
+ private static boolean isBootstrappingPeer(RaftServer.Division server, RaftPeerId peerId) {
+ return ((RaftServerImpl) server).getRole().getLeaderState()
+ .filter(LeaderStateImpl::inStagingState)
+ .map(state -> state.isBootStrappingPeer(peerId))
+ .orElse(false);
+ }
+
void runTestKillLeaderDuringReconf(CLUSTER cluster) throws Exception {
- final AtomicBoolean clientRunning = new AtomicBoolean(true);
- Thread clientThread = null;
+ final ExecutorService executor = ConcurrentUtils.newSingleThreadExecutor(
+ JavaUtils.getClassSimpleName(getClass()) + "-testKillLeaderDuringReconf-client");
try {
final RaftPeerId leaderId = RaftTestUtil.waitForLeader(cluster).getId();
- PeerChanges c1 = cluster.addNewPeers(1, false);
- PeerChanges c2 = cluster.removePeers(1, false, c1.getAddedPeers());
+ final PeerChanges c1 = cluster.addNewPeers(1, false);
+ final PeerChanges c2 = cluster.removePeers(1, false, c1.getAddedPeers());
+ final RaftPeerId newPeerId = c1.getAddedPeers().get(0).getId();
LOG.info("Start setConf: {}", c2.getPeersInNewConf());
LOG.info(cluster.printServers());
- final CompletableFuture<Void> setConf = new CompletableFuture<>();
- clientThread = new Thread(() -> {
- try(final RaftClient client = cluster.createClient(leaderId)) {
- for(int i = 0; clientRunning.get() && !setConf.isDone(); i++) {
- final RaftClientReply reply = client.admin().setConfiguration(c2.getPeersInNewConf());
- if (reply.isSuccess()) {
- setConf.complete(null);
- return;
- }
- LOG.info("setConf attempt #{} failed, {}", i, cluster.printServers());
- }
- } catch(Exception e) {
- LOG.error("Failed to setConf", e);
- setConf.completeExceptionally(e);
+ final Future<RaftClientReply> setConfTask = executor.submit(() -> {
+ try (RaftClient client = cluster.createClient(leaderId)) {
+ return client.admin().setConfiguration(c2.getPeersInNewConf());
}
});
- clientThread.start();
-
- // the leader cannot generate the (old, new) conf, and it will keep
- // bootstrapping the 1 new peer since it has not started yet.
- Assertions.assertFalse(((RaftConfigurationImpl)cluster.getLeader().getRaftConf()).isTransitional());
- // (0) the first conf entry, (1) the 1st setConf entry, (2) a metadata entry
- // (3) new current conf entry (4) a metadata entry
- {
- final RaftLog leaderLog = cluster.getLeader().getRaftLog();
- for(LogEntryProto e : RaftTestUtil.getLogEntryProtos(leaderLog)) {
- LOG.info("{}", LogProtoUtils.toLogEntryString(e));
+ try {
+ JavaUtils.attempt(() -> {
+ assertFalse(setConfTask.isDone(),
+ () -> "setConfiguration completed before the leader was killed; " + cluster.printServers());
+ assertTrue(isBootstrappingPeer(cluster.getDivision(leaderId), newPeerId),
+ () -> "Leader " + leaderId + " is not bootstrapping peer " + newPeerId
+ + "; " + cluster.printServers());
+ }, 10, cluster.getTimeoutMax(), "wait for the original leader to bootstrap " + newPeerId, LOG);
+
+ // The leader cannot generate the (old, new) conf, and it will keep
+ // bootstrapping the new peer since it has not started yet.
+ assertFalse(((RaftConfigurationImpl)cluster.getLeader().getRaftConf()).isTransitional());
+
+ // (0) the first conf entry, (1) the 1st setConf entry, (2) a metadata entry
+ // (3) new current conf entry (4) a metadata entry
+ {
+ final RaftLog leaderLog = cluster.getLeader().getRaftLog();
+ for(LogEntryProto e : RaftTestUtil.getLogEntryProtos(leaderLog)) {
+ LOG.info("{}", LogProtoUtils.toLogEntryString(e));
+ }
+ final long commitIndex = leaderLog.getLastCommittedIndex();
+ assertTrue(commitIndex <= 2,
+ () -> "commitIndex = " + commitIndex + " > 2; " + cluster.printServers());
}
- final long commitIndex = leaderLog.getLastCommittedIndex();
- Assertions.assertTrue(commitIndex <= 2, "commitIndex = " + commitIndex + " > 2");
- }
-
- final RaftPeerId killed = RaftTestUtil.waitAndKillLeader(cluster);
- Assertions.assertEquals(leaderId, killed);
- final RaftPeerId newLeaderId = RaftTestUtil.waitForLeader(cluster).getId();
- LOG.info("newLeaderId: {}", newLeaderId);
- TimeDuration.valueOf(1500, TimeUnit.MILLISECONDS).sleep();
- LOG.info("start new peers: {}", c1.getAddedPeers());
- for (RaftPeer np : c1.getAddedPeers()) {
- cluster.restartServer(np.getId(), false);
- }
+ final RaftPeerId killed = RaftTestUtil.waitAndKillLeader(cluster);
+ assertEquals(leaderId, killed);
+ final RaftServer.Division newLeader = RaftTestUtil.waitForLeader(cluster);
+ final RaftPeerId newLeaderId = newLeader.getId();
+ assertNotEquals(leaderId, newLeaderId);
+ LOG.info("newLeaderId: {}", newLeaderId);
+
+ JavaUtils.attempt(() -> {
+ assertFalse(setConfTask.isDone(),
+ () -> "setConfiguration completed before the new peer was started; " + cluster.printServers());
+ assertTrue(isBootstrappingPeer(newLeader, newPeerId),
+ () -> "New leader " + newLeaderId + " is not bootstrapping peer " + newPeerId
+ + "; " + cluster.printServers());
+ }, 30, cluster.getTimeoutMax(), "wait for the new leader to bootstrap " + newPeerId, LOG);
+
+ LOG.info("start new peer: {}", newPeerId);
+ cluster.getServer(newPeerId).start();
+ final RaftServer.Division newDivision = cluster.getDivision(newPeerId);
+
+ final RaftClientReply reply;
+ try {
+ reply = setConfTask.get(30, TimeUnit.SECONDS);
+ } catch (TimeoutException e) {
+ throw new AssertionError("Timed out waiting for reconfiguration; newPeer=" + newPeerId
+ + ", newPeerState=" + newDivision.getInfo().getLifeCycleState() + ", setConfTask[done="
+ + setConfTask.isDone() + ", cancelled=" + setConfTask.isCancelled() + "], "
+ + cluster.printServers(), e);
+ } catch (ExecutionException e) {
+ throw new AssertionError("setConfiguration failed; newPeer=" + newPeerId
+ + ", newPeerState=" + newDivision.getInfo().getLifeCycleState() + ", "
+ + cluster.printServers(), e.getCause());
+ }
- try {
- setConf.get(10, TimeUnit.SECONDS);
Review Comment:
A timeout means that the expected reconfiguration has not completed. Ignoring it allows the test to continue with an unknown state and may produce a false success, especially when the final checks are skipped for a closed peer. Treat the timeout as a test failure and include the
`task`, `peer`, and `cluster` states in the diagnostic message.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org