Skip to content

Conversation

@zjffdu
Copy link
Contributor

@zjffdu zjffdu commented Aug 18, 2016

What changes were proposed in this pull request?

Allow to set spark configuration using non-string type in sparkR

How was this patch tested?

Tested manually using the following command

sparkR.session(master="yarn-client", sparkConfig = list(spark.executor.instances=1))

@SparkQA
Copy link

SparkQA commented Aug 18, 2016

Test build #63981 has finished for PR 14699 at commit 5540366.

  • This patch fails SparkR unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@felixcheung
Copy link
Member

Your example was failing for a different reason:

Error in invokeJava(isStatic = TRUE, className, methodName, ...) : 
  java.lang.IllegalArgumentException: spark.executor.instances should be int, but was 1.0

Means that your 1 is being interpreted as numeric instead of integer.
This works:

sparkR.session(master="yarn-client", sparkConfig = list(spark.executor.instances=1L))

@zjffdu
Copy link
Contributor Author

zjffdu commented Aug 18, 2016

Make sense, @felixcheung Thanks

@felixcheung
Copy link
Member

I haven't exhaustively test out configs, not sure always coercing to string might be limiting, in certain cases (eg. user was expecting 1 -> 1.0 (numeric) but now 1 -> "1" (character)

@zjffdu
Copy link
Contributor Author

zjffdu commented Aug 19, 2016

@felixcheung In what of case user was expecting 1 -> 1.0 (numeric), because internally SparkConf use string for both key and value.

@felixcheung
Copy link
Member

It's hard to say. Right now it is being converted on the JVM side - so it is possible to have 1 -> 1.0 -> "1.0"
Also convertNamedListToEnv are being in several other cases that seem to expect numeric type - could you check that?

@felixcheung
Copy link
Member

would you like to follow up or close this?

srowen added a commit to srowen/spark that referenced this pull request Oct 31, 2016
@asfgit asfgit closed this in 26b07f1 Oct 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants