You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2019/01/03 01:23:50 UTC
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
Mike Percy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12147
Change subject: java: KuduBinaryExtractor should use the thread context classloader
......................................................................
java: KuduBinaryExtractor should use the thread context classloader
Using the thread context classloader makes it straightforward to write
end-to-end tests, one of which is included in this patch.
This patch makes the following changes:
1. The KuduBinaryExtractor will now search for the test binary jars
via the thread context classloader, if available.
2. Remove the work-in-progress OS-detection implementation from
KuduBinaryExtractor (to be replaced in a future commit) because it
was failing tests.
3. KuduBinaryExtractor will no longer cache its search results to make
it more straightforward to use a thread local context classloader.
4. KuduBinaryExtractor.extractKuduBinary() now throws
FileNotFoundException instead of IllegalStateException when the Kudu
binary test jar cannot be found. Since it is a checked exception and
a subclass of IOException it will be less likely to go uncaught.
5. Update the API docs to be more specific about the semantics of the
public methods of KuduBinaryExtractor, including the use of the
thread context classloader.
6. Add a simple test binary locator test using a child classloader
plumbed into the KuduBinaryExtractor code by setting it as the thread
context classloader.
Change-Id: I5e1cf188bb557eeaea0b2867243855f3f2d121f1
---
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java
M java/kudu-test-utils/src/test/java/org/apache/kudu/test/cluster/TestKuduBinaryJarExtractor.java
2 files changed, 60 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/12147/1
--
To view, visit http://gerrit.cloudera.org:8080/12147
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5e1cf188bb557eeaea0b2867243855f3f2d121f1
Gerrit-Change-Number: 12147
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12147 )
Change subject: java: KuduBinaryExtractor should use the thread context classloader
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12147/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java
File java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java:
http://gerrit.cloudera.org:8080/#/c/12147/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java@54
PS1, Line 54: /** Return the thread context classloader or the parent classloader for this class. */
> It was a nit, because it's not supper important. Generally thats the style
Do we have any documentation on this? I don't want to violate any style guidelines, but if I'm not in violation then I don't see the problem, as long as it's under the line length limit (which it is).
--
To view, visit http://gerrit.cloudera.org:8080/12147
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e1cf188bb557eeaea0b2867243855f3f2d121f1
Gerrit-Change-Number: 12147
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 03 Jan 2019 17:22:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12147 )
Change subject: java: KuduBinaryExtractor should use the thread context classloader
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12147/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java
File java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java:
http://gerrit.cloudera.org:8080/#/c/12147/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java@54
PS1, Line 54: /** Return the thread context classloader or the parent classloader for this class. */
> Do we have any documentation on this? I don't want to violate any style gui
While I can't find the documentation on this, we used to use the Google style guide for Java which explicitly allows this: https://google.github.io/styleguide/javaguide.html#s7.1.1-javadoc-multi-line
I think if we are still using the Google Java style guide then we should add a link to it on https://github.com/cloudera/kudu/blob/master/docs/contributing.adoc#java-code-style or if not then we should specify which Java style guide we are following to avoid disagreements based on a difference of personal preference.
--
To view, visit http://gerrit.cloudera.org:8080/12147
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e1cf188bb557eeaea0b2867243855f3f2d121f1
Gerrit-Change-Number: 12147
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 03 Jan 2019 17:30:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12147 )
Change subject: java: KuduBinaryExtractor should use the thread context classloader
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12147/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java
File java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java:
http://gerrit.cloudera.org:8080/#/c/12147/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java@54
PS1, Line 54: /** Return the thread context classloader or the parent classloader for this class. */
> Why?
It was a nit, because it's not supper important. Generally thats the style used and it just helps with uniformity.
The other 2 single line comments in this file that use the /** style use multiple lines. It's the most common across the board too.
--
To view, visit http://gerrit.cloudera.org:8080/12147
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e1cf188bb557eeaea0b2867243855f3f2d121f1
Gerrit-Change-Number: 12147
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 03 Jan 2019 17:11:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12147 )
Change subject: java: KuduBinaryExtractor should use the thread context classloader
......................................................................
Patch Set 1: Verified+1
Overriding unrelated c++ test flake
--
To view, visit http://gerrit.cloudera.org:8080/12147
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e1cf188bb557eeaea0b2867243855f3f2d121f1
Gerrit-Change-Number: 12147
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 03 Jan 2019 06:19:14 +0000
Gerrit-HasComments: No
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
Posted by "Brian McDevitt (Code Review)" <ge...@cloudera.org>.
Brian McDevitt has posted comments on this change. ( http://gerrit.cloudera.org:8080/12147 )
Change subject: java: KuduBinaryExtractor should use the thread context classloader
......................................................................
Patch Set 1: Code-Review+1
LGTM
--
To view, visit http://gerrit.cloudera.org:8080/12147
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e1cf188bb557eeaea0b2867243855f3f2d121f1
Gerrit-Change-Number: 12147
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 03 Jan 2019 15:21:33 +0000
Gerrit-HasComments: No
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has removed a vote on this change.
Change subject: java: KuduBinaryExtractor should use the thread context classloader
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/12147
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I5e1cf188bb557eeaea0b2867243855f3f2d121f1
Gerrit-Change-Number: 12147
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12147 )
Change subject: java: KuduBinaryExtractor should use the thread context classloader
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12147/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java
File java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java:
http://gerrit.cloudera.org:8080/#/c/12147/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java@54
PS1, Line 54: /** Return the thread context classloader or the parent classloader for this class. */
> Nit: Single line style or break to multiline.
Why?
--
To view, visit http://gerrit.cloudera.org:8080/12147
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e1cf188bb557eeaea0b2867243855f3f2d121f1
Gerrit-Change-Number: 12147
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 03 Jan 2019 17:09:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12147 )
Change subject: java: KuduBinaryExtractor should use the thread context classloader
......................................................................
Patch Set 1: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12147/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java
File java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java:
http://gerrit.cloudera.org:8080/#/c/12147/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java@54
PS1, Line 54: /** Return the thread context classloader or the parent classloader for this class. */
Nit: Single line style or break to multiline.
http://gerrit.cloudera.org:8080/#/c/12147/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java@128
PS1, Line 128: return extractJar(Paths.get(kuduBinDir.toURI()), target, prefix).toString();
Do we need to handle if the target already exists?
--
To view, visit http://gerrit.cloudera.org:8080/12147
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e1cf188bb557eeaea0b2867243855f3f2d121f1
Gerrit-Change-Number: 12147
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 03 Jan 2019 15:54:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12147 )
Change subject: java: KuduBinaryExtractor should use the thread context classloader
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12147/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java
File java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java:
http://gerrit.cloudera.org:8080/#/c/12147/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java@54
PS1, Line 54: /** Return the thread context classloader or the parent classloader for this class. */
> Ideally we would use an auto formatter to remove any need for nits like thi
OK, I'm going to merge this as-is. I'll start a thread on dev@ about the Java style guidelines since I also prefer to avoid these types of discussions. :D
--
To view, visit http://gerrit.cloudera.org:8080/12147
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e1cf188bb557eeaea0b2867243855f3f2d121f1
Gerrit-Change-Number: 12147
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 03 Jan 2019 17:40:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12147 )
Change subject: java: KuduBinaryExtractor should use the thread context classloader
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12147/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java
File java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java:
http://gerrit.cloudera.org:8080/#/c/12147/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java@128
PS1, Line 128: return extractJar(Paths.get(kuduBinDir.toURI()), target, prefix).toString();
> Do we need to handle if the target already exists?
I think it's reasonable to assume that when someone gives us a destination to write to that they own it, so my initial reaction is no, assuming we go back and implement a cleanup code path (which is still TODO).
--
To view, visit http://gerrit.cloudera.org:8080/12147
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e1cf188bb557eeaea0b2867243855f3f2d121f1
Gerrit-Change-Number: 12147
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 03 Jan 2019 17:09:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12147 )
Change subject: java: KuduBinaryExtractor should use the thread context classloader
......................................................................
java: KuduBinaryExtractor should use the thread context classloader
Using the thread context classloader makes it straightforward to write
end-to-end tests, one of which is included in this patch.
This patch makes the following changes:
1. The KuduBinaryExtractor will now search for the test binary jars
via the thread context classloader, if available.
2. Remove the work-in-progress OS-detection implementation from
KuduBinaryExtractor (to be replaced in a future commit) because it
was failing tests.
3. KuduBinaryExtractor will no longer cache its search results to make
it more straightforward to use a thread local context classloader.
4. KuduBinaryExtractor.extractKuduBinary() now throws
FileNotFoundException instead of IllegalStateException when the Kudu
binary test jar cannot be found. Since it is a checked exception and
a subclass of IOException it will be less likely to go uncaught.
5. Update the API docs to be more specific about the semantics of the
public methods of KuduBinaryExtractor, including the use of the
thread context classloader.
6. Add a simple test binary locator test using a child classloader
plumbed into the KuduBinaryExtractor code by setting it as the thread
context classloader.
Change-Id: I5e1cf188bb557eeaea0b2867243855f3f2d121f1
Reviewed-on: http://gerrit.cloudera.org:8080/12147
Tested-by: Mike Percy <mp...@apache.org>
Reviewed-by: Brian McDevitt <br...@phdata.io>
Reviewed-by: Grant Henke <gr...@apache.org>
---
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java
M java/kudu-test-utils/src/test/java/org/apache/kudu/test/cluster/TestKuduBinaryJarExtractor.java
2 files changed, 60 insertions(+), 20 deletions(-)
Approvals:
Mike Percy: Verified
Brian McDevitt: Looks good to me, but someone else must approve
Grant Henke: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/12147
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I5e1cf188bb557eeaea0b2867243855f3f2d121f1
Gerrit-Change-Number: 12147
Gerrit-PatchSet: 2
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
Posted by "Brian McDevitt (Code Review)" <ge...@cloudera.org>.
Brian McDevitt has posted comments on this change. ( http://gerrit.cloudera.org:8080/12147 )
Change subject: java: KuduBinaryExtractor should use the thread context classloader
......................................................................
Patch Set 1:
I think this will work nicely, we'll need to make sure the test still passes on both Linux and MacOS after the detection is added. Possibly need to enhance the 'createKuduBinaryJar' test util.
--
To view, visit http://gerrit.cloudera.org:8080/12147
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e1cf188bb557eeaea0b2867243855f3f2d121f1
Gerrit-Change-Number: 12147
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 03 Jan 2019 05:44:43 +0000
Gerrit-HasComments: No
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12147 )
Change subject: java: KuduBinaryExtractor should use the thread context classloader
......................................................................
Patch Set 1:
> We'll need to make sure the test still passes on both Linux and MacOS after the detection is added. Possibly need to enhance the 'createKuduBinaryJar' test util.
Right.
--
To view, visit http://gerrit.cloudera.org:8080/12147
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e1cf188bb557eeaea0b2867243855f3f2d121f1
Gerrit-Change-Number: 12147
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 03 Jan 2019 06:19:53 +0000
Gerrit-HasComments: No
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12147 )
Change subject: java: KuduBinaryExtractor should use the thread context classloader
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12147/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java
File java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java:
http://gerrit.cloudera.org:8080/#/c/12147/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java@54
PS1, Line 54: /** Return the thread context classloader or the parent classloader for this class. */
> While I can't find the documentation on this, we used to use the Google sty
Ideally we would use an auto formatter to remove any need for nits like this from needing to be reviewed frankly. I am not sure, the formal style guide rule on it. It's likely just personal preference/pattern.
Feel free to merge as is. I intended that to be the case since I added a +2 along with the review.
--
To view, visit http://gerrit.cloudera.org:8080/12147
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e1cf188bb557eeaea0b2867243855f3f2d121f1
Gerrit-Change-Number: 12147
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 03 Jan 2019 17:33:50 +0000
Gerrit-HasComments: Yes