SPARK-7169: Metrics can be additionally configured from Spark configuration #5788
Conversation
|
Merged build triggered. |
|
Merged build started. |
|
Test build #31320 has started for PR 5788 at commit
|
|
Test build #31320 has finished for PR 5788 at commit
|
|
Merged build finished. Test FAILed. |
|
Test FAILed. |
jacek-lewandowski
force-pushed
the
SPARK-7169
branch
from
750927c
to
71993fe
Compare
|
retest this please |
|
Merged build triggered. |
|
Merged build started. |
|
Test build #31324 has started for PR 5788 at commit
|
|
Test build #31324 has finished for PR 5788 at commit
|
|
Merged build finished. Test PASSed. |
|
Test PASSed. |
|
Why did you submit PR against branch 1.2? |
|
@jerryshao because this is a very minor fix and I wanted to prepare a PR for the lowest version I think it it suitable. After successful review and agree to merge it to particular versions i'll create particular PRs. |
| @@ -131,6 +131,11 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging { | |||
| set("spark.home", home) | |||
| } | |||
|
|
|||
| /** Set the metrics configuration property */ | |||
| def setMetricsProperty(name: String, value: String): SparkConf = { | |||
There was a problem hiding this comment.
I'm a little unsure about adding these metric-specific methods here. They're very particular to a specific scenario, while SparkConf is supposed to be a generic configuration class. I'd rather not add these (since I expect most people to set these via config files anyway).
|
LGTM aside from the SparkConf changes, which I think are unnecessary. I also agree with @jerryshao's comment; any change, be it a bug fix or feature, should always be made on master first, and later backported if desired. The only exception is when the change is not needed on master. |
|
@jacek-lewandowski do you plan to address the feedback here? Otherwise I can pick this up and post a PR against master. |
|
@jacek-lewandowski ping? |
|
@vanzin i'm really sorry - i must have missed the notification |
|
@jacek-lewandowski the updated version of this is already merged at #6560. Would you mind closing this patch? |
No description provided.