HADOOP-18993 Allow to not isolate S3AFileSystem classloader when needed #6301
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
af3b88c to
a694380
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/index.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
production code good, just suggesting lots of changes to the tests, which is expected as you are being introduced to a new test hierarchy.
Actually, your test setup doesn't actually need to call initialize(), does it -so making it a unit test is viable. My review assumes that we are instantiating it, and so it needs to be recategorised. real unit Test cases run under yetus (no need for secrets) and are better as long as they don't do horrible things with mockito to work
If this can be done in a unit test, it should extend AbstractHadoopTestBase and still use AssertJ for assertions, but you can leave the rest as is.
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/index.md
Outdated
Show resolved
Hide resolved
.../hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AFileSystemIsolatedClassloader.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
Outdated
Show resolved
Hide resolved
.../hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AFileSystemIsolatedClassloader.java
Outdated
Show resolved
Hide resolved
.../hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AFileSystemIsolatedClassloader.java
Outdated
Show resolved
Hide resolved
| @Test | ||
| public void isolatedClasspath() throws IOException { | ||
| try (S3AFileSystem fs = new S3AFileSystem()) { | ||
| Configuration conf = new Configuration(); |
There was a problem hiding this comment.
use getConfiguration() of and you'll get the set up config from the superclass
There was a problem hiding this comment.
I actually could not use getConfiguration() otherwise Configuration object would've been initialised in a way that made my tests fail, I hope the way I did it is still good enough
.../hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AFileSystemIsolatedClassloader.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
Thanks for the review @steveloughran ! |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Hi @steveloughran , can you have a look at the PR? |
steveloughran
requested changes
Dec 2, 2023
There was a problem hiding this comment.
some comments on the javadocs, more on testing.
- Merge all your commits down to a single patch, force push it, and yetus won't complain any more
- do read the testing.md doc in the hadoop-aws site docs *and say which s3 endpoint you've run the entire hadoop-aws test suite against. yetus doesn't have the credentials to do this itself, and while I will inevitably rerun it once merged, you get to do the work first and, to validate that diligence, say where.
Everyone has to do this: even (especially) me: #6259
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java
Outdated
Show resolved
Hide resolved
...hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileSystemIsolatedClassloader.java
Outdated
Show resolved
Hide resolved
...hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileSystemIsolatedClassloader.java
Outdated
Show resolved
Hide resolved
...hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileSystemIsolatedClassloader.java
Outdated
Show resolved
Hide resolved
|
any update on this? |
|
Hi @steveloughran, I found some issues on running integration tests against my s3 bucket, I needed to tweak some confs and still getting an unknown host exception. I will spend another couple of hours on this today probably and let you know :) |
fb821c1 to
dd8d780
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
So finally I go the final "push" on this. Maybe the last thing that I might do to improve this is to make it a unit test instead of an integration one, but given your comment I guess given the fact that instantiates and fs it must be an IT test and I can't think a way of testing this without instantiating the fs. |
dd8d780 to
2a77456
Compare
|
🎊 +1 overall
This message was automatically generated. |
2a77456 to
7daaeef
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
Hi @steveloughran will you have any time to review this soon? |
|
@tmnd1991 if you saw my backlog of reviews and my own todo list, you'd know the answer to that. |
steveloughran
requested changes
Jan 31, 2024
| S3AFileSystem fs = new S3AFileSystem(); | ||
| fs.initialize(getFileSystem().getUri(), conf); | ||
| return fs; | ||
| } catch (IOException e) { |
There was a problem hiding this comment.
just have it throw the exception all the way up; no need to wrap
| return new HashMap<>(); | ||
| } | ||
|
|
||
| private Map<String, String> mapOf(String key, String value) { |
| } | ||
| } | ||
|
|
||
| private void test(Map<String, String> confToSet, Consumer<FileSystem> asserts) |
There was a problem hiding this comment.
- can you add a javadoc to explain what is happening
- can you give it a slightly more explicit name, e.g assertInNewFilesystem()
| import org.apache.hadoop.conf.Configuration; | ||
| import org.apache.hadoop.fs.FileSystem; | ||
|
|
||
| public class ITestS3AFileSystemIsolatedClassloader extends AbstractS3ATestBase { |
There was a problem hiding this comment.
nit: javadocs to explian what it does
|
|
||
| public class ITestS3AFileSystemIsolatedClassloader extends AbstractS3ATestBase { | ||
|
|
||
| public static class CustomClassLoader extends ClassLoader { |
| * @param conf configuration object. | ||
| * @param classLoader isolated classLoader. | ||
| */ | ||
| private void isolateClassloader(Configuration conf, ClassLoader classLoader) { |
There was a problem hiding this comment.
move into S3AUtils, make static. I have a dream of S3AFs getting smaller -or at least no larger
f9b61a9 to
285ede2
Compare
|
thanks for the review @steveloughran. |
|
🎊 +1 overall
This message was automatically generated. |
|
This the result of verify, looks good to me: |
|
🎊 +1 overall
This message was automatically generated. |
|
ITestS3AClosedFS looks new. I'd have expected the others today (fix in progress) does it work on a test run on hadoop trunk without your pr |
|
|
|
So, I got time to check them. First: when I run it alone, the test passes, both on trunk (2f1718c) and on my branch. Second: when I run all the tests it fails both on trunk and on my branch. Therefore I think I did not introduce the regression 😄 |
steveloughran
requested changes
Feb 3, 2024
| * @param conf configuration object. | ||
| * @param classLoader isolated classLoader. | ||
| */ | ||
| static void isolateClassloader(Configuration conf, ClassLoader classLoader) { |
There was a problem hiding this comment.
let's rename this "maybeIsolateClassloader" to make clear it is conditional.
285ede2 to
f17aed0
Compare
|
Done 👍 |
|
🎊 +1 overall
This message was automatically generated. |
|
I've looked at the closed fs test. It is verifying that threads are all gone so its potentially a sign of some thread leakage. Can you post the stack trace/output you are seeing? I don't see this as the cause, so will merge to trunk/branch-3.4; we can create a new JIRA with the new failure as it may need to be made more resilient as well as maybe improving reporting |
jiajunmao
pushed a commit
to jiajunmao/hadoop-MLEC
that referenced
this pull request
Feb 6, 2024
The option fs.s3a.classloader.isolation (default: true) can be set to false to disable s3a classloader isolation; This can assist in using custom credential providers and other extension points. Contributed by Antonio Murgia
@steveloughran created the jira issue: https://issues.apache.org/jira/browse/HADOOP-19068 |
|
thanks. low priority issue as it'll just be fs leakage somewhere. simplest fix would be to use closeAllForUGI() in setup to destroy any old ones around |
steveloughran
pushed a commit
that referenced
this pull request
Feb 7, 2024
The option fs.s3a.classloader.isolation (default: true) can be set to false to disable s3a classloader isolation; This can assist in using custom credential providers and other extension points. Contributed by Antonio Murgia
slfan1989
pushed a commit
that referenced
this pull request
Feb 10, 2024
The option fs.s3a.classloader.isolation (default: true) can be set to false to disable s3a classloader isolation; This can assist in using custom credential providers and other extension points. Contributed by Antonio Murgia
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.<
17AE
/span>
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
In HADOOP-17372 the S3AFileSystem forces the configuration classloader to be the same as the one that loaded S3AFileSystem. This leads to the impossibility in Spark applications to load third party credentials providers as user jars.
I propose to add a configuration key fs.s3a.extensions.isolated.classloader with a default value of true that if set to false will not perform the classloader set.
How was this patch tested?
Unit tests at
org.apache.hadoop.fs.s3a.TestS3AFileSystemIsolatedClassloader.For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?