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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
