Jean-Daniel Cryans has posted comments on this change. Change subject: [java client] Add a way to restart processes ......................................................................
Patch Set 1: (15 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 358: * Picks at random a tablet server that serves tables from the passed table and restarts it. Waits > I think this should be 'serves tablets' instead of 'serves tables'. Done Line 370: Random random = new Random(); > Shouldn't create a new Random() instance with each call; that's equivalent We currently care about getting a random behavior since it's for the integration test. Still, I'll bring it out. Line 375: Thread.sleep(1000); > Why is the sleep necessary? Give a chance for the clients to fail reaching that TS (or below that master). 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, Done Line 56: private final Map<Integer, String[]> commandLines = new ConcurrentHashMap<>(); > Rather than introducing yet another map, can you create a new class that wr Three things: - We access Process directly a lot, so a wrapper class gets in the way. - I can't extend Process either, see how it gets built. - We care about keeping the historical list of processes that were started, and we also care about knowing which ones were killed. So in the end a wrapper class could manage processes that restart, but it seems clunkier. Line 111: List<Integer> ports = TestUtils.findFreePorts(startPort, numTservers * 2); > I don't understand why we are doing a linear search from a startPort instea We'd then have to query the TS to know which port it picked. Line 113: String dataDirPath = baseDirPath + "/ts-" + i + "-" + now; > Do we run into issue when time is removed from the path? This seems like i Not that I'm aware of, but then again we wouldn't notice it. Line 167: long now = System.currentTimeMillis(); > In the TS equivalent above, System.currentTimeMillis() is outside the loop. Yeah I wonder why... I guess I can change it and hopefully it doesn't break anything. Line 244: return restartDeadProcessOnPort (port, masterProcesses); > Extra whitespace here. On L254 too. Done Line 257: private boolean restartDeadProcessOnPort(int port, Map<Integer, Process> map) throws Exception { > Having this return boolean seems dangerous to me. E.g. the return is ignor You're right. Line 366: @VisibleForTesting > Nit: technically, any method in these classes is "visible for testing" or e I wanted to identify that this particular method is only visible for this class' own tests. I can remove and move this as a javadoc comment. 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 "As Done Line 53: Assert.assertEquals(masterPort, TestUtils.findFreePort(masterPort)); > This is extremely racy. What is it even testing? It tests that the process is gone. Except on OSX we assign different hostnames to each test that runs so I doubt it's as racy as you think? Line 54: cluster.restartDeadMasterOnPort(masterPort); > Another place where the return should be used. It's now throwing an exception. 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. Done. Sorry, new IDE, didn't move over my settings. -- 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: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
