Adar Dembo has posted comments on this change. Change subject: [java client] Add a way to restart processes ......................................................................
Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/2828/1/java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java File java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java: Line 370: Random random = new Random(); Shouldn't create a new Random() instance with each call; that's equivalent to reseeding with every call. Line 375: Thread.sleep(1000); Why is the sleep necessary? http://gerrit.cloudera.org:8080/#/c/2828/1/java/kudu-client/src/test/java/org/kududb/client/MiniKuduCluster.java File java/kudu-client/src/test/java/org/kududb/client/MiniKuduCluster.java: Line 31: import java.util.*; Can you revert this change? As long as we haven't converged on a standard, let's not ping-pong wildcard vs. non-wildcard imports in existing files. Line 56: private final Map<Integer, String[]> commandLines = new ConcurrentHashMap<>(); Rather than introducing yet another map, can you create a new class that wraps java.util.Process and augments it with the command line? Line 244: return restartDeadProcessOnPort (port, masterProcesses); Extra whitespace here. On L254 too. Line 366: @VisibleForTesting Nit: technically, any method in these classes is "visible for testing" or even "only exists for testing". That is, @VisibleForTesting is only really worth using in production classes, not test classes. http://gerrit.cloudera.org:8080/#/c/2828/1/java/kudu-client/src/test/java/org/kududb/client/TestMiniKuduCluster.java File java/kudu-client/src/test/java/org/kududb/client/TestMiniKuduCluster.java: Line 31: Assert.assertTrue(cluster.waitForTabletServers(NUM_TABLET_SERVERS)); Nit: can you statically import org.junit.Assert.* so you could drop the "Assert." prefixes? I find them to be just noise. http://gerrit.cloudera.org:8080/#/c/2828/1/java/kudu-client/src/test/java/org/kududb/client/TestUtils.java File java/kudu-client/src/test/java/org/kududb/client/TestUtils.java: Line 32: import java.net.*; Unwildcard this. -- To view, visit http://gerrit.cloudera.org:8080/2828 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I065917cd219e1dd8fa93fed9ca36d58a3372935d Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
