Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BEAM-2530] Make classloader handling more compatible with JDK 9 #4497

Closed
wants to merge 1 commit into from
+7 −0
Closed

[BEAM-2530] Make classloader handling more compatible with JDK 9 #4497

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ...ction-java/src/main/java/org/apache/beam/runners/core/construction/PipelineResources.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
*/
package org.apache.beam.runners.core.construction;

import com.google.common.base.Splitter;
import com.google.common.base.StandardSystemProperty;
import java.io.File;
import java.net.URISyntaxException;
import java.net.URL;
Expand All @@ -37,6 +39,11 @@ public class PipelineResources {
* @return A list of absolute paths to the resources the class loader uses.
*/
public static List<String> detectClassPathResourcesToStage(ClassLoader classLoader) {
if (classLoader == ClassLoader.getSystemClassLoader()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this logic https://github.com/apache/geronimo-xbean/blob/trunk/xbean-finder/src/main/java/org/apache/xbean/finder/ClassLoaders.java#L38 can be beneficial. High level idea is to use getResources() to determine what is in the classloader.

Copy link
Member

@lukecwik lukecwik Jan 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you find in practice that all Java jars have a META-INF folder?

Also, it doesn't seem that getResources guarantees to return the same order of jars as specified on the classpath.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That isn't guaranteed to work for all jars - jar M will create an archive that doesn't contain a META-INF folder.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why it is not the only strategy but likely the most portable i saw accross years. In particular in the context of this method. Do you expect all jars to be in the cp string? Not using manifest to load jars ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither approach is going to work in all cases, but I've had more success with this one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test this class? getResources("") - not only metainf -
works on most jvm and supports all cases compared to classpath scanning which doesnt support a lot of bootstraps except plain mains :(.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested this logic, but I haven't been able to test this specific usage of it. I ran into a similar problem I'm fixing in GoogleCloudPlatform/DataflowJavaSDK#621 and thought it was worth fixing here too.

If you'd like to defer until we're sure it's a problem, or implement the getResources approach instead, that's fine with me :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this approach seems to solve a legitimate issue and other approaches can be implemented and added.

If you have a good idea on how to test this effectively since its easy to break this kind of functionality without knowing it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to share the idea here what i had in mind high level #4514

return Splitter.on(File.pathSeparatorChar)
.splitToList(StandardSystemProperty.JAVA_CLASS_PATH.value());
}

if (!(classLoader instanceof URLClassLoader)) {
String message = String.format("Unable to use ClassLoader to detect classpath elements. "
+ "Current ClassLoader is %s, only URLClassLoaders are supported.", classLoader);
Expand Down