You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2018/09/19 19:33:06 UTC
[kudu-CR] [test] Adjust Kudu binary locator logic.
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11475
Change subject: [test] Adjust Kudu binary locator logic.
......................................................................
[test] Adjust Kudu binary locator logic.
This patch adjusts the binary locating logic to be less fragile and more externally useful. It also breaks the
logic out of the TestUtils class into it’s own class.
I broke out the remaining TestUtils methods as
well and remove any that were unused.
This patch also adjusts the Maven and Gradle build
to define the `kuduBinDir` system property. Doing
this means we don’t need to detect git repos and
search the repository for binaries. This also fixes
the issue where Maven tests would fail to locate the
binary because they were using jars installed into
the local repository.
Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d
---
M java/README.adoc
M java/buildSrc/src/main/groovy/org/apache/kudu/gradle/DistTestTask.java
M java/gradle/tests.gradle
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
D java/kudu-client/src/test/java/org/apache/kudu/client/TestTestUtils.java
D java/kudu-client/src/test/java/org/apache/kudu/client/TestUtils.java
A java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java
A java/kudu-client/src/test/java/org/apache/kudu/util/ProtocolUtils.java
A java/kudu-client/src/test/java/org/apache/kudu/util/RandomUtils.java
M java/pom.xml
14 files changed, 241 insertions(+), 365 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/11475/1
--
To view, visit http://gerrit.cloudera.org:8080/11475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d
Gerrit-Change-Number: 11475
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
[kudu-CR] [test] Adjust Kudu binary locator logic.
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11475 )
Change subject: [test] Adjust Kudu binary locator logic.
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11475/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11475/3//COMMIT_MSG@10
PS3, Line 10: It also breaks the
: logic out of the TestUtils class into it’s own class.
: I broke out the remaining TestUtils methods as
: well
I'm -0 on the proliferation of util classes. What's the motivation? It definitely makes the changes to finding the binary dir harder to follow.
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java
File java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java:
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@35
PS1, Line 35: public class KuduBinaryLocator {
> Part of why it is separate is that I had considered the case where someone
Why would we want an overridable abstraction here, as opposed to requiring the application to set the kuduBinDir property?
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@60
PS1, Line 60: String kuduHomeVar = System.getenv(KUDU_HOME_VAR);
> Inside the kudu source tree, and most source trees honestly, I expect the s
I agree with Adar, we have not used KUDU_HOME in this way previously, and I'd caution against introducing it here. These kind of conventions are really painful to remove once people start using them. As a concrete example, the python and rust test harnesses use KUDU_HOME in a different way:
https://github.com/apache/kudu/blob/master/python/setup.py#L104-L124
https://github.com/danburkert/kudu-rs/blob/a059fe9ec4559d8aec80a92a2c3f6601a525caeb/src/mini_cluster.rs#L140-L164
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@71
PS1, Line 71: Runtime runtime = Runtime.getRuntime();
> I guess animal sniffer doesn't check test classes: https://github.com/mojoh
try-with-resources is Java 7, and I think this code was more straightforward when it used it.
http://gerrit.cloudera.org:8080/#/c/11475/3/java/kudu-client/src/test/java/org/apache/kudu/util/ProtocolUtils.java
File java/kudu-client/src/test/java/org/apache/kudu/util/ProtocolUtils.java:
http://gerrit.cloudera.org:8080/#/c/11475/3/java/kudu-client/src/test/java/org/apache/kudu/util/ProtocolUtils.java@29
PS3, Line 29: public class ProtocolUtils {
I think this would be better named 'ProtobufUtils'.
--
To view, visit http://gerrit.cloudera.org:8080/11475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d
Gerrit-Change-Number: 11475
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 20 Sep 2018 18:51:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] [test] Adjust Kudu binary locator logic.
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11475 )
Change subject: [test] Adjust Kudu binary locator logic.
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d
Gerrit-Change-Number: 11475
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 21 Sep 2018 16:03:08 +0000
Gerrit-HasComments: No
[kudu-CR] [test] Adjust Kudu binary locator logic.
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11475 )
Change subject: [test] Adjust Kudu binary locator logic.
......................................................................
Patch Set 7: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/11475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d
Gerrit-Change-Number: 11475
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 21 Sep 2018 05:22:31 +0000
Gerrit-HasComments: No
[kudu-CR] [test] Adjust Kudu binary locator logic.
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Dan Burkert, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11475
to look at the new patch set (#4).
Change subject: [test] Adjust Kudu binary locator logic.
......................................................................
[test] Adjust Kudu binary locator logic.
This patch adjusts the binary locating logic to be less
fragile and more externally useful. It also breaks the
logic out of the TestUtils class into it’s own class.
I broke out the remaining TestUtils methods as
well and remove any that were unused.
This patch also adjusts the Maven and Gradle build
to define the `kuduBinDir` system property. Doing
this means we don’t need to detect git repos and
search the repository for binaries. This also fixes
the issue where Maven tests would fail to locate the
binary because they were using jars installed into
the local repository.
Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d
---
M java/README.adoc
M java/buildSrc/src/main/groovy/org/apache/kudu/gradle/DistTestTask.java
M java/gradle/tests.gradle
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
D java/kudu-client/src/test/java/org/apache/kudu/client/TestTestUtils.java
D java/kudu-client/src/test/java/org/apache/kudu/client/TestUtils.java
A java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java
A java/kudu-client/src/test/java/org/apache/kudu/util/ProtobufUtils.java
A java/kudu-client/src/test/java/org/apache/kudu/util/RandomUtils.java
M java/pom.xml
14 files changed, 232 insertions(+), 365 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/11475/4
--
To view, visit http://gerrit.cloudera.org:8080/11475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d
Gerrit-Change-Number: 11475
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [test] Adjust Kudu binary locator logic.
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11475 )
Change subject: [test] Adjust Kudu binary locator logic.
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/11475/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11475/1//COMMIT_MSG@9
PS1, Line 9: This patch adjusts the binary locating logic to be less fragile and more externally useful. It also breaks the
Nit: line too long.
http://gerrit.cloudera.org:8080/#/c/11475/1//COMMIT_MSG@17
PS1, Line 17: This also fixes
: the issue where Maven tests would fail to locate the
: binary because they were using jars installed into
: the local repository.
I presume you've already read through this: https://gerrit.cloudera.org/c/5328/
May want to give Dan a heads-up to make sure he's OK with this alternate approach.
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java
File java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java:
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@35
PS1, Line 35: public class KuduBinaryLocator {
This is only used by the MiniKuduCluster. Perhaps move it there instead of making it a standalone class?
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@53
PS1, Line 53: LOG.info("Using Kudu binary directory specified by system property '{}': {}", KUDU_BIN_DIR_PROP, kuduHomeProp);
Some of these lines look to be too long.
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@60
PS1, Line 60: LOG.info("Using Kudu home directory specified by environment variable '{}': {}", KUDU_HOME_VAR, kuduHomeVar);
In various places in the codebase, the KUDU_HOME env variable refers to the root of the Kudu source tree (i.e. where the www/ subdirectory can be found). This isn't that, so I don't think we should overload the name.
In fact, I'm not really sure what this directory is. Seems like maybe a vendor-specific directory root?
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@65
PS1, Line 65: // Last, use the kudu that is available on the path.
That's not quite what this is doing though; it's using 'kudu' on the path as a proxy for where the bin dir should be.
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@71
PS1, Line 71: try (final Reader reader = new InputStreamReader(process.getInputStream(), UTF_8)) {
Where's the 'catch' or finally' for this 'try'? Is it L78?
--
To view, visit http://gerrit.cloudera.org:8080/11475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d
Gerrit-Change-Number: 11475
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Wed, 19 Sep 2018 20:28:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] [test] Adjust Kudu binary locator logic.
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Dan Burkert, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11475
to look at the new patch set (#2).
Change subject: [test] Adjust Kudu binary locator logic.
......................................................................
[test] Adjust Kudu binary locator logic.
This patch adjusts the binary locating logic to be less
fragile and more externally useful. It also breaks the
logic out of the TestUtils class into it’s own class.
I broke out the remaining TestUtils methods as
well and remove any that were unused.
This patch also adjusts the Maven and Gradle build
to define the `kuduBinDir` system property. Doing
this means we don’t need to detect git repos and
search the repository for binaries. This also fixes
the issue where Maven tests would fail to locate the
binary because they were using jars installed into
the local repository.
Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d
---
M java/README.adoc
M java/buildSrc/src/main/groovy/org/apache/kudu/gradle/DistTestTask.java
M java/gradle/tests.gradle
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
D java/kudu-client/src/test/java/org/apache/kudu/client/TestTestUtils.java
D java/kudu-client/src/test/java/org/apache/kudu/client/TestUtils.java
A java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java
A java/kudu-client/src/test/java/org/apache/kudu/util/ProtocolUtils.java
A java/kudu-client/src/test/java/org/apache/kudu/util/RandomUtils.java
M java/pom.xml
14 files changed, 248 insertions(+), 365 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/11475/2
--
To view, visit http://gerrit.cloudera.org:8080/11475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d
Gerrit-Change-Number: 11475
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [test] Adjust Kudu binary locator logic.
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11475 )
Change subject: [test] Adjust Kudu binary locator logic.
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11475/4/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java
File java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java:
http://gerrit.cloudera.org:8080/#/c/11475/4/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@50
PS4, Line 50: String kuduHomeProp = System.getProperty(KUDU_BIN_DIR_PROP);
Nit: kuduHomeProp? Or kuduBinProp?
--
To view, visit http://gerrit.cloudera.org:8080/11475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d
Gerrit-Change-Number: 11475
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 20 Sep 2018 19:27:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] [test] Adjust Kudu binary locator logic.
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11475 )
Change subject: [test] Adjust Kudu binary locator logic.
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java
File java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java:
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@60
PS1, Line 60: String kuduHomeVar = System.getenv(KUDU_HOME_VAR);
> I was just following the ${name}_HOME pattern that is often used. For examp
Well, I guess it sort of depends on whether you expect this to be used more from inside a Kudu source tree, or from outside. If the latter, then I agree that KUDU_HOME (a la JAVA_HOME) makes more sense.
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@71
PS1, Line 71: Runtime runtime = Runtime.getRuntime();
> There is no catch or finally. This is a java "try-with-resources Statement
Hmm, I'm surprised animalsniffer didn't fail your build; it's configured to disallow any Java API usage above 1.7.
On second thought, I guess this isn't an API call but a feature of the language. Could you look into why we don't prohibit that?
--
To view, visit http://gerrit.cloudera.org:8080/11475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d
Gerrit-Change-Number: 11475
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Wed, 19 Sep 2018 21:22:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] [test] Adjust Kudu binary locator logic.
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11475 )
Change subject: [test] Adjust Kudu binary locator logic.
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java
File java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java:
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@60
PS1, Line 60: LOG.info("Using Kudu home directory specified by environment variable '{}': {}", KUDU_HOME_VAR, kuduHomeVar);
> Well, I guess it sort of depends on whether you expect this to be used more
Inside the kudu source tree, and most source trees honestly, I expect the system property to be used and defined in the build. This is the way our build works.
I expect KUDU_HOME would be used if you wanted to do some integration testing on an already installed version of Kudu or something like that.
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@71
PS1, Line 71: try (final Reader reader = new InputStreamReader(process.getInputStream(), UTF_8)) {
> Hmm, I'm surprised animalsniffer didn't fail your build; it's configured to
I guess animal sniffer doesn't check test classes: https://github.com/mojohaus/animal-sniffer/issues/5
That said, we actually need Java 8 to run tests due to some of our dependencies so it's not to big of a deal.
--
To view, visit http://gerrit.cloudera.org:8080/11475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d
Gerrit-Change-Number: 11475
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Wed, 19 Sep 2018 21:32:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] [test] Adjust Kudu binary locator logic.
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11475 )
Change subject: [test] Adjust Kudu binary locator logic.
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/11475/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11475/1//COMMIT_MSG@9
PS1, Line 9: This patch adjusts the binary locating logic to be less fragile and more externally useful. It also breaks the
> Nit: line too long.
Done
http://gerrit.cloudera.org:8080/#/c/11475/1//COMMIT_MSG@17
PS1, Line 17: This also fixes
: the issue where Maven tests would fail to locate the
: binary because they were using jars installed into
: the local repository.
> I presume you've already read through this: https://gerrit.cloudera.org/c/5
Added him to the review and pinged out of band. Based on that discussion, I don't think he will have an issue.
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java
File java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java:
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@35
PS1, Line 35: public class KuduBinaryLocator {
> This is only used by the MiniKuduCluster. Perhaps move it there instead of
Part of why it is separate is that I had considered the case where someone might want to override the logic here.
I could add a setter to the MiniKuduCluster allowing you to provide your own instance. I didn't do that yet, but I could in this patch.
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@53
PS1, Line 53: LOG.info("Using Kudu binary directory specified by system property '{}': {}", KUDU_BIN_DIR_PROP, kuduHomeProp);
> Some of these lines look to be too long.
Done
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@60
PS1, Line 60: LOG.info("Using Kudu home directory specified by environment variable '{}': {}", KUDU_HOME_VAR, kuduHomeVar);
> In various places in the codebase, the KUDU_HOME env variable refers to the
I was just following the ${name}_HOME pattern that is often used. For example, JAVA_HOME. This way KUDU_HOME could be exported to indicate the home directory of the Kudu install.
Any ideas for a better environment variable to use? The ${name}_HOME pattern seams extremely common.
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@65
PS1, Line 65: // Last, use the kudu that is available on the path.
> That's not quite what this is doing though; it's using 'kudu' on the path a
Done
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@71
PS1, Line 71: try (final Reader reader = new InputStreamReader(process.getInputStream(), UTF_8)) {
> Where's the 'catch' or finally' for this 'try'? Is it L78?
There is no catch or finally. This is a java "try-with-resources Statement" that will automatically handle closing the reader on error or success. It's effectively an automatic finally clause.
This is a Java 8 feature though, so perhaps I should remove this.
--
To view, visit http://gerrit.cloudera.org:8080/11475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d
Gerrit-Change-Number: 11475
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Wed, 19 Sep 2018 21:18:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] [test] Adjust Kudu binary locator logic.
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Dan Burkert, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11475
to look at the new patch set (#5).
Change subject: [test] Adjust Kudu binary locator logic.
......................................................................
[test] Adjust Kudu binary locator logic.
This patch adjusts the binary locating logic to be less
fragile and more externally useful. It also breaks the
logic out of the TestUtils class into it’s own class.
I broke out the remaining TestUtils methods as
well and remove any that were unused.
This patch also adjusts the Maven and Gradle build
to define the `kuduBinDir` system property. Doing
this means we don’t need to detect git repos and
search the repository for binaries. This also fixes
the issue where Maven tests would fail to locate the
binary because they were using jars installed into
the local repository.
Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d
---
M java/README.adoc
M java/buildSrc/src/main/groovy/org/apache/kudu/gradle/DistTestTask.java
M java/gradle/tests.gradle
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
D java/kudu-client/src/test/java/org/apache/kudu/client/TestTestUtils.java
D java/kudu-client/src/test/java/org/apache/kudu/client/TestUtils.java
A java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java
A java/kudu-client/src/test/java/org/apache/kudu/util/ProtobufUtils.java
A java/kudu-client/src/test/java/org/apache/kudu/util/RandomUtils.java
M java/pom.xml
14 files changed, 232 insertions(+), 365 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/11475/5
--
To view, visit http://gerrit.cloudera.org:8080/11475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d
Gerrit-Change-Number: 11475
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [test] Adjust Kudu binary locator logic.
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11475 )
Change subject: [test] Adjust Kudu binary locator logic.
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11475/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11475/3//COMMIT_MSG@10
PS3, Line 10: It also breaks the
: logic out of the TestUtils class into it’s own class.
: I broke out the remaining TestUtils methods as
: well
> I'm -0 on the proliferation of util classes. What's the motivation? It de
Part of the reason is I plan to move some of these classes around in the future to enable breaking out the MiniKuduCluster functionality for general integration testing outside the project.
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java
File java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java:
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@35
PS1, Line 35: public class KuduBinaryLocator {
> Why would we want an overridable abstraction here, as opposed to requiring
This is preliminary work to breaking out a separate module/jar for the KuduMiniCluster to support simplified integration testing. I expect that would be a workaround for any case that doesn't fit existing options.
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@60
PS1, Line 60: String kuduHomeVar = System.getenv(KUDU_HOME_VAR);
> I agree with Adar, we have not used KUDU_HOME in this way previously, and I
I will remove the KUDU_HOME option. The system property and path should be flexible enough.
http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@71
PS1, Line 71: Runtime runtime = Runtime.getRuntime();
> try-with-resources is Java 7, and I think this code was more straightforwar
Done
http://gerrit.cloudera.org:8080/#/c/11475/3/java/kudu-client/src/test/java/org/apache/kudu/util/ProtocolUtils.java
File java/kudu-client/src/test/java/org/apache/kudu/util/ProtocolUtils.java:
http://gerrit.cloudera.org:8080/#/c/11475/3/java/kudu-client/src/test/java/org/apache/kudu/util/ProtocolUtils.java@29
PS3, Line 29: public class ProtocolUtils {
> I think this would be better named 'ProtobufUtils'.
Done
--
To view, visit http://gerrit.cloudera.org:8080/11475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d
Gerrit-Change-Number: 11475
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 20 Sep 2018 19:19:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] [test] Adjust Kudu binary locator logic.
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11475 )
Change subject: [test] Adjust Kudu binary locator logic.
......................................................................
[test] Adjust Kudu binary locator logic.
This patch adjusts the binary locating logic to be less
fragile and more externally useful. It also breaks the
logic out of the TestUtils class into it’s own class.
I broke out the remaining TestUtils methods as
well and remove any that were unused.
This patch also adjusts the Maven and Gradle build
to define the `kuduBinDir` system property. Doing
this means we don’t need to detect git repos and
search the repository for binaries. This also fixes
the issue where Maven tests would fail to locate the
binary because they were using jars installed into
the local repository.
Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d
Reviewed-on: http://gerrit.cloudera.org:8080/11475
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Dan Burkert <da...@apache.org>
---
M java/README.adoc
M java/buildSrc/src/main/groovy/org/apache/kudu/gradle/DistTestTask.java
M java/gradle/tests.gradle
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
D java/kudu-client/src/test/java/org/apache/kudu/client/TestTestUtils.java
D java/kudu-client/src/test/java/org/apache/kudu/client/TestUtils.java
A java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java
A java/kudu-client/src/test/java/org/apache/kudu/util/ProtobufUtils.java
A java/kudu-client/src/test/java/org/apache/kudu/util/RandomUtils.java
M java/pom.xml
14 files changed, 232 insertions(+), 365 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, but someone else must approve
Dan Burkert: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/11475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d
Gerrit-Change-Number: 11475
Gerrit-PatchSet: 8
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [test] Adjust Kudu binary locator logic.
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11475 )
Change subject: [test] Adjust Kudu binary locator logic.
......................................................................
Patch Set 5: Code-Review+1
Will defer to Dan.
--
To view, visit http://gerrit.cloudera.org:8080/11475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d
Gerrit-Change-Number: 11475
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 20 Sep 2018 20:14:47 +0000
Gerrit-HasComments: No