portable way to get the files to stage from current classloader #4514
Conversation
…lassloader + ensuring the right classloader is used
| throw new IllegalArgumentException("Unsupported entry: " + url.toExternalForm()); | ||
| } | ||
|
|
||
| private static String decode(String fileName) { |
There was a problem hiding this comment.
Use an implementation that already exists instead of re-implementing this.
There was a problem hiding this comment.
Anyone you are thinking about? Dont think we want xbean here (another shady dep)
There was a problem hiding this comment.
There was a problem hiding this comment.
From memory the rule was not 100% the same but can need to be checked, makes years now
| public final class Classloaders { | ||
| private static final ClassLoader SYSTEM = ClassLoader.getSystemClassLoader(); | ||
|
|
||
| public static Stream<File> toFiles(final ClassLoader classLoader) throws IOException { |
There was a problem hiding this comment.
Lets hide all this logic and make it private within PipelineResources.
| .map(Classloaders::toFile); | ||
| } | ||
|
|
||
| private static boolean isSystemParent(final ClassLoader classLoader) { |
There was a problem hiding this comment.
nit: rename to isAncestorOfSystemClassLoader
|
|
||
| /** | ||
| * Allows to configure how the staging files are selected. | ||
| * TODO: allow to select in parent classloaders as well as current one. |
There was a problem hiding this comment.
Add a comment around the order in which the filters are applied.
| */ | ||
| @Description("Include regex for file staging.") | ||
| String getClassLoaderIncludeFilter(); | ||
| void setClassLoaderIncludeFilter(String value); |
There was a problem hiding this comment.
Should we just accept a list to start since its likely people will want to be able to specify more then one include and exclude instead of writing a giant regex or filter?
| * when staging the classloader. | ||
| * The regex is applied on the file name. | ||
| */ | ||
| @Description("Include regex for file staging.") |
There was a problem hiding this comment.
Be explicit in this description since this appears in the output of --help.
Ditto on the exclude filter.
|
|
||
| /** | ||
| * Allows to configure how the staging files are selected. | ||
| * TODO: allow to select in parent classloaders as well as current one. |
There was a problem hiding this comment.
The TODO should really be in PipelineResources.
|
|
||
| PipelineResources.detectClassPathResourcesToStage(classLoader); | ||
| })) { | ||
| assertEquals(0, PipelineResources.detectClassPathResourcesToStage(null, classLoader).size()); |
There was a problem hiding this comment.
This should continue to throw, we don't want to silently ignore parts of the classpath we don't understand. Would rather make this limitation be explicit for users.
There was a problem hiding this comment.
We dont but this url is invalid and ignored by the classloader. We still fail if the url is not jar or file.
|
I brought this discussion back to the dev@ to choose between PR/4497 or PR/4514 or support both. |
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
follow up PR of #4497 to share the idea in a concrete way
Follow this checklist to help us incorporate your contribution quickly and easily:
[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue.mvn clean verifyto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.