Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11547 )
Change subject: [test] Move BaseKuduTest to a Junit Rule ...................................................................... Patch Set 1: (21 comments) I think composing RetryRule within KuduRule makes sense, but rather than leaving it up to authors to decide which to use, I think it'd be simpler if we enforced (in code review) that every test use KuduRule. Which means if it's important to allow tests to manage their own miniclusters, KuduRule should be modified to support not building a cluster. But, I suspect it'd be more clean to further parameterize KuduRule such that all custom minicluster management could be done there. http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITExportCsv.java File java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITExportCsv.java: http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client-tools/src/test/java/org/apache/kudu/mapreduce/tools/ITExportCsv.java@49 PS1, Line 49: public KuduRule kudu = new KuduRule(); 'kudu' is a pretty vague noun to use for this thing. If you end up renaming KuduRule to KuduTestFoo as I suggested in a different comment, I'd use the foo as the name of the instance. So for example if you use KuduTestHarness, this should be called 'harness'. http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/main/java/org/apache/kudu/client/LocatedTablet.java File java/kudu-client/src/main/java/org/apache/kudu/client/LocatedTablet.java: http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/main/java/org/apache/kudu/client/LocatedTablet.java@44 PS1, Line 44: public LocatedTablet(RemoteTablet tablet) { Hmm, doesn't this suggest that applications could construct these? But that would be useless since there are no public APIs that accept LocatedTablets. Can you annotate this in a way that suggests that it's only for tests? http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java File java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java: http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java@56 PS1, Line 56: public class RemoteTablet implements Comparable<RemoteTablet> { Likewise, can we annotate this as being public only to support KuduRule being in a separate package? http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java: http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java@54 PS1, Line 54: private KuduClient client; : private AsyncKuduClient asyncClient; What's the purpose of this indirection? Why not go to 'kudu' directly for the clients? I get that it's useful as shorthand, but when reviewing this my first thought was that these must be customized from the clients provided by 'kudu' in some way. This applies to some of other other tests too. http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java: http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java@35 PS1, Line 35: public class TestConnectToCluster { This test also doesn't use the retry rule. http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java@49 PS1, Line 49: MiniKuduCluster cluster = new MiniKuduCluster.MiniKuduClusterBuilder() It'd be safer for these tests to get the base cluster builder and modify it. Granted, that'd have no real effect (since the base builder only sets the number of servers), but it'd avoid problems in the future if someone adds a property to the base builder expecting it to apply universally. http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java: http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java@30 PS1, Line 30: public class TestConnectionCache { No retry rule. http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java: http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java@28 PS1, Line 28: public class TestMiniKuduCluster { This test isn't configured with KuduRule, which means it's also not configured with RetryRule. Are you going to address that in a follow-on commit? http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java: http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java@27 PS1, Line 27: public class TestNegotiation { No retry rule. http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/test/KuduRule.java File java/kudu-client/src/test/java/org/apache/kudu/test/KuduRule.java: PS1: License header. http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/test/KuduRule.java@43 PS1, Line 43: public class KuduRule extends ExternalResource { It'd be nice to use a name that describes how this class serves as a foundation for Kudu unit tests, but isn't to be extended (i.e. avoid the word 'base'). What do you think of KuduTestFramework, KuduTestInfra, KuduTestHarness, KuduTestSupport, KuduTestBasis, or KuduTestRoot? My preference would be for KuduTestHarness. http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/test/KuduRule.java@73 PS1, Line 73: usefull useful http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/test/KuduRule.java@96 PS1, Line 96: for(String flag : tabletServerConfig.flags()) { for (String Above too. http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/test/KuduRule.java@104 PS1, Line 104: // We use this with Gradle because it doesn't support : // Surefire/Failsafe rerunFailingTestsCount like Maven does. I'd rewrite this, because as written it sounds like we're somehow only using this with Gradle. You can talk about how Maven's surefire/failsafe execution frameworks both support rerunning failed tests, but Gradle doesn't, and so it was a lot easier to implement retrying as a JUnit rule to be applied universally than it was to hack up Gradle to support the equivalent logic. http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/test/KuduRule.java@115 PS1, Line 115: asyncClient = new AsyncKuduClient.AsyncKuduClientBuilder(miniCluster.getMasterAddressesAsString()) Line too long? http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/test/KuduRule.java@124 PS1, Line 124: if (asyncClient != null) { Shouldn't this be a client != null check? http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/test/KuduRule.java@138 PS1, Line 138: public KuduClient getClient() { : return client; : } : : public AsyncKuduClient getAsyncClient() { : return asyncClient; : } Since you've got getters for these, can you make client and asyncClient private? http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/test/KuduRule.java@146 PS1, Line 146: public KuduTable createTable(String tableName, Schema schema, Add Javadoc? http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/test/KuduRule.java@147 PS1, Line 147: CreateTableOptions builder) throws KuduException { Indentation http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/test/KuduRule.java@159 PS1, Line 159: Deferred<KuduTable> d = asyncClient.openTable(name); : return d.join(DEFAULT_SLEEP); Could just use the sync client to simplify this, no? I imagine the current implementation is just a holdover from KuduBaseTest. http://gerrit.cloudera.org:8080/#/c/11547/1/java/kudu-client/src/test/java/org/apache/kudu/test/KuduRule.java@404 PS1, Line 404: asyncClient = new AsyncKuduClient.AsyncKuduClientBuilder(miniCluster.getMasterAddressesAsString()) Too long. -- To view, visit http://gerrit.cloudera.org:8080/11547 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32c83b47a576377b924ea41dbeaf78ce2b75e4c4 Gerrit-Change-Number: 11547 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Comment-Date: Mon, 01 Oct 2018 18:30:38 +0000 Gerrit-HasComments: Yes
