[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
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 Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 03 Jan 2019 17:40:14 + Gerrit-HasComments: Yes
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
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 Reviewed-by: Brian McDevitt Reviewed-by: Grant Henke --- 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 Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
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 Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 03 Jan 2019 17:33:50 + Gerrit-HasComments: Yes
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
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 Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 03 Jan 2019 17:30:26 + Gerrit-HasComments: Yes
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
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 Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 03 Jan 2019 17:22:44 + Gerrit-HasComments: Yes
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
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 Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 03 Jan 2019 17:09:04 + Gerrit-HasComments: Yes
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
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 Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 03 Jan 2019 17:11:29 + Gerrit-HasComments: Yes
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
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 Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 03 Jan 2019 17:09:25 + Gerrit-HasComments: Yes
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
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 Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 03 Jan 2019 15:54:44 + Gerrit-HasComments: Yes
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
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 Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 03 Jan 2019 15:21:33 + Gerrit-HasComments: No
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
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 Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 03 Jan 2019 06:19:53 + Gerrit-HasComments: No
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
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 Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 03 Jan 2019 06:19:14 + Gerrit-HasComments: No
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
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 Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR] java: KuduBinaryExtractor should use the thread context classloader
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 Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 03 Jan 2019 05:44:43 + Gerrit-HasComments: No
[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