Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10838 )
Change subject: KUDU-2420: Support parallel java tests ...................................................................... Patch Set 2: (3 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 > I ran the kudu-client tests with `gradle clean test`: Yeah, that'd be a good idea. You can file a JIRA too if there's too much follow-up work. 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(); > I didn't want to make too many test changes at once. I think a cleanup of t OK, though the suggestion I'm recommending would mean rewriting all those new static blocks that you added, so another patch would mean some unnecessary churn. 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@118 PS1, Line 118: File tempRoot = Files.createTempDirectory("mini-kudu-cluster").toFile(); > Do we need both? I like this functionality here because it handles the clea There's this code in tool_action_test.cc: // Normal exit, clean up cluster root. if (cluster) { cluster->Shutdown(); WARN_NOT_OK(Env::Default()->DeleteRecursively(cluster->cluster_root()), "Could not delete cluster root"); } Doesn't that clean up the cluster state when the cluster is shut down? If so, maybe we don't need this at all? Anyway, my original concern was just that if you define a test class with 20 tests in it, those 20 cluster root directories are only cleaned up after all 20 tests finish running. It'd be better to delete each one after the test is done, to keep the overall disk space consumption down. -- 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: 2 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: Fri, 29 Jun 2018 00:19:57 +0000 Gerrit-HasComments: Yes
