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