[SPARK-20887][CORE] support alternative keys in ConfigBuilder #18110
Conversation
|
FWIW I didn't actually say that we should rename that key since the cost of
the confusing name isn't that high right now. So while I don't oppose this
mechanism I'm neutral on it given that the only use case so far seems kind
of minor. I was mostly commenting just so that future readers and reviewers
can more easily spot the issue and hopefully pick better names going
forward.
…On Thu, May 25, 2017 at 8:57 AM Apache Spark QA ***@***.***> wrote:
*Test build #77372 has started
<https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/77372/testReport>*
for PR 18110 at commit cc51dd0
<cc51dd0>
.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#18110 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AADGPMaYT_X0iqsDi6q5OhpOOxZpnKQzks5r9aT0gaJpZM4Nmjjt>
.
|
|
@JoshRosen actually there are more use cases. Currently we have 2 ways to define a config: create a config entry in the |
|
@cloud-fan, what about |
|
It's only used in the |
|
Ahhh, makes sense. Thanks for the clarification. |
|
Test build #77372 has finished for PR 18110 at commit
|
jiangxb1987
left a comment
jiangxb1987
left a comment
There was a problem hiding this comment.
LGTM only have a nit.
| } | ||
| } | ||
|
|
||
| private def getOrDefault(conf: ConfigProvider, key: String): Option[String] = { |
There was a problem hiding this comment.
nit: add comment for this method.
|
Test build #77398 has finished for PR 18110 at commit
|
|
retest please |
|
Test build #77411 has started for PR 18110 at commit |
|
retest this please |
|
Test build #77417 has finished for PR 18110 at commit
|
|
thanks for the review, merging to master! |
### _Why are the changes needed?_ Refer Spark PR: apache/spark#18110 ConfigBuilder builds ConfigEntry which can only read value with one key, if we wanna change the config name but still keep the old one, it's hard to do. This PR introduce ConfigBuilder.withAlternative, to support reading config value with alternative keys. ### _How was this patch tested?_ - [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [ ] Add screenshots for manual tests if appropriate - [x] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request Closes #3659 from turboFei/conf_alternative. Closes #3659 e268fef [Fei Wang] add ut a2300b2 [Fei Wang] add ut 53eccf9 [Fei Wang] Support alternative keys in ConfigBuilder Authored-by: Fei Wang <[email protected]> Signed-off-by: Fei Wang <[email protected]>
4 participants
What changes were proposed in this pull request?
ConfigBuilderbuildsConfigEntrywhich can only read value with one key, if we wanna change the config name but still keep the old one, it's hard to do.This PR introduce
ConfigBuilder.withAlternative, to support reading config value with alternative keys. And also renamespark.scheduler.listenerbus.eventqueue.sizetospark.scheduler.listenerbus.eventqueue.capacitywith this feature, according to #14269 (comment)How was this patch tested?
a new test