Dan Burkert has posted comments on this change. Change subject: [java client] Add a way to restart processes ......................................................................
Patch Set 1: (9 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'. 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.*; I'm not a fan of this, but I don't know what our style guide says. Line 56: private final Map<Integer, String[]> commandLines = new ConcurrentHashMap<>(); I think this would be simpler as Map<Integer, List<String>>. ProcessBuilder internally uses List<String> Line 111: List<Integer> ports = TestUtils.findFreePorts(startPort, numTservers * 2); I don't understand why we are doing a linear search from a startPort instead of just binding to port 0 and returning whatever the OS gives us as the free port. Line 113: String dataDirPath = baseDirPath + "/ts-" + i + "-" + now; Do we run into issue when time is removed from the path? This seems like it's just asking to leak directories, without us ever noticing. Line 167: long now = System.currentTimeMillis(); In the TS equivalent above, System.currentTimeMillis() is outside the loop. 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 ignored in BaseKuduTest::restartLeaderMaster. I think throwing on fail will make issues easier to track down. 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 53: Assert.assertEquals(masterPort, TestUtils.findFreePort(masterPort)); This is extremely racy. What is it even testing? Line 54: cluster.restartDeadMasterOnPort(masterPort); Another place where the return should be used. -- 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
