Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10838 )
Change subject: KUDU-2420: Support parallel java tests ...................................................................... Patch Set 1: (13 comments) http://gerrit.cloudera.org:8080/#/c/10838/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10838/1//COMMIT_MSG@7 PS1, Line 7: KUDU-2420: Support parallel java tests Without any parallelism, how much slower are Java tests now? http://gerrit.cloudera.org:8080/#/c/10838/1//COMMIT_MSG@10 PS1, Line 10: - Ensures each MiniKuduCluser uses it’s own cluster root. MiniKuduCluster http://gerrit.cloudera.org:8080/#/c/10838/1//COMMIT_MSG@10 PS1, Line 10: it’s its http://gerrit.cloudera.org:8080/#/c/10838/1//COMMIT_MSG@13 PS1, Line 13: it’s its http://gerrit.cloudera.org:8080/#/c/10838/1/java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java File java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java: http://gerrit.cloudera.org:8080/#/c/10838/1/java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java@55 PS1, Line 55: // Expose the MiniKuduCluster builder so that subclasses can alter the builder. : protected static final MiniKuduCluster.MiniKuduClusterBuilder miniClusterBuilder = : new MiniKuduCluster.MiniKuduClusterBuilder(); : : // Comma separate describing the master addresses and ports. : protected static String masterAddresses; : protected static List<HostAndPort> masterHostPorts; : : // We create both versions of the client for ease of use. : protected static AsyncKuduClient client; : protected static KuduClient syncClient; : protected static final Schema basicSchema = getBasicSchema(); : protected static final Schema allTypesSchema = getSchemaWithAllTypes(); Shouldn't these all be non-static now that they're instantiated per-test? Oh, I see, various tests use static blocks to mutate this state before setUpBase() runs. Perhaps we can do that via an overridden method in BaseKuduTest? For example, you could make miniClusterBuilder private, then provide a no-op version of "void configureCluster(MiniKuduCluster.MiniKuduClusterBuilder foo)" here. Interested tests could override it and make changes to 'foo', then setUpBase() or whatever needs to call configureCluster() before doSetup(). http://gerrit.cloudera.org:8080/#/c/10838/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java: http://gerrit.cloudera.org:8080/#/c/10838/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@115 PS1, Line 115: // If a cluster root was not set, create a temp directory to use and clean it up on exit. Could you add here whether the temp directory is guaranteed to be unique? I imagine it is (otherwise what's the point), but I wanted to be sure. http://gerrit.cloudera.org:8080/#/c/10838/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@118 PS1, Line 118: tempRoot.deleteOnExit(); Would it be possible to delete this after shutdown() too? Does the actual mini-cluster implementation (the one backed by the control shell) already do that? http://gerrit.cloudera.org:8080/#/c/10838/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@518 PS1, Line 518: private String clusterRoot = ""; Nit: I don't particularly like the convention of using empty string to mean "create a cluster root for me" when NULL is an already existing "special" value. Can we use NULL instead? http://gerrit.cloudera.org:8080/#/c/10838/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java: http://gerrit.cloudera.org:8080/#/c/10838/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@126 PS1, Line 126: // Scan a table to ensure a timestamp is propagated. : KuduTable table = syncClient.createTable(tableName, basicSchema, getBasicCreateTableOptions()); : syncClient.newScannerBuilder(table).build().nextRows().getNumRows(); : assertTrue(syncClient.hasLastPropagatedTimestamp()); : assertTrue(client.hasLastPropagatedTimestamp()); What's this doing here? Doesn't seem related to this change. http://gerrit.cloudera.org:8080/#/c/10838/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestRowResult.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestRowResult.java: http://gerrit.cloudera.org:8080/#/c/10838/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestRowResult.java@36 PS1, Line 36: // Generate a unique table name : private static final String TABLE_NAME = : TestRowResult.class.getName() + "-" + System.currentTimeMillis(); You can also get rid of these sort of things now, because there's no danger of reusing existing cluster state when retrying. Could be in a separate patch if you prefer. http://gerrit.cloudera.org:8080/#/c/10838/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java: http://gerrit.cloudera.org:8080/#/c/10838/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java@50 PS1, Line 50: public class TestSecurity { Could we make use of BaseKuduClient in this test? http://gerrit.cloudera.org:8080/#/c/10838/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/JarFinder.java File java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/JarFinder.java: http://gerrit.cloudera.org:8080/#/c/10838/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/JarFinder.java@29 PS1, Line 29: import java.nio.file.Path; Can drop? http://gerrit.cloudera.org:8080/#/c/10838/1/java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/TestJarFinder.java File java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/TestJarFinder.java: http://gerrit.cloudera.org:8080/#/c/10838/1/java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/TestJarFinder.java@50 PS1, Line 50: testDir.deleteOnExit(); Is this really necessary if tearDown() deletes it? -- To view, visit http://gerrit.cloudera.org:8080/10838 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I97c6718b022936a8839f957da0928f54ff6e7371 Gerrit-Change-Number: 10838 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Wed, 27 Jun 2018 22:47:52 +0000 Gerrit-HasComments: Yes
