You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by "CRZbulabula (via GitHub)" <gi...@apache.org> on 2026/05/29 05:37:05 UTC
[PR] RATIS-2548. Stabilize timing-sensitive Ratis tests [ratis]
CRZbulabula opened a new pull request, #1475:
URL: https://github.com/apache/ratis/pull/1475
## What changes were proposed in this pull request?
This PR stabilizes several timing-sensitive tests by replacing fixed sleeps or immediate assertions with waits for the concrete condition each test needs.
The changes include:
* Wait for restart futures before continuing to log assertions in `RaftBasicTests`.
* Wait for commit index / state machine count in linearizable read tests instead of relying on fixed sleeps.
* Wait for the transaction context map to become empty in `RaftLogTruncateTests`.
* Pause the current leader before election stepDown and wait until a different leader is elected.
* Allow more time for the install-snapshot follower next-index assertion on slow CI.
## Why are the changes needed?
These tests can fail on slower CI machines when asynchronous restart, log cleanup, state machine application, leadership transition, or install snapshot progress completes later than the fixed delay assumed by the test.
## How was this patch tested?
`mvn -pl ratis-server,ratis-test -am -Dtest=TestLinearizableReadRepliedIndexWithGrpc,ElectionCommandIntegrationTest,RaftLogTruncateTests test`
`mvn -pl ratis-test -am -Dtest=TestRaftLogTruncateWithGrpc,TestElectionCommandIntegrationWithGrpc,TestInstallSnapshotNotificationWithGrpc,TestRaftWithGrpc test`
--
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-2548. Stabilize timing-sensitive Ratis tests [ratis]
Posted by "CRZbulabula (via GitHub)" <gi...@apache.org>.
CRZbulabula commented on PR #1475:
URL: https://github.com/apache/ratis/pull/1475#issuecomment-4585721927
Thanks for the review. I updated the PR to address the inline comments:
* Removed the `LeaderStateImpl` change from this PR; this PR is test-only now.
* Restored the kill-leader timing so leader restart is scheduled before client messages are sent.
* Kept the async reply list cleanup and removed the redundant `allOf` join.
* Joined restart futures before `cluster.printAllLogs()` without the try/finally restructuring.
* Removed the redundant `toString()` calls in the election command test.
* For kill-leader append tests, the final assertion now verifies the expected messages appear in order while tolerating an extra retry/failover state-machine entry.
Local verification:
`mvn -pl ratis-test -am -Dtest=TestLinearizableReadRepliedIndexWithGrpc,TestRaftAsyncWithGrpc#testBasicAppendEntriesAsyncKillLeader,TestElectionCommandIntegrationWithGrpc,TestRaftLogTruncateWithGrpc,TestRaftWithGrpc test`
`mvn -pl ratis-test -am -Dtest=TestRaftReconfigurationWithSimulatedRpc#testKillLeaderDuringReconf test`
--
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-2548. Stabilize timing-sensitive Ratis tests [ratis]
Posted by "CRZbulabula (via GitHub)" <gi...@apache.org>.
CRZbulabula commented on code in PR #1475:
URL: https://github.com/apache/ratis/pull/1475#discussion_r3329719212
##########
ratis-test/src/test/java/org/apache/ratis/shell/cli/sh/ElectionCommandIntegrationTest.java:
##########
@@ -150,13 +150,16 @@ public void testElectionStepDownCommand() throws Exception {
void runTestElectionStepDownCommand(MiniRaftCluster cluster) throws Exception {
final RaftServer.Division leader = RaftTestUtil.waitForLeader(cluster);
String sb = getClusterAddress(cluster);
- RaftServer.Division newLeader = cluster.getFollowers().get(0);
final StringPrintStream out = new StringPrintStream();
RatisShell shell = new RatisShell(out.getPrintStream());
- Assertions.assertNotEquals(cluster.getLeader().getId(), newLeader.getId());
Assertions.assertEquals(2, cluster.getFollowers().size());
- int ret = shell.run("election", "stepDown", "-peers", sb.toString());
+ int ret = shell.run("election", "pause", "-peers", sb.toString(), "-address",
+ leader.getPeer().getAddress());
+ Assertions.assertEquals(0, ret);
+
+ ret = shell.run("election", "stepDown", "-peers", sb.toString());
Review Comment:
Done. Removed the redundant `toString()` calls.
--
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-2548. Stabilize timing-sensitive Ratis tests [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #1475:
URL: https://github.com/apache/ratis/pull/1475
--
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-2548. Stabilize timing-sensitive Ratis tests [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1475:
URL: https://github.com/apache/ratis/pull/1475#discussion_r3330482105
##########
ratis-server/src/test/java/org/apache/ratis/server/impl/LeaderElectionTests.java:
##########
@@ -148,29 +148,32 @@ public void testAddServerForWaitReady() throws IOException, InterruptedException
final MiniRaftCluster cluster = newCluster(3);
cluster.start();
RaftTestUtil.waitForLeader(cluster);
- try (RaftClient client = cluster.createClient()) {
- for (int i = 0; i < 10; ++i) {
- RaftClientReply reply = client.io().send(new RaftTestUtil.SimpleMessage("message_" + i));
- assertTrue(reply.isSuccess());
- }
- // add 3 new servers and wait longer time
+ try {
CodeInjectionForTesting.put(RaftServerImpl.START_COMPLETE, new SleepCode(2000));
- final PeerChanges peerChanges = cluster.addNewPeers(2, true);
- LOG.info("add new 3 servers");
- LOG.info(cluster.printServers());
- RaftClientReply reply = client.admin().setConfiguration(SetConfigurationRequest.Arguments.newBuilder()
- .setServersInNewConf(peerChanges.getAddedPeers())
- .setMode(SetConfigurationRequest.Mode.ADD).build());
- assertTrue(reply.isSuccess());
- for (RaftServer server : cluster.getServers()) {
- RaftServerProxy proxy = (RaftServerProxy) server;
- proxy.getImpls().forEach(s -> {
- assertTrue(s.isRunning());
- });
+ try (RaftClient client = cluster.createClient()) {
+ for (int i = 0; i < 10; ++i) {
+ RaftClientReply reply = client.io().send(new RaftTestUtil.SimpleMessage("message_" + i));
+ assertTrue(reply.isSuccess());
+ }
+ // add 3 new servers and wait longer time
+ final PeerChanges peerChanges = cluster.addNewPeers(2, true);
+ LOG.info("add new 3 servers");
+ LOG.info(cluster.printServers());
+ RaftClientReply reply = client.admin().setConfiguration(SetConfigurationRequest.Arguments.newBuilder()
+ .setServersInNewConf(peerChanges.getAddedPeers())
+ .setMode(SetConfigurationRequest.Mode.ADD).build());
+ assertTrue(reply.isSuccess());
+ JavaUtils.attempt(() -> {
+ for (RaftServer server : cluster.getServers()) {
+ RaftServerProxy proxy = (RaftServerProxy) server;
+ proxy.getImpls().forEach(s -> assertTrue(s.isRunning()));
+ }
+ }, 10, HUNDRED_MILLIS, "all server impls running", LOG);
}
+ } finally {
+ cluster.shutdown();
+ CodeInjectionForTesting.remove(RaftServerImpl.START_COMPLETE);
}
Review Comment:
Adding finally is good but the double try-block is not needed. Since this is only a test, I am fine with the current change.
```diff
+++ b/ratis-server/src/test/java/org/apache/ratis/server/impl/LeaderElectionTests.java
@@ -162,15 +162,16 @@ public abstract class LeaderElectionTests<CLUSTER extends MiniRaftCluster>
.setServersInNewConf(peerChanges.getAddedPeers())
.setMode(SetConfigurationRequest.Mode.ADD).build());
assertTrue(reply.isSuccess());
- for (RaftServer server : cluster.getServers()) {
- RaftServerProxy proxy = (RaftServerProxy) server;
- proxy.getImpls().forEach(s -> {
- assertTrue(s.isRunning());
- });
- }
+ JavaUtils.attempt(() -> {
+ for (RaftServer server : cluster.getServers()) {
+ RaftServerProxy proxy = (RaftServerProxy) server;
+ proxy.getImpls().forEach(s -> assertTrue(s.isRunning()));
+ }
+ }, 10, HUNDRED_MILLIS, "all server impls running", LOG);
+ } finally {
+ cluster.shutdown();
+ CodeInjectionForTesting.remove(RaftServerImpl.START_COMPLETE);
}
- cluster.shutdown();;
- CodeInjectionForTesting.remove(RaftServerImpl.START_COMPLETE);
}
```
--
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-2548. Stabilize timing-sensitive Ratis tests [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1475:
URL: https://github.com/apache/ratis/pull/1475#discussion_r3325497505
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -1154,6 +1154,10 @@ public boolean checkLeadership() {
final RaftConfigurationImpl conf = server.getRaftConf();
+ if (conf.isSingleMode(server.getId())) {
+ return true;
+ }
Review Comment:
Is this a bug fix or just a performance improvement?
If this is not a bug fix, let's do it separately. Then, this PR changes only the test code.
##########
ratis-server/src/test/java/org/apache/ratis/RaftBasicTests.java:
##########
@@ -127,46 +128,41 @@ static void runTestBasicAppendEntries(
final CompletableFuture<Void> killAndRestartFollower = killAndRestartServer(
cluster.getFollowers().get(0).getId(), 0, 1000, cluster, log);
- final CompletableFuture<Void> killAndRestartLeader;
- if (killLeader) {
- log.info("killAndRestart leader " + leader.getId());
- killAndRestartLeader = killAndRestartServer(leader.getId(), 2000, 4000, cluster, log);
- } else {
- killAndRestartLeader = CompletableFuture.completedFuture(null);
- }
-
- log.info(cluster.printServers());
+ CompletableFuture<Void> killAndRestartLeader = CompletableFuture.completedFuture(null);
final SimpleMessage[] messages = SimpleMessage.create(numMessages);
- try (final RaftClient client = cluster.createClient()) {
- final AtomicInteger asyncReplyCount = new AtomicInteger();
- final CompletableFuture<Void> f = new CompletableFuture<>();
+ try {
+ log.info(cluster.printServers());
+
+ try (final RaftClient client = cluster.createClient()) {
+ final List<CompletableFuture<RaftClientReply>> asyncReplies = new ArrayList<>();
- for (SimpleMessage message : messages) {
+ for (SimpleMessage message : messages) {
+ if (async) {
+ asyncReplies.add(client.async().send(message));
+ } else {
+ final RaftClientReply reply = client.io().send(message);
+ Assertions.assertTrue(reply.isSuccess());
+ }
+ }
if (async) {
- client.async().send(message).thenAcceptAsync(reply -> {
- if (!reply.isSuccess()) {
- f.completeExceptionally(
- new AssertionError("Failed with reply " + reply));
- } else if (asyncReplyCount.incrementAndGet() == messages.length) {
- f.complete(null);
- }
+ CompletableFuture.allOf(asyncReplies.toArray(new CompletableFuture<?>[0])).join();
+ asyncReplies.forEach(f -> {
+ final RaftClientReply reply = f.join();
+ Assertions.assertTrue(reply.isSuccess(), () -> "Failed with reply " + reply);
});
- } else {
- final RaftClientReply reply = client.io().send(message);
- Assertions.assertTrue(reply.isSuccess());
}
}
- if (async) {
- f.join();
- Assertions.assertEquals(messages.length, asyncReplyCount.get());
+ if (killLeader) {
+ log.info("killAndRestart leader " + leader.getId());
+ killAndRestartLeader = killAndRestartServer(leader.getId(), 0, 4000, cluster, log);
}
+ Thread.sleep(cluster.getTimeoutMax().toIntExact(TimeUnit.MILLISECONDS) + 100);
+ } finally {
+ CompletableFuture.allOf(killAndRestartFollower, killAndRestartLeader).join();
}
- Thread.sleep(cluster.getTimeoutMax().toIntExact(TimeUnit.MILLISECONDS) + 100);
log.info(cluster.printAllLogs());
- killAndRestartFollower.join();
- killAndRestartLeader.join();
Review Comment:
> Wait for restart futures before continuing to log assertions in RaftBasicTests.
You are right that we should join before printing the log.
How about we simply move cluster.printAllLogs() up? The try-finally make the code harder to read.
```java
Thread.sleep(cluster.getTimeoutMax().toIntExact(TimeUnit.MILLISECONDS) + 100);
- log.info(cluster.printAllLogs());
killAndRestartFollower.join();
killAndRestartLeader.join();
+ log.info(cluster.printAllLogs());
```
##########
ratis-test/src/test/java/org/apache/ratis/shell/cli/sh/ElectionCommandIntegrationTest.java:
##########
@@ -150,13 +150,16 @@ public void testElectionStepDownCommand() throws Exception {
void runTestElectionStepDownCommand(MiniRaftCluster cluster) throws Exception {
final RaftServer.Division leader = RaftTestUtil.waitForLeader(cluster);
String sb = getClusterAddress(cluster);
- RaftServer.Division newLeader = cluster.getFollowers().get(0);
final StringPrintStream out = new StringPrintStream();
RatisShell shell = new RatisShell(out.getPrintStream());
- Assertions.assertNotEquals(cluster.getLeader().getId(), newLeader.getId());
Assertions.assertEquals(2, cluster.getFollowers().size());
- int ret = shell.run("election", "stepDown", "-peers", sb.toString());
+ int ret = shell.run("election", "pause", "-peers", sb.toString(), "-address",
+ leader.getPeer().getAddress());
+ Assertions.assertEquals(0, ret);
+
+ ret = shell.run("election", "stepDown", "-peers", sb.toString());
Review Comment:
This change is good. Could you also remove the redundant toString() calls?
```java
int ret = shell.run("election", "pause", "-peers", sb, "-address", leader.getPeer().getAddress());
Assertions.assertEquals(0, ret);
ret = shell.run("election", "stepDown", "-peers", sb);
Assertions.assertEquals(0, ret);
```
##########
ratis-server/src/test/java/org/apache/ratis/RaftBasicTests.java:
##########
@@ -127,46 +128,41 @@ static void runTestBasicAppendEntries(
final CompletableFuture<Void> killAndRestartFollower = killAndRestartServer(
cluster.getFollowers().get(0).getId(), 0, 1000, cluster, log);
- final CompletableFuture<Void> killAndRestartLeader;
- if (killLeader) {
- log.info("killAndRestart leader " + leader.getId());
- killAndRestartLeader = killAndRestartServer(leader.getId(), 2000, 4000, cluster, log);
- } else {
- killAndRestartLeader = CompletableFuture.completedFuture(null);
- }
-
- log.info(cluster.printServers());
+ CompletableFuture<Void> killAndRestartLeader = CompletableFuture.completedFuture(null);
final SimpleMessage[] messages = SimpleMessage.create(numMessages);
- try (final RaftClient client = cluster.createClient()) {
- final AtomicInteger asyncReplyCount = new AtomicInteger();
- final CompletableFuture<Void> f = new CompletableFuture<>();
+ try {
+ log.info(cluster.printServers());
+
+ try (final RaftClient client = cluster.createClient()) {
+ final List<CompletableFuture<RaftClientReply>> asyncReplies = new ArrayList<>();
- for (SimpleMessage message : messages) {
+ for (SimpleMessage message : messages) {
+ if (async) {
+ asyncReplies.add(client.async().send(message));
+ } else {
+ final RaftClientReply reply = client.io().send(message);
+ Assertions.assertTrue(reply.isSuccess());
+ }
+ }
if (async) {
- client.async().send(message).thenAcceptAsync(reply -> {
- if (!reply.isSuccess()) {
- f.completeExceptionally(
- new AssertionError("Failed with reply " + reply));
- } else if (asyncReplyCount.incrementAndGet() == messages.length) {
- f.complete(null);
- }
+ CompletableFuture.allOf(asyncReplies.toArray(new CompletableFuture<?>[0])).join();
Review Comment:
Since join() is called below. This allOf is not needed. Let's remove it.
BTW, changing
```java
final AtomicInteger asyncReplyCount = new AtomicInteger();
final CompletableFuture<Void> f = new CompletableFuture<>();
```
to
```java
final List<CompletableFuture<RaftClientReply>> asyncReplies = new ArrayList<>();
```
does make the code easier to understand (although the original code is also correct.)
##########
ratis-server/src/test/java/org/apache/ratis/RaftBasicTests.java:
##########
@@ -127,46 +128,41 @@ static void runTestBasicAppendEntries(
final CompletableFuture<Void> killAndRestartFollower = killAndRestartServer(
cluster.getFollowers().get(0).getId(), 0, 1000, cluster, log);
- final CompletableFuture<Void> killAndRestartLeader;
- if (killLeader) {
- log.info("killAndRestart leader " + leader.getId());
- killAndRestartLeader = killAndRestartServer(leader.getId(), 2000, 4000, cluster, log);
- } else {
- killAndRestartLeader = CompletableFuture.completedFuture(null);
- }
-
- log.info(cluster.printServers());
+ CompletableFuture<Void> killAndRestartLeader = CompletableFuture.completedFuture(null);
final SimpleMessage[] messages = SimpleMessage.create(numMessages);
- try (final RaftClient client = cluster.createClient()) {
- final AtomicInteger asyncReplyCount = new AtomicInteger();
- final CompletableFuture<Void> f = new CompletableFuture<>();
+ try {
+ log.info(cluster.printServers());
+
+ try (final RaftClient client = cluster.createClient()) {
+ final List<CompletableFuture<RaftClientReply>> asyncReplies = new ArrayList<>();
- for (SimpleMessage message : messages) {
+ for (SimpleMessage message : messages) {
+ if (async) {
+ asyncReplies.add(client.async().send(message));
+ } else {
+ final RaftClientReply reply = client.io().send(message);
+ Assertions.assertTrue(reply.isSuccess());
+ }
+ }
if (async) {
- client.async().send(message).thenAcceptAsync(reply -> {
- if (!reply.isSuccess()) {
- f.completeExceptionally(
- new AssertionError("Failed with reply " + reply));
- } else if (asyncReplyCount.incrementAndGet() == messages.length) {
- f.complete(null);
- }
+ CompletableFuture.allOf(asyncReplies.toArray(new CompletableFuture<?>[0])).join();
+ asyncReplies.forEach(f -> {
+ final RaftClientReply reply = f.join();
+ Assertions.assertTrue(reply.isSuccess(), () -> "Failed with reply " + reply);
});
- } else {
- final RaftClientReply reply = client.io().send(message);
- Assertions.assertTrue(reply.isSuccess());
}
}
- if (async) {
- f.join();
- Assertions.assertEquals(messages.length, asyncReplyCount.get());
+ if (killLeader) {
+ log.info("killAndRestart leader " + leader.getId());
+ killAndRestartLeader = killAndRestartServer(leader.getId(), 0, 4000, cluster, log);
}
Review Comment:
> Wait for async append replies before injecting the kill-leader restart in RaftBasicTests.
Before this change, killLeader is in the beginning. This change moves it to the end. It makes the test easier to pass but not fixing a bug.
It is good to test killLeader before client sending messages. So, let's don't make this change?
--
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-2548. Stabilize timing-sensitive Ratis tests [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1475:
URL: https://github.com/apache/ratis/pull/1475#discussion_r3325497505
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -1154,6 +1154,10 @@ public boolean checkLeadership() {
final RaftConfigurationImpl conf = server.getRaftConf();
+ if (conf.isSingleMode(server.getId())) {
+ return true;
+ }
Review Comment:
Let's do this change separately. Then, this PR changes only the test code.
--
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-2548. Stabilize timing-sensitive Ratis tests [ratis]
Posted by "CRZbulabula (via GitHub)" <gi...@apache.org>.
CRZbulabula commented on PR #1475:
URL: https://github.com/apache/ratis/pull/1475#issuecomment-4585944423
Pushed `fb3ee4ee` to address the latest CI failures.
Summary:
- `TestManualRestoreSnapshot` now verifies that the restarted follower has restored and applied at least the saved snapshot index, instead of racing against the leader current lastApplied value.
- `testAddServerForWaitReady` now waits until the new server impls report running after the ADD configuration change, and always cleans up the START_COMPLETE injection in a finally block.
Local verification:
- `mvn -pl ratis-examples -am -Dtest=TestManualRestoreSnapshot test`
- `mvn -pl ratis-test -am -Dtest=TestLeaderElectionWithSimulatedRpc#testAddServerForWaitReady,TestLeaderElectionWithGrpc#testAddServerForWaitReady,TestLeaderElectionWithNetty#testAddServerForWaitReady test`
GitHub Actions for `fb3ee4ee` are green; coverage is skipped as expected.
--
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-2548. Stabilize timing-sensitive Ratis tests [ratis]
Posted by "CRZbulabula (via GitHub)" <gi...@apache.org>.
CRZbulabula commented on code in PR #1475:
URL: https://github.com/apache/ratis/pull/1475#discussion_r3329719365
##########
ratis-server/src/test/java/org/apache/ratis/RaftBasicTests.java:
##########
@@ -127,46 +128,41 @@ static void runTestBasicAppendEntries(
final CompletableFuture<Void> killAndRestartFollower = killAndRestartServer(
cluster.getFollowers().get(0).getId(), 0, 1000, cluster, log);
- final CompletableFuture<Void> killAndRestartLeader;
- if (killLeader) {
- log.info("killAndRestart leader " + leader.getId());
- killAndRestartLeader = killAndRestartServer(leader.getId(), 2000, 4000, cluster, log);
- } else {
- killAndRestartLeader = CompletableFuture.completedFuture(null);
- }
-
- log.info(cluster.printServers());
+ CompletableFuture<Void> killAndRestartLeader = CompletableFuture.completedFuture(null);
final SimpleMessage[] messages = SimpleMessage.create(numMessages);
- try (final RaftClient client = cluster.createClient()) {
- final AtomicInteger asyncReplyCount = new AtomicInteger();
- final CompletableFuture<Void> f = new CompletableFuture<>();
+ try {
+ log.info(cluster.printServers());
+
+ try (final RaftClient client = cluster.createClient()) {
+ final List<CompletableFuture<RaftClientReply>> asyncReplies = new ArrayList<>();
- for (SimpleMessage message : messages) {
+ for (SimpleMessage message : messages) {
+ if (async) {
+ asyncReplies.add(client.async().send(message));
+ } else {
+ final RaftClientReply reply = client.io().send(message);
+ Assertions.assertTrue(reply.isSuccess());
+ }
+ }
if (async) {
- client.async().send(message).thenAcceptAsync(reply -> {
- if (!reply.isSuccess()) {
- f.completeExceptionally(
- new AssertionError("Failed with reply " + reply));
- } else if (asyncReplyCount.incrementAndGet() == messages.length) {
- f.complete(null);
- }
+ CompletableFuture.allOf(asyncReplies.toArray(new CompletableFuture<?>[0])).join();
+ asyncReplies.forEach(f -> {
+ final RaftClientReply reply = f.join();
+ Assertions.assertTrue(reply.isSuccess(), () -> "Failed with reply " + reply);
});
- } else {
- final RaftClientReply reply = client.io().send(message);
- Assertions.assertTrue(reply.isSuccess());
}
}
- if (async) {
- f.join();
- Assertions.assertEquals(messages.length, asyncReplyCount.get());
+ if (killLeader) {
+ log.info("killAndRestart leader " + leader.getId());
+ killAndRestartLeader = killAndRestartServer(leader.getId(), 0, 4000, cluster, log);
}
+ Thread.sleep(cluster.getTimeoutMax().toIntExact(TimeUnit.MILLISECONDS) + 100);
+ } finally {
+ CompletableFuture.allOf(killAndRestartFollower, killAndRestartLeader).join();
}
- Thread.sleep(cluster.getTimeoutMax().toIntExact(TimeUnit.MILLISECONDS) + 100);
log.info(cluster.printAllLogs());
- killAndRestartFollower.join();
- killAndRestartLeader.join();
Review Comment:
Done. Removed the try/finally restructuring and now join the restart futures before `cluster.printAllLogs()`.
##########
ratis-server/src/test/java/org/apache/ratis/RaftBasicTests.java:
##########
@@ -127,46 +128,41 @@ static void runTestBasicAppendEntries(
final CompletableFuture<Void> killAndRestartFollower = killAndRestartServer(
cluster.getFollowers().get(0).getId(), 0, 1000, cluster, log);
- final CompletableFuture<Void> killAndRestartLeader;
- if (killLeader) {
- log.info("killAndRestart leader " + leader.getId());
- killAndRestartLeader = killAndRestartServer(leader.getId(), 2000, 4000, cluster, log);
- } else {
- killAndRestartLeader = CompletableFuture.completedFuture(null);
- }
-
- log.info(cluster.printServers());
+ CompletableFuture<Void> killAndRestartLeader = CompletableFuture.completedFuture(null);
final SimpleMessage[] messages = SimpleMessage.create(numMessages);
- try (final RaftClient client = cluster.createClient()) {
- final AtomicInteger asyncReplyCount = new AtomicInteger();
- final CompletableFuture<Void> f = new CompletableFuture<>();
+ try {
+ log.info(cluster.printServers());
+
+ try (final RaftClient client = cluster.createClient()) {
+ final List<CompletableFuture<RaftClientReply>> asyncReplies = new ArrayList<>();
- for (SimpleMessage message : messages) {
+ for (SimpleMessage message : messages) {
+ if (async) {
+ asyncReplies.add(client.async().send(message));
+ } else {
+ final RaftClientReply reply = client.io().send(message);
+ Assertions.assertTrue(reply.isSuccess());
+ }
+ }
if (async) {
- client.async().send(message).thenAcceptAsync(reply -> {
- if (!reply.isSuccess()) {
- f.completeExceptionally(
- new AssertionError("Failed with reply " + reply));
- } else if (asyncReplyCount.incrementAndGet() == messages.length) {
- f.complete(null);
- }
+ CompletableFuture.allOf(asyncReplies.toArray(new CompletableFuture<?>[0])).join();
Review Comment:
Done. Removed the redundant `CompletableFuture.allOf(...).join()` and kept the reply list 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-2548. Stabilize timing-sensitive Ratis tests [ratis]
Posted by "CRZbulabula (via GitHub)" <gi...@apache.org>.
CRZbulabula commented on code in PR #1475:
URL: https://github.com/apache/ratis/pull/1475#discussion_r3329718860
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -1154,6 +1154,10 @@ public boolean checkLeadership() {
final RaftConfigurationImpl conf = server.getRaftConf();
+ if (conf.isSingleMode(server.getId())) {
+ return true;
+ }
Review Comment:
Done. Removed the `LeaderStateImpl` change from this PR, so the current PR diff is test-only now. I will handle that leadership check separately.
##########
ratis-server/src/test/java/org/apache/ratis/RaftBasicTests.java:
##########
@@ -127,46 +128,41 @@ static void runTestBasicAppendEntries(
final CompletableFuture<Void> killAndRestartFollower = killAndRestartServer(
cluster.getFollowers().get(0).getId(), 0, 1000, cluster, log);
- final CompletableFuture<Void> killAndRestartLeader;
- if (killLeader) {
- log.info("killAndRestart leader " + leader.getId());
- killAndRestartLeader = killAndRestartServer(leader.getId(), 2000, 4000, cluster, log);
- } else {
- killAndRestartLeader = CompletableFuture.completedFuture(null);
- }
-
- log.info(cluster.printServers());
+ CompletableFuture<Void> killAndRestartLeader = CompletableFuture.completedFuture(null);
final SimpleMessage[] messages = SimpleMessage.create(numMessages);
- try (final RaftClient client = cluster.createClient()) {
- final AtomicInteger asyncReplyCount = new AtomicInteger();
- final CompletableFuture<Void> f = new CompletableFuture<>();
+ try {
+ log.info(cluster.printServers());
+
+ try (final RaftClient client = cluster.createClient()) {
+ final List<CompletableFuture<RaftClientReply>> asyncReplies = new ArrayList<>();
- for (SimpleMessage message : messages) {
+ for (SimpleMessage message : messages) {
+ if (async) {
+ asyncReplies.add(client.async().send(message));
+ } else {
+ final RaftClientReply reply = client.io().send(message);
+ Assertions.assertTrue(reply.isSuccess());
+ }
+ }
if (async) {
- client.async().send(message).thenAcceptAsync(reply -> {
- if (!reply.isSuccess()) {
- f.completeExceptionally(
- new AssertionError("Failed with reply " + reply));
- } else if (asyncReplyCount.incrementAndGet() == messages.length) {
- f.complete(null);
- }
+ CompletableFuture.allOf(asyncReplies.toArray(new CompletableFuture<?>[0])).join();
+ asyncReplies.forEach(f -> {
+ final RaftClientReply reply = f.join();
+ Assertions.assertTrue(reply.isSuccess(), () -> "Failed with reply " + reply);
});
- } else {
- final RaftClientReply reply = client.io().send(message);
- Assertions.assertTrue(reply.isSuccess());
}
}
- if (async) {
- f.join();
- Assertions.assertEquals(messages.length, asyncReplyCount.get());
+ if (killLeader) {
+ log.info("killAndRestart leader " + leader.getId());
+ killAndRestartLeader = killAndRestartServer(leader.getId(), 0, 4000, cluster, log);
}
Review Comment:
Done. Restored the kill-leader timing so the leader restart is scheduled before client messages are sent. The final kill-leader assertion now checks that the expected messages appear in order, while allowing an extra state-machine entry produced by retry/failover.
--
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