You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by stephenh <gi...@git.apache.org> on 2014/12/17 21:25:01 UTC
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
GitHub user stephenh opened a pull request:
https://github.com/apache/spark/pull/3725
[SPARK-4877] Allow user first classes to extend classes in the parent.
Previously, the classloader isolation was almost too good, such
that if a child class needed to load/reference a class that was
only available in the parent, it could not do so.
This adds tests for that case, the user-first Fake2 class extends
the only-in-parent Fake3 class.
It also sneaks in a fix where only the first stage seemed to work,
and on subsequent stages, a LinkageError happened because classes
from the user-first classpath were getting defined twice.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/stephenh/spark 4877_user_first_parent_inheritance
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/3725.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #3725
----
commit 209d6d39438b3f0560a46220298a380db24210de
Author: Stephen Haberman <st...@exigencecorp.com>
Date: 2014-12-17T20:20:39Z
[SPARK-4877] Allow user first classes to extend classes in the parent.
Previously, the classloader isolation was almost too good, such
that if a child class needed to load/reference a class that was
only available in the parent, it could not do so.
This adds tests for that case, the user-first Fake2 class extends
the only-in-parent Fake3 class.
It also sneaks in a fix where only the first stage seemed to work,
and on subsequent stages, a LinkageError happened because classes
from the user-first classpath were getting defined twice.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-67387606
[Test build #24553 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24553/consoleFull) for PR 3725 at commit [`209d6d3`](https://github.com/apache/spark/commit/209d6d39438b3f0560a46220298a380db24210de).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by stephenh <gi...@git.apache.org>.
Github user stephenh commented on a diff in the pull request:
https://github.com/apache/spark/pull/3725#discussion_r22001164
--- Diff: core/src/main/scala/org/apache/spark/TestUtils.scala ---
@@ -85,15 +95,26 @@ private[spark] object TestUtils {
}
/** Creates a compiled class with the given name. Class file will be placed in destDir. */
- def createCompiledClass(className: String, destDir: File, value: String = ""): File = {
+ def createCompiledClass(
+ className: String,
+ destDir: File,
+ value: String = "",
+ baseClass: String = null,
+ classpathUrls: Seq[URL] = Seq()): File = {
--- End diff --
I added a classpathUrls param so that when we're compiling the child-first classes, they can have the parent classes on the classpath (to model the child-class extends a class available in the parent loader).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by stephenh <gi...@git.apache.org>.
Github user stephenh commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-72731355
@pwendell any chance this could make the 1.3 branch? @holdenk has (I believe) signed off, but doesn't have commit access.
We're currently running a patched version of Spark with this fix and it would be nice to move to a vanilla 1.3 release.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by stephenh <gi...@git.apache.org>.
Github user stephenh commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-67540252
@vanzin that seems likely...I see that PR changes the findClass to loadClass. I'm not entirely sure if that would pass the tests that I added, but it might. Is that PR being accepted soon-ish? If so/when that happens, I can try that and abandon this PR if that takes care of it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/3725
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-71389367
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26066/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by stephenh <gi...@git.apache.org>.
Github user stephenh commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-72874323
@JoshRosen thanks; rebased, it was just some import conflicts.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-71385608
[Test build #26066 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/26066/consoleFull) for PR 3725 at commit [`b2c5bf8`](https://github.com/apache/spark/commit/b2c5bf89fe7a4a7a2f21e56827e01d8854e5d455).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by stephenh <gi...@git.apache.org>.
Github user stephenh commented on a diff in the pull request:
https://github.com/apache/spark/pull/3725#discussion_r22001258
--- Diff: core/src/main/scala/org/apache/spark/executor/ExecutorURLClassLoader.scala ---
@@ -39,7 +39,17 @@ private[spark] class ChildExecutorURLClassLoader(urls: Array[URL], parent: Class
super.addURL(url)
}
override def findClass(name: String): Class[_] = {
- super.findClass(name)
+ val loaded = super.findLoadedClass(name)
+ if (loaded != null) {
+ return loaded
--- End diff --
This loaded check solves the LinkageError that the non-first stages would have happen, and is based on a comment in this SO post:
http://stackoverflow.com/questions/5445511/how-do-i-create-a-parent-last-child-first-classloader-in-java-or-how-to-overr
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by stephenh <gi...@git.apache.org>.
Github user stephenh commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-70326953
Ping?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-67526925
BTW I'm pretty sure I addressed this as part of #3233, although in a different way.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-72738765
If you fix the merge conflicts, I'll commit this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-72888837
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26755/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by stephenh <gi...@git.apache.org>.
Github user stephenh commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-68025292
Hey Josh, thanks! (262 open PRs...wow... :-))
Per the branches, I checked branch-1.0, branch-1.1, and branch-1.2, and all of them have the spark.files.userClassPathFirst config option, but with the as-in-master code that doesn't do child -> parent fallback. Not sure how many releases you're backporting too...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by holdenk <gi...@git.apache.org>.
Github user holdenk commented on a diff in the pull request:
https://github.com/apache/spark/pull/3725#discussion_r22125536
--- Diff: core/src/main/scala/org/apache/spark/executor/ExecutorURLClassLoader.scala ---
@@ -39,7 +39,17 @@ private[spark] class ChildExecutorURLClassLoader(urls: Array[URL], parent: Class
super.addURL(url)
}
override def findClass(name: String): Class[_] = {
- super.findClass(name)
+ val loaded = super.findLoadedClass(name)
+ if (loaded != null) {
+ return loaded
--- End diff --
Interesting, thanks for fixing this :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by stephenh <gi...@git.apache.org>.
Github user stephenh commented on a diff in the pull request:
https://github.com/apache/spark/pull/3725#discussion_r22001106
--- Diff: core/src/main/scala/org/apache/spark/TestUtils.scala ---
@@ -43,9 +43,19 @@ private[spark] object TestUtils {
* Note: if this is used during class loader tests, class names should be unique
* in order to avoid interference between tests.
*/
- def createJarWithClasses(classNames: Seq[String], value: String = ""): URL = {
+ def createJarWithClasses(
+ classNames: Seq[String],
+ value: String = "",
+ classpathUrls: Seq[URL] = Seq()): URL = {
val tempDir = Utils.createTempDir()
- val files = for (name <- classNames) yield createCompiledClass(name, tempDir, value)
+ val files = for (name <- classNames) yield {
+ if (name.indexOf(" ") == -1) {
--- End diff --
This is kind of janky, but I encoded "I want this ChildClass to extend BaseClass" as passing in the string "ChildClass BaseClass".
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by stephenh <gi...@git.apache.org>.
Github user stephenh commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-68020315
This patch has been reviewed by two people; should I cc someone else to get it approved + committed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-67540967
Well, no one has actually reviewed that PR yet, and since it's kinda large it might take a while. So it's probably OK to get this in in the meantime.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-72874588
[Test build #26755 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/26755/consoleFull) for PR 3725 at commit [`dabcd35`](https://github.com/apache/spark/commit/dabcd35e130141b5548095c73def6dd80af996b0).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by holdenk <gi...@git.apache.org>.
Github user holdenk commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-71386034
@JoshRosen I don't have commit privileges so someone else will need to commit this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by stephenh <gi...@git.apache.org>.
Github user stephenh commented on a diff in the pull request:
https://github.com/apache/spark/pull/3725#discussion_r22126116
--- Diff: core/src/test/scala/org/apache/spark/executor/ExecutorURLClassLoaderSuite.scala ---
@@ -37,6 +37,8 @@ class ExecutorURLClassLoaderSuite extends FunSuite {
val fakeClass = classLoader.loadClass("FakeClass2").newInstance()
val fakeClassVersion = fakeClass.toString
assert(fakeClassVersion === "1")
+ val fakeClass2 = classLoader.loadClass("FakeClass2").newInstance()
--- End diff --
I saw the LinkageError running a job; it went through the first stage just fine, but blew up on the 2nd stage; the error was:
java.lang.LinkageError (loader (instance of org/apache/spark/executor/ChildExecutorURLClassLoader$userClassLoader$): attempted duplicate class definition for name: "com/...one of our classes...") [duplicate 35]
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by stephenh <gi...@git.apache.org>.
Github user stephenh commented on a diff in the pull request:
https://github.com/apache/spark/pull/3725#discussion_r22001361
--- Diff: core/src/test/scala/org/apache/spark/executor/ExecutorURLClassLoaderSuite.scala ---
@@ -37,6 +37,8 @@ class ExecutorURLClassLoaderSuite extends FunSuite {
val fakeClass = classLoader.loadClass("FakeClass2").newInstance()
val fakeClassVersion = fakeClass.toString
assert(fakeClassVersion === "1")
+ val fakeClass2 = classLoader.loadClass("FakeClass2").newInstance()
--- End diff --
This 2nd loadClass would cause the LinkageError.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-70392673
@holdenk @pwendell Can one of you review this, sign off, and commit? I don't really have enough expertise here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-67674679
[Test build #24651 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24651/consoleFull) for PR 3725 at commit [`bb6dc46`](https://github.com/apache/spark/commit/bb6dc4636a981eb62b0f110e9492bde08f45858d).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* ` "public class " + className + extendsText + " implements java.io.Serializable `
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by holdenk <gi...@git.apache.org>.
Github user holdenk commented on a diff in the pull request:
https://github.com/apache/spark/pull/3725#discussion_r22125664
--- Diff: core/src/test/scala/org/apache/spark/executor/ExecutorURLClassLoaderSuite.scala ---
@@ -37,6 +37,8 @@ class ExecutorURLClassLoaderSuite extends FunSuite {
val fakeClass = classLoader.loadClass("FakeClass2").newInstance()
val fakeClassVersion = fakeClass.toString
assert(fakeClassVersion === "1")
+ val fakeClass2 = classLoader.loadClass("FakeClass2").newInstance()
--- End diff --
Good catch :) Out of interest, did someone observe this in production or was the classloader protected from this unsafe access pattern through how set the classloader?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-67405754
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24553/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-68029024
Ah, gotcha; this sounds like it fixes a fairly serious bug, so I'll try to cherry-pick this into our backport branches. I'll take a pass through this tomorrow when I'm not tired.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by stephenh <gi...@git.apache.org>.
Github user stephenh commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-68071870
Cool, sounds good. FWIW there are few things to do after this gets in:
a) document that if userClassPathFirst=true, then user's uberjar should not include any Spark or Scala code (or else they'll get class cast exceptions b/c the parent scala.Function will be different from the child scala.Function),
b) either accept Marcelo's PR as-is (which, among other things, applies the user-first classloader to driver code) or pull out just the driver part of his PR until the rest gets in (I've done this for our local Spark build),
c) as a few others have said, adapt the filtering logic from Jetty/Hadoop that will prefer scala.* and org.apache.spark.* (and a few others) from the parent classloader all the time, even if the user's uberjar does accidentally include them (at this point, the documentation added in a) could be removed).
I included these in order of small -> large, with the idea that, unless someone beats me to it (which would be great :-)), I'll progressively work through each one.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by stephenh <gi...@git.apache.org>.
Github user stephenh commented on a diff in the pull request:
https://github.com/apache/spark/pull/3725#discussion_r22001331
--- Diff: core/src/main/scala/org/apache/spark/executor/ExecutorURLClassLoader.scala ---
@@ -39,7 +39,17 @@ private[spark] class ChildExecutorURLClassLoader(urls: Array[URL], parent: Class
super.addURL(url)
}
override def findClass(name: String): Class[_] = {
- super.findClass(name)
+ val loaded = super.findLoadedClass(name)
+ if (loaded != null) {
+ return loaded
+ }
+ try {
+ super.findClass(name)
+ } catch {
+ case e: ClassNotFoundException => {
+ parentClassLoader.loadClass(name)
--- End diff --
This CNFE/call parent is based on the same SO post, which AFAICT might have even been the inspiration for the original implementation here (the SO post was linked in one of the original user-first tickets), but this detail got dropped, I assume accidentally.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-67663434
[Test build #24651 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24651/consoleFull) for PR 3725 at commit [`bb6dc46`](https://github.com/apache/spark/commit/bb6dc4636a981eb62b0f110e9492bde08f45858d).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-68022069
Hey @stephenh,
I'd be glad to take a pass through this to try to get it merged (I've been spending today trying to make some progress through our [PR review backlog](https://spark-prs.appspot.com); we're now down to 262 open PRs). I'm not super-familiar with this code, so I'll defer to @holdenk's review here. Give me a little bit to look over this to make sure that I understand the rough gist of the change, then I'll merge it.
Quick question: which branches should this go into? Does this only impact 1.2?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by stephenh <gi...@git.apache.org>.
Github user stephenh commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-67594886
@holdenk can you review this PR? I see that you wrote most of the original ChildExecutorURLClassLoader.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-67405745
[Test build #24553 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24553/consoleFull) for PR 3725 at commit [`209d6d3`](https://github.com/apache/spark/commit/209d6d39438b3f0560a46220298a380db24210de).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* ` "public class " + className + extendsText + " implements java.io.Serializable `
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by stephenh <gi...@git.apache.org>.
Github user stephenh commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-72731411
Oh, I see it has merge conflicts now...I'll work on rebasing it...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-71389365
[Test build #26066 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/26066/consoleFull) for PR 3725 at commit [`b2c5bf8`](https://github.com/apache/spark/commit/b2c5bf89fe7a4a7a2f21e56827e01d8854e5d455).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* ` "public class " + className + extendsText + " implements java.io.Serializable `
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-72888826
[Test build #26755 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/26755/consoleFull) for PR 3725 at commit [`dabcd35`](https://github.com/apache/spark/commit/dabcd35e130141b5548095c73def6dd80af996b0).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* ` "public class " + className + extendsText + " implements java.io.Serializable `
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by stephenh <gi...@git.apache.org>.
Github user stephenh commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-73272234
@srowen since you mentioned on the dev list that you have an itchy commit finger... :-), Josh had said a few comments up "If you fix the merge conflicts, I'll commit this.", which I've done (tests pass, etc.), so this should be all good to go.
(Cherry picking into the 1.3 branch would be awesome as well, FWIW...not sure how that works...)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by holdenk <gi...@git.apache.org>.
Github user holdenk commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-67690369
This looks good to me :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by stephenh <gi...@git.apache.org>.
Github user stephenh commented on a diff in the pull request:
https://github.com/apache/spark/pull/3725#discussion_r22114215
--- Diff: core/src/main/scala/org/apache/spark/TestUtils.scala ---
@@ -43,9 +43,19 @@ private[spark] object TestUtils {
* Note: if this is used during class loader tests, class names should be unique
* in order to avoid interference between tests.
*/
- def createJarWithClasses(classNames: Seq[String], value: String = ""): URL = {
+ def createJarWithClasses(
+ classNames: Seq[String],
+ value: String = "",
+ classpathUrls: Seq[URL] = Seq()): URL = {
val tempDir = Utils.createTempDir()
- val files = for (name <- classNames) yield createCompiledClass(name, tempDir, value)
+ val files = for (name <- classNames) yield {
+ if (name.indexOf(" ") == -1) {
--- End diff --
Makes sense; I added another param "classNamesWithBase" that is a Seq[(String,String)].
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-67674693
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24651/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by holdenk <gi...@git.apache.org>.
Github user holdenk commented on a diff in the pull request:
https://github.com/apache/spark/pull/3725#discussion_r22126832
--- Diff: core/src/test/scala/org/apache/spark/executor/ExecutorURLClassLoaderSuite.scala ---
@@ -37,6 +37,8 @@ class ExecutorURLClassLoaderSuite extends FunSuite {
val fakeClass = classLoader.loadClass("FakeClass2").newInstance()
val fakeClassVersion = fakeClass.toString
assert(fakeClassVersion === "1")
+ val fakeClass2 = classLoader.loadClass("FakeClass2").newInstance()
--- End diff --
thats bad :( Well glad we have a fix now :) Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-73293433
Thanks for the reminder. Given the sign-off from @holdenk and @vanzin, I'm going to merge this into `master` (1.4.0) and `branch-1.3` (1.3.0).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/3725#discussion_r22097263
--- Diff: core/src/main/scala/org/apache/spark/TestUtils.scala ---
@@ -43,9 +43,19 @@ private[spark] object TestUtils {
* Note: if this is used during class loader tests, class names should be unique
* in order to avoid interference between tests.
*/
- def createJarWithClasses(classNames: Seq[String], value: String = ""): URL = {
+ def createJarWithClasses(
+ classNames: Seq[String],
+ value: String = "",
+ classpathUrls: Seq[URL] = Seq()): URL = {
val tempDir = Utils.createTempDir()
- val files = for (name <- classNames) yield createCompiledClass(name, tempDir, value)
+ val files = for (name <- classNames) yield {
+ if (name.indexOf(" ") == -1) {
--- End diff --
How about a method taking `Seq[(String,String)]` instead as an overload for this case?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4877] Allow user first classes to exten...
Posted by holdenk <gi...@git.apache.org>.
Github user holdenk commented on the pull request:
https://github.com/apache/spark/pull/3725#issuecomment-67676131
I can take a look tonight :)
On Thursday, December 18, 2014, Stephen Haberman <no...@github.com>
wrote:
> @holdenk <https://github.com/holdenk> can you review this PR? I see that
> you wrote most of the original ChildExecutorURLClassLoader.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/3725#issuecomment-67594886>.
>
--
Cell : 425-233-8271
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org