Grant Henke 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? I ran the kudu-client tests with `gradle clean test`: Before: 7m 18s After: 9m 34s With -PmaxParallelForks=#: 4: 5m 17s 8: 4m 23s Although TestSecurity seams to almost double the time in the test with 8 forks. I ran all the test excluding TestSecurity with 8 threads and it took 2m 28s. It looks like it has some large loops with 5 seconds sleeps. Speeding up those tests could be helpful as a follow up change. http://gerrit.cloudera.org:8080/#/c/10838/1//COMMIT_MSG@10 PS1, Line 10: - Ensures each MiniKuduCluser uses it’s own cluster root. > MiniKuduCluster Done http://gerrit.cloudera.org:8080/#/c/10838/1//COMMIT_MSG@10 PS1, Line 10: it’s > its Done http://gerrit.cloudera.org:8080/#/c/10838/1//COMMIT_MSG@13 PS1, Line 13: it’s > its Done 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? I didn't want to make too many test changes at once. I think a cleanup of the BaseKuduTest usage in a follow up would be good. 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 Yeah it is. 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 m Do we need both? I like this functionality here because it handles the clean up in the case that you didn't provide your own root directory, but lets you handle it if you provide your own. Currently the mini-cluster doesn't do any cleanup. 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 Done 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. Previously a timestamp was propagated because the static clients were used in an earlier test and those tests just happened to already perform a scan. This one baffled me for a bit too. 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 I will consider this for a BaseKuduTest and test cleanup patch. 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? BaseKuduTest? We can't because the cluster configuration changes for each test and BaseKuduTest doesn't have that type of flexibility. We could make more flexible methods in BaseKuduTest that could be leveraged, but that should be outside this change. 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? Done 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? I was toying with which method was a better practice for temp files. I will remove 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: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Thu, 28 Jun 2018 04:04:57 +0000 Gerrit-HasComments: Yes
