Grant Henke 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 agree we could make the rule more flexible to not require setting up a cluster. Given this patch is fairly large and I have a few changes that need this, do you mind if I do that as a follow on change? 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 I agree. 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 Done 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 bei We annotate that RemoteTable is a private API. I don't think I should note the exact location it's used because that is documentation easily subject to being wrong overtime and finding where it is used is easy. 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 t Mainly this prevented me from needing modify a ton of test cases. Any new tests would likely just use the client directly from the harness without this indirection. 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. Done 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 I didn't change this, but it could be address in a follow on patch. 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. Done 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 configu Done 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. I planned to migrate any tests not currently using BaseKuduTest/KuduRule in follow up patches. I will add a retry rule for now, but it should get replaced. 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. Done 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 founda I agree, naming this was hard for me and I expected to get some feedback on it. Do you think it is Important to include "Rule" in the name give that is how it is intended to be used? 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 Done 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 Done 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 usin I will actually remove this. We have commentary elsewhere on it's existence and the Maven build is not long for this world. 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? Done 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? Done 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 pri Done 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? I actually think I will remove this. It's a carry over from the base class, but it provides effectively no functionality given it just passes through to the sync client, which you can do yourself. 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 Done 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 I think I will remove this for similar reasons to the createTable method above. 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. Done -- 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Mon, 01 Oct 2018 19:55:46 +0000 Gerrit-HasComments: Yes
