[FLINK-1789] [core] [runtime] [java-api] Allow adding of URLs to the usercode class loader #593

twalthr · 2015-04-13T15:28:37Z

I was a little bit unsure in JobWithJars because it is not guaranteed that the classpath is correct locally, maybe someone can have a look at it.

rmetzger · 2015-04-13T18:24:40Z

I didn't spot anything. But, the travis build didn't pass ;)

twalthr · 2015-04-14T12:49:30Z

It builds now ;)

aalexandrov · 2015-04-14T13:35:25Z

👏

aljoscha · 2015-04-23T09:09:38Z

This does not work if the user uses classes that are not available on the local machine since you don't add the additional class path entries in JobWithJars.buildUserCodeClassLoader(). Correct?

aalexandrov · 2015-04-23T11:54:30Z

@aljoscha I'm sorry, I cannot follow? Can you elaborate?

The idea is to add support for proper folders next to jars when opening an execution environment. Since folders cannot be handled the same way as jars during execution (i.e., serialize and ship them around by the blob manager), the assumption for folder paths is that they are accessible from all agents in the distributed runtime (JobManager + TaskManagers), e.g., via shared NFS folders.

aljoscha · 2015-04-23T12:14:54Z

Yes, this is true, but the way it is implemented, the folders are not always added to the class loader. Maybe I'm wrong here, but JobWithJars.getUserCodeClassLoader and JobWithJars.buildUserCodeClassLoader don't add the URLs to the ClassLoader that they create.

twalthr · 2015-04-23T13:47:34Z

@aljoscha Yes, this is what I also mentioned in my PR description. If we are using a RemoteEnvironment, it is unlikely that the classpath that is available on the cluster is also valid on the local machine. The user has to add the libraries to his local project manually.

aljoscha · 2015-04-24T08:09:50Z

OK, @StephanEwen, any thoughts on this? Should we allow that the local user code class loader in the client potentially doesn't have the same jars available as the workers?

StephanEwen · 2015-04-24T09:28:00Z

I think that keeping the classpaths in sync is really crucial. What is the problem in exposing this to the client as well?

Can we make the following assumptions for the use case where we need the global class path:

The URL is either a file path that points to a directory accessible to all nodes (NFS or so) and the client runs in the cluster as well.
The URL is an HTTP URL or so that points to a file server that serves the classes to work in non-shared directory settings.

Some other remark: This makes the code quite dirty on a lot of parts, since we have always Files and URLs in Client, executors, ... Can we not store everything as URLs (rather than having Strings for jar files and URLs extra). Files can be expressed as URLs anyways as well...

aljoscha · 2015-04-27T07:32:09Z

Yes, this would make things a lot cleaner. @twalthr what do you think?

twalthr · 2015-04-27T10:05:00Z

It is not a problem in exposing this to the client as well, I was just unsure if it is necessary. I will add to the client as well to be in sync.
Yes, it makes sense to use URLs instead of URLs and Files. But the user facing APIs remain Strings right?

StephanEwen · 2015-04-27T10:28:26Z

Yes, user-facing remains strings which are file paths.

twalthr · 2015-05-18T13:52:26Z

I have implemented your comments and rebased to the current master. The build succeeded except for a KafkaITCase, but I think that has nothing to do with my code.

rmetzger · 2015-05-18T13:57:09Z

Oh .. failing KafkaITCases are something I'm very interested in ;)
Sadly I can not see the root cause of the issue :( But the issue is unrelated to your change.

twalthr · 2015-06-03T23:18:03Z

Any comments for this PR?

StephanEwen · 2015-06-04T10:56:51Z

I was originally thinking to merge this after #681 , but I may have to merge this by itself to get it into the release.

twalthr · 2015-07-27T22:06:01Z

I have rebased the PR and added commits of @aalexandrov that introduce a CLI option --classpath.

aalexandrov · 2015-09-18T13:45:05Z

@twalthr @rmetzger is there a chance to include this PR in the 10.0 release?

rmetzger · 2015-09-18T13:53:03Z

Do you mean the 0.10-milestone-1 release? or the final 0.10 release?

Fabian is managing the 0.10-milestone-1 release, as far as I know he has not started with the release activities, so we could still include it, given that we have consensus to merge the PR

aalexandrov · 2015-09-18T14:04:23Z

@rmetzger the final 10.0, this and the Scala 2.11 cross-building support (#885), which is currently handled by @chiwanpark, are the two pending issues that make the current Emma master incompatible with vanilla Flink.

twalthr · 2015-09-21T11:54:12Z

I rebased the PR two times. So before I will rebase it a third time, it would be great if we finally find a consensus.

rmetzger · 2015-09-21T13:46:33Z

I agree. Lets wait until #858 is merged. As far as I understood Stephan, this PR depends on #858.

rmetzger · 2015-09-23T08:21:41Z

#858 is merged, so we can try to get consensus here

rmetzger · 2015-09-23T08:23:43Z

flink-clients/src/main/java/org/apache/flink/client/cli/CliFrontendParser.java

@@ -47,6 +47,11 @@
 			"Class with the program entry point (\"main\" method or \"getPlan()\" method. Only needed if the " +
 					"JAR file does not specify the class in its manifest.");

+	static final Option CLASSPATH_OPTION = new Option("C", "classpath", true, "Adds a URL to each user code classloader " +


would hdfs:/// also be possible?

HDFS is not possible yet. the user would have to provide a special URL protocol handler via java -Djava.protocol.handler.pkgs=org.my.protocols such that the URLClassloader understands the protocol.

Is this documented somewhere?

@tillrohrmann I added a comment that only protocols are supported which are supported by the URLClassLoader. The parameter above is Java standard.

twalthr · 2015-10-06T16:13:23Z

Conflicts solved again ;)

mxm · 2015-10-06T18:32:36Z

Thanks a lot @twalthr for rebasing the pull request. There is an issue with the Scalashell test. Not sure why it only occurs in some builds.

aalexandrov · 2015-10-06T19:38:28Z

@mxm The issue is probably related to Scala 2.10, since the two passing builds have Dscala-2.11. I'll investigate tonight if I can get a hold on the PR commits (GitHub is somewhat slow at the moment)

twalthr · 2015-10-06T20:09:58Z

I think the NullPointerException was due to a wrong array to list conversion. I don't know why this is related to the Scala version.

aalexandrov · 2015-10-06T20:12:21Z

@twalthr It could be that the test is not executed for Scala 2.11.

twalthr · 2015-10-07T08:49:18Z

The build succeeded. How do we proceed? It would be very nice if we finally merge this.

mxm · 2015-10-07T10:56:46Z

...e/src/main/java/org/apache/flink/runtime/execution/librarycache/BlobLibraryCacheManager.java

-
+
+				// add classpaths
+				URL[] cpUrls = requiredClasspaths.toArray(new URL[requiredClasspaths.size()]);


Any reason you're creating an array here first instead of simply iterating over requiredClassPaths?

You're right. It is completely unnecessary. I will correct that.

mxm · 2015-10-07T11:20:26Z

I went through the pull request again and it looks good.

+1 for merging

twalthr · 2015-10-07T13:12:44Z

Do we need another committers +1? It touches very sensitive parts...

mxm · 2015-10-07T13:49:41Z

I think @StephanEwen wanted to merge it as well. @rmetzger concerns should be out of the way with the new version. I would merge this by the end of the day.

StephanEwen · 2015-10-07T14:10:07Z

Looks good to merge from my side.

The only uncertainty remaining is whether we need the static factory methods on the ExecutionEnvironment. Adding these global classpaths seems a very specific case, in which we can probably expect people to manually create a RemoteEnvironment. That way we don't over load the ExecutionEnvironment with too many factory methods (we have a tendency to get API bloat).

mxm · 2015-10-08T08:40:47Z

flink-java/src/main/java/org/apache/flink/api/java/RemoteEnvironment.java

-	 *                 user-defined functions, user-defined input formats, or any libraries, those must be
-	 *                 provided in the JAR files.
-	 */	
-	public RemoteEnvironment(String host, int port, String... jarFiles) {


Could we keep this constructor?

mxm · 2015-10-08T08:43:32Z

+1 for removing the factory methods from ExecutionEnvironment. Most users won't even think about using this feature and be rather confused by the additional methods. Let's also keep the old constructor of the RemoteEnvironment in addition to the new one.

mxm · 2015-10-08T10:09:39Z

@twalthr Could you make the changes and update the PR? Thank you.

…usercode class loader

twalthr · 2015-10-08T10:35:39Z

@mxm done ;)

mxm · 2015-10-08T11:28:24Z

@twalthr Doesn't seem to compile :)

mxm · 2015-10-08T13:59:04Z

Merged. Congrats :)

twalthr · 2015-10-08T14:03:45Z

Thanks, @mxm ;)

…s loader This closes apache#593.

twalthr force-pushed the CustomUrlsToUserClassloader branch from cbbb554 to 90cf0db Compare April 14, 2015 11:01

twalthr force-pushed the CustomUrlsToUserClassloader branch from 90cf0db to ede7cb2 Compare May 18, 2015 12:48

aalexandrov mentioned this pull request Jul 7, 2015

Realease an Emma-compatible Flink fork emmalanguage/emma#46

Closed

8 tasks

twalthr force-pushed the CustomUrlsToUserClassloader branch 3 times, most recently from 7b7a1ad to 2230808 Compare July 27, 2015 15:47

rmetzger reviewed Sep 23, 2015
View reviewed changes

mxm reviewed Oct 7, 2015
View reviewed changes

mxm reviewed Oct 8, 2015
View reviewed changes

twalthr added 3 commits October 8, 2015 12:11

[FLINK-1789] [core] [runtime] [java-api] Allow adding of URLs to the …

d4cd272

…usercode class loader

Array creation fix

14bdc0e

Code clean up

cb0234f

twalthr force-pushed the CustomUrlsToUserClassloader branch from 33565ae to 120df65 Compare October 8, 2015 10:33

Make it a more internal feature

9c18e3e

twalthr force-pushed the CustomUrlsToUserClassloader branch from 120df65 to 9c18e3e Compare October 8, 2015 11:40

asfgit closed this in 0ee0c1f Oct 8, 2015

lofifnc pushed a commit to lofifnc/flink that referenced this pull request Oct 8, 2015

[FLINK-1789][core][runtime] Allow adding of URLs to the usercode clas…

ca473ff

…s loader This closes apache#593.

cfmcgrady pushed a commit to cfmcgrady/flink that referenced this pull request Oct 23, 2015

[FLINK-1789][core][runtime] Allow adding of URLs to the usercode clas…

e00b9b2

…s loader This closes apache#593.

rmetzger added the component=Runtime/Coordination label Mar 14, 2019

pnowojski pushed a commit to pnowojski/flink that referenced this pull request Mar 8, 2024

Trigger CTS on build (apache#593)

72d3014



		// add classpaths
		URL[] cpUrls = requiredClasspaths.toArray(new URL[requiredClasspaths.size()]);

[FLINK-1789] [core] [runtime] [java-api] Allow adding of URLs to the usercode class loader #593

[FLINK-1789] [core] [runtime] [java-api] Allow adding of URLs to the usercode class loader #593

Conversation

twalthr commented Apr 13, 2015

rmetzger commented Apr 13, 2015

twalthr commented Apr 14, 2015

aalexandrov commented Apr 14, 2015

aljoscha commented Apr 23, 2015

aalexandrov commented Apr 23, 2015

aljoscha commented Apr 23, 2015

twalthr commented Apr 23, 2015

aljoscha commented Apr 24, 2015

StephanEwen commented Apr 24, 2015

aljoscha commented Apr 27, 2015

twalthr commented Apr 27, 2015

StephanEwen commented Apr 27, 2015

twalthr commented May 18, 2015

rmetzger commented May 18, 2015

twalthr commented Jun 3, 2015

StephanEwen commented Jun 4, 2015

twalthr commented Jul 27, 2015

aalexandrov commented Sep 18, 2015

rmetzger commented Sep 18, 2015

aalexandrov commented Sep 18, 2015

twalthr commented Sep 21, 2015

rmetzger commented Sep 21, 2015

rmetzger commented Sep 23, 2015

rmetzger Sep 23, 2015

Choose a reason for hiding this comment

twalthr Sep 24, 2015

Choose a reason for hiding this comment

tillrohrmann Sep 24, 2015

Choose a reason for hiding this comment

twalthr Sep 30, 2015

Choose a reason for hiding this comment

twalthr commented Oct 6, 2015

mxm commented Oct 6, 2015

aalexandrov commented Oct 6, 2015

twalthr commented Oct 6, 2015

aalexandrov commented Oct 6, 2015

twalthr commented Oct 7, 2015

mxm Oct 7, 2015

Choose a reason for hiding this comment

twalthr Oct 7, 2015

Choose a reason for hiding this comment

mxm commented Oct 7, 2015

twalthr commented Oct 7, 2015

mxm commented Oct 7, 2015

StephanEwen commented Oct 7, 2015

mxm Oct 8, 2015

Choose a reason for hiding this comment

mxm commented Oct 8, 2015

mxm commented Oct 8, 2015

twalthr commented Oct 8, 2015

mxm commented Oct 8, 2015

mxm commented Oct 8, 2015

twalthr commented Oct 8, 2015