[SPARK-5249] Added type specific set functions to SparkConf. #4042
Conversation
|
Can one of the admins verify this patch? |
| @@ -141,6 +141,26 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging { | |||
| this | |||
| } | |||
|
|
|||
| /** Set a boolean parameter */ | |||
There was a problem hiding this comment.
Rather than all these overloads, why not expose set(key: String, value: Any) since it is stored internally as a String from toString() anyway? This lets you add a generic setIfMissing too without so many methods.
Would you also then change the code that calls set to not have to make it into a String in the caller? to gain the benefit that this enables.
|
Made the changes, what do you think? |
| @@ -56,14 +56,14 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging { | |||
| } | |||
|
|
|||
| /** Set a configuration variable. */ | |||
| def set(key: String, value: String): SparkConf = { | |||
| def set(key: String, value: Any): SparkConf = { | |||
There was a problem hiding this comment.
Ah, the problem is that this would break binary compatibility, I believe. I think this would have to be a new method. Are there cases in the code base where you can now replace a set("foo", bar.toString) with set("foo", bar)?
There was a problem hiding this comment.
I recompiled spark with this change and played around with spark-shell and everything seems to be fine.
|
If you run the MiMa checks, I'm pretty sure that this will break binary compatibility because it changes the signature of a public method. Let's see, though: Jenkins, this is ok to test. |
|
Hey @AdamGS - unfortunately we can't accept changes that break binary compatibility like this. Jenkins, test this please. |
|
@pwendell, will just adding the new set (and setIfMissing) methods work? |
|
Yes, we could just add new ones. For this feature though, I guess I don't see why users cant' just call |
|
It's just a small thing that bothers me and might bother others, so I decided to try getting it into the code base because it makes for "prettier" code in my opinion. |
|
I'd actually prefer not to have this in Spark. It's not really clear what we will do with an |
|
@JoshRosen or @srowen - what are your feelings on it? |
|
It's a small thing, so I can't feel too strongly either way. I think it would have been a fine method to add in the beginning. Now, it's only really worth it if the code base is changed to use the simplified method, and that may not be worth it at this point. |
|
I don't think that this change is worth it; having to call |
|
Okay - @AdamGS thanks for sending this patch but I think we'll pass on adding this API. Overall we're pretty conservative with adding API's like this if there isn't a compelling reason. |
|
Let's close this issue. |
Just something that bothered me, might me comfortable to others.