-
Notifications
You must be signed in to change notification settings - Fork 28.2k
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.
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
[SPARK-8607] SparkR -- jars not being added to application classpath correctly #7001
Conversation
Add `getStaticClass` method in SparkR's `RBackendHandler`
JIRA issue open here: https://issues.apache.org/jira/browse/SPARK-8607 |
@@ -88,6 +88,19 @@ private[r] class RBackendHandler(server: RBackend) | |||
ctx.close() | |||
} | |||
|
|||
//get classPath for static object. This addresses SPARK-5185 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: Space after //
- Also could you expand the comment to say Looks up a class given a class name. This function first checks the current class loader and if a class is not found, it looks up the class in the context class loader.
Jenkins, add to whitelist |
Jenkins, ok to test |
Test build #35732 has finished for PR 7001 at commit
|
@shivaram Fixed style nits. |
@cafreeman Code change looks good -- I'm wondering if there is way to add a test for this in SparkR. We could add a dummy jar file and then try and see if we can call into that ? @davies Do we have a dummy jar file we use for similar tests in Python / Java etc. |
Test build #35738 has finished for PR 7001 at commit
|
Jenkins, retest this please |
Test build #35742 has finished for PR 7001 at commit
|
@shivaram where would we store test data? I have a JAR we could probably use. |
We could put it in in |
@shivaram Just committed a first draft of a test, but there are two issues:
|
Sorry the first one was a bug we introduced yesterday. Let me send a hotfix for it right now |
@cafreeman The worrying thing is that the tests seem to pass right now on Jenkins and on my machine if I run |
@cafreeman Created #7022 -- Could you take a look at it ? |
@cafreeman Regarding the test itself, a couple of things
|
Test build #35798 has finished for PR 7001 at commit
|
@shivaram Alright, I've uploaded a smaller JAR and switched to an absolute filepath using SPARK_HOME. So far this test runs successfully in both the sparkR shell and in regular R, but fails when triggered along with the entire test suite, but I'm not sure why. I added a |
Test build #35810 has finished for PR 7001 at commit
|
Thanks @cafreeman for the update. I'll try this out today and see why |
So I took a look at this and I managed to figure out why the tests don't work but why it works from sparkR shell. Its due to the fact we have two different ways in which things are initialized
For the unit tests we can't test the first case as it is a batch script. But we can test the second case by actually running the sparkR script from the unit test. This would be something similar to https://github.com/apache/spark/blob/master/python/pyspark/tests.py#L1641 |
`test_includeJAR.R` now executes an external sparkR script which, in turn, initializes a new spark context with the test JAR, runs the test functions, and returns the output to the original test. As a result, we can now run this test inside the SparkR test suite and get around the fact that we can't add the "sparkJars" argument to `sparkR.init()` once the JVM has already started.
@shivaram Took your advice and changed things so that the test runs another script using R's |
Test build #35876 has finished for PR 7001 at commit
|
Hmm - weirdly the Scala spark-submit test failed. Lets give this another go and see if it was just a flaky test. Jenkins, retest this please |
Jenkins, retest this please |
Test build #35884 has finished for PR 7001 at commit
|
LGTM. That test case looks great. Thanks @cafreeman |
No problem! |
…correctly Add `getStaticClass` method in SparkR's `RBackendHandler` This is a fix for the problem referenced in [SPARK-5185](https://issues.apache.org/jira/browse/SPARK-5185). cc shivaram Author: cafreeman <cfreeman@alteryx.com> Closes #7001 from cafreeman/branch-1.4 and squashes the following commits: 8f81194 [cafreeman] Add missing license 31aedcf [cafreeman] Refactor test to call an external R script 2c22073 [cafreeman] Merge branch 'branch-1.4' of github.com:apache/spark into branch-1.4 0bea809 [cafreeman] Fixed relative path issue and added smaller JAR ee25e60 [cafreeman] Merge branch 'branch-1.4' of github.com:apache/spark into branch-1.4 9a5c362 [cafreeman] test for including JAR when launching sparkContext 9101223 [cafreeman] Merge branch 'branch-1.4' of github.com:apache/spark into branch-1.4 5a80844 [cafreeman] Fix style nits 7c6bd0c [cafreeman] [SPARK-8607] SparkR
Add
getStaticClass
method in SparkR'sRBackendHandler
This is a fix for the problem referenced in SPARK-5185.
cc @shivaram