[SPARK-11818][REPL] Fix ExecutorClassLoader to lookup resources from … #9812
Conversation
…parent class loader * Without patch, some tests of ExecutorClassLoaderSuite fails
|
Jenkins, please test this |
| @@ -55,6 +57,14 @@ class ExecutorClassLoader(conf: SparkConf, classUri: String, parent: ClassLoader | |||
| } | |||
| } | |||
|
|
|||
| override def getResource(name: String): URL = { | |||
| parentLoader.getResource(name) | |||
There was a problem hiding this comment.
@srowen
It doesn't need to check userClassPathFirst since this implementation implies that REPL never provides resources dynamically so there's no need to lookup resource from ExecutorClassLoader itself.
Btw, could precondition be broken? I couldn't imagine REPL generating resources.
There was a problem hiding this comment.
I'd be surprised if the repl generated resources, but at the same time, what if someone tries to load the generated class file as a resource? It's an unusual but valid use case.
The code to support that is not complicated; it already exists in MutableURLClassLoader.scala (class ChildFirstURLClassLoader for, example). If you're up for it you could even create a helper trait or a utility method somewhere for that.
There was a problem hiding this comment.
@vanzin
I didn't see use case you mentioned but it could make sense.
In order to achieve, we have to implement findResource() and findResources() for ExecutorClassLoader since ExecutorClassLoader cannot rely on superclass (ClassLoader) to find resource.
It is easy to provide resource URL which points to origin scheme (http, https, ftp, hdfs), but since I'm new to classloader, so I'm wondering it is safe to return URL from findResource() and findResources() which doesn't point to local file.
If it is not safe to return non local file as URL, what's recommended way to do?
I can only think about downloading files to local temp directory per every call.
There was a problem hiding this comment.
I'm wondering it is safe to return URL from findResource() and findResources() which doesn't point to local file
That should be perfectly fine. That's how URLClassLoader works, after all. The only potential odd thing would be getResourceAsStream, which returns an InputStream, and my guess is if the JDK's URL class doesn't support the protocol, would cause an error unless you overrode it here.
So perhaps it's too much to worry about and we can just assume that no one will do that, and fix it if someone ever needs that feature.
There was a problem hiding this comment.
@vanzin
To clarify about "feature", do you want me to change implementation of findResource() and findResources() for pointing origin scheme, and forget about potential odd? Or forget about finding resources from REPL uri and leave as this PR is?
There was a problem hiding this comment.
Leaving the PR as is should be fine for now.
There was a problem hiding this comment.
@vanzin
OK, Thanks for clarification! :)
There was a problem hiding this comment.
That all seams reasonable to me (sorry for the slow reply I'm in Tokyo right now).
|
ok to test |
|
Test build #46272 has finished for PR 9812 at commit
|
| @@ -54,6 +60,8 @@ class ExecutorClassLoaderSuite | |||
| url1 = "file://" + tempDir1 | |||
| urls2 = List(tempDir2.toURI.toURL).toArray | |||
| childClassNames.foreach(TestUtils.createCompiledClass(_, tempDir1, "1")) | |||
| parentResourceNames.foreach(x => | |||
|
@vanzin Thanks for reviewing, I addressed your comment. Please take a look again. |
|
Test build #46410 has finished for PR 9812 at commit
|
|
Failed tests seems not related. |
|
yeah, pyspark tests are super flaky lately. but it never hurts: retest this please |
|
Test build #46427 has finished for PR 9812 at commit
|
|
@vanzin are you OK with this one? |
|
Ah, yeah, fell through the cracks. Merging to master / 1.6. |
…parent class loader Without patch, two additional tests of ExecutorClassLoaderSuite fails. - "resource from parent" - "resources from parent" Detailed explanation is here, https://issues.apache.org/jira/browse/SPARK-11818?focusedCommentId=15011202&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15011202 Author: Jungtaek Lim <kabhwan@gmail.com> Closes #9812 from HeartSaVioR/SPARK-11818. (cherry picked from commit be9dd15) Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>
…parent class loader
Without patch, two additional tests of ExecutorClassLoaderSuite fails.
Detailed explanation is here, https://issues.apache.org/jira/browse/SPARK-11818?focusedCommentId=15011202&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15011202