-
Notifications
You must be signed in to change notification settings - Fork 28.4k
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-21208][R] Adds setLocalProperty and getLocalProperty in R #20075
Conversation
@@ -564,6 +564,23 @@ setJobDescription <- function(value) { | |||
invisible(callJMethod(sc, "setJobDescription", value)) | |||
} | |||
|
|||
#' Set a local property that affects jobs submitted from this thread, such as the | |||
#' Spark fair scheduler pool. |
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.
I tried to make a better description for a while but then just ended up with copying Python's:
spark/python/pyspark/context.py
Lines 934 to 939 in 209b936
def setLocalProperty(self, key, value): | |
""" | |
Set a local property that affects jobs submitted from this thread, such as the | |
Spark fair scheduler pool. | |
""" | |
self._jsc.setLocalProperty(key, value) |
cc @felixcheung, could you take a look please? |
Test build #85369 has finished for PR 20075 at commit
|
retest this please |
Test build #85370 has finished for PR 20075 at commit
|
LG, I’d as.character on both the key and value to make sure it doesn’t fail with signature mismatch.
Also should we also have getLocalProperty, you think?
|
Yea, let me update it soon. |
35e05a0
to
650a486
Compare
Test build #85405 has finished for PR 20075 at commit
|
Test build #85406 has finished for PR 20075 at commit
|
getLocalProperty Should not be returning invisible, I think.
Also can you check if we need to fail for key == NA?
|
Yup, they should be handled too. Let me update. |
if (is.null(key) || is.na(key)) { | ||
stop("key should not be NULL or NA.") | ||
} | ||
if (!is.null(value)) { |
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.
Just to be clear, is.na
case seems fine with passing it with as.character
. I added the test.
Test build #85417 has finished for PR 20075 at commit
|
Great LGTM
|
Merged to master. Thank you @felixcheung. |
What changes were proposed in this pull request?
This PR adds
setLocalProperty
andgetLocalProperty
in R.How was this patch tested?
Manually tested and a test in
R/pkg/tests/fulltests/test_context.R
.