[SPARK-30060][CORE] Rename metrics enable/disable configs #26692

LucaCanali · 2019-11-27T11:14:22Z

What changes were proposed in this pull request?

This proposes to introduce a naming convention for Spark metrics configuration parameters used to enable/disable metrics source reporting using the Dropwizard metrics library: spark.metrics.sourceNameCamelCase.enabled and update 2 parameters to use this naming convention.

Why are the changes needed?

Currently Spark has a few parameters to enable/disable metrics reporting. Their naming pattern is not uniform and this can create confusion. Currently we have:
spark.metrics.static.sources.enabled
spark.app.status.metrics.enabled
spark.sql.streaming.metricsEnabled

Does this PR introduce any user-facing change?

Update parameters for enabling/disabling metrics reporting new in Spark 3.0: spark.metrics.static.sources.enabled -> spark.metrics.staticSources.enabled, spark.app.status.metrics.enabled -> spark.metrics.appStatusSource.enabled.
Note: spark.sql.streaming.metricsEnabled is left unchanged as it is already in use in Spark 2.x.

How was this patch tested?

Manually tested

core/src/main/scala/org/apache/spark/internal/config/Status.scala

cloud-fan · 2019-11-27T12:12:06Z

ok to test

cloud-fan · 2019-11-27T14:29:45Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala


  val STREAMING_METRICS_ENABLED =
-    buildConf("spark.sql.streaming.metricsEnabled")
+    buildConf("spark.metrics.sparkStreamingSource.enabled")


It's a breaking change to rename an already released config. Can we leave it as it is?

Or, what about renaming together at 3.0? We still can support legacy config names together, cannot we?
Also, cc @gatorsmile since he suggest renaming.

The old config would have to be deprecated and be an alias for the new one, at least.

Yep. +1 for that.

I fully agree on the need to deprecate the old parameter and adding an alias.
Another point I see about renaming spark.sql.streaming.metricsEnabled to spark.metrics.sparkStreamingSource.enabled, is that the configuration lives in [[SQLConf]] where all configuration parameters appear to have spark.sql as their prefix. For example, would using spark.sql.metrics.sparkStreamingSource.enabled be a better choice? I see pros and cons to that. Any advice?

Adding the voice of name name here: we may want to differentiate DStream and Structured Streaming. Before the change it is clear that it's for Structured Streaming (sql.streaming) which new name is not clear about that. If we want to only mention streaming it should also mention sql. If we are OK to have structuredStreaming as a part of name, that's fine.

SparkQA · 2019-11-27T14:46:29Z

Test build #114531 has finished for PR 26692 at commit dd089f0.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2019-11-29T01:14:48Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

      .createWithDefault(true)

  val STREAMING_METRICS_ENABLED =
-    buildConf("spark.sql.streaming.metricsEnabled")


If you want to change this existing naming space, you should change all others consistently, e.g. spark.sql.streaming.fileSource.schema.forceNullable. sql.streaming looks pretty clear to me.

You should also consider moving from SQLConf to SparkConf if you want to change the name space consistently. (e.g., spark.pyspark vs spark.sql...pyspark)

It doesn't look like worth changing to me given the overhead. Let's keep this name space.

I agree that changing spark.sql.streaming.metricsEnabled appears to open a can of worms. Maybe we should just stick to the other 2 parameters that are easier to change.

SparkQA · 2019-11-29T11:31:05Z

Test build #114622 has finished for PR 26692 at commit 852e865.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2019-12-03T00:46:27Z

core/src/main/scala/org/apache/spark/internal/config/Status.scala

    .intConf
    .createWithDefault(Int.MaxValue)

  val APP_STATUS_METRICS_ENABLED =


While we are consolidating this, shall we update APP_STATUS_METRICS_ENABLED -> METRICS_APP_STATUS_SOURCE_ENABLED together more consistently?

HyukjinKwon

Looks fine to me too except @dongjoon-hyun's comment. It should be best to keep PR description updated.

SparkQA · 2019-12-03T08:05:02Z

Test build #114756 has finished for PR 26692 at commit 49052de.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-12-03T08:57:47Z

retest this please

SparkQA · 2019-12-03T11:24:44Z

Test build #114763 has finished for PR 26692 at commit 49052de.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun

+1, LGTM. Merged to master.
Thank you all!

### What changes were proposed in this pull request? This proposes to introduce a naming convention for Spark metrics configuration parameters used to enable/disable metrics source reporting using the Dropwizard metrics library: `spark.metrics.sourceNameCamelCase.enabled` and update 2 parameters to use this naming convention. ### Why are the changes needed? Currently Spark has a few parameters to enable/disable metrics reporting. Their naming pattern is not uniform and this can create confusion. Currently we have: `spark.metrics.static.sources.enabled` `spark.app.status.metrics.enabled` `spark.sql.streaming.metricsEnabled` ### Does this PR introduce any user-facing change? Update parameters for enabling/disabling metrics reporting new in Spark 3.0: `spark.metrics.static.sources.enabled` -> `spark.metrics.staticSources.enabled`, `spark.app.status.metrics.enabled` -> `spark.metrics.appStatusSource.enabled`. Note: `spark.sql.streaming.metricsEnabled` is left unchanged as it is already in use in Spark 2.x. ### How was this patch tested? Manually tested Closes apache#26692 from LucaCanali/uniformNamingMetricsEnableParameters. Authored-by: Luca Canali <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

LucaCanali mentioned this pull request Nov 27, 2019

[SPARK-29654][CORE] Add configuration to allow disabling registration of static sources to the metrics system #26320

Closed

cloud-fan reviewed Nov 27, 2019

View reviewed changes

core/src/main/scala/org/apache/spark/internal/config/Status.scala Show resolved Hide resolved

cloud-fan reviewed Nov 27, 2019

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-30060][CORE][SS][DOC] Implement naming convention for Spark metrics configuration parameters used to enable/disable metrics sources~~ Rename metrics enable/disable configs Nov 27, 2019

dongjoon-hyun changed the title ~~Rename metrics enable/disable configs~~ [SPARK-30060][CORE][SS] Rename metrics enable/disable configs Nov 27, 2019

dongjoon-hyun added the SPARK CORE label Nov 27, 2019

HyukjinKwon reviewed Nov 29, 2019

View reviewed changes

Rename metrics enable/disable configs

852e865

LucaCanali force-pushed the uniformNamingMetricsEnableParameters branch from dd089f0 to 852e865 Compare November 29, 2019 08:26

cloud-fan approved these changes Dec 2, 2019

View reviewed changes

dongjoon-hyun reviewed Dec 3, 2019

View reviewed changes

HyukjinKwon approved these changes Dec 3, 2019

View reviewed changes

LucaCanali changed the title ~~[SPARK-30060][CORE][SS] Rename metrics enable/disable configs~~ [SPARK-30060][CORE] Rename metrics enable/disable configs Dec 3, 2019

Renamed APP_STATUS_METRICS_ENABLED to METRICS_APP_STATUS_SOURCE_ENABLED

49052de

dongjoon-hyun approved these changes Dec 3, 2019

View reviewed changes

dongjoon-hyun closed this in 60f20e5 Dec 3, 2019

[SPARK-30060][CORE] Rename metrics enable/disable configs #26692

[SPARK-30060][CORE] Rename metrics enable/disable configs #26692

Uh oh!

Conversation

LucaCanali commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Uh oh!

cloud-fan commented Nov 27, 2019

Uh oh!

cloud-fan Nov 27, 2019

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Nov 27, 2019

Choose a reason for hiding this comment

Uh oh!

srowen Nov 27, 2019

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Nov 27, 2019

Choose a reason for hiding this comment

Uh oh!

LucaCanali Nov 27, 2019

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR Nov 27, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 27, 2019

Uh oh!

HyukjinKwon Nov 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LucaCanali Nov 29, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 29, 2019

Uh oh!

dongjoon-hyun Dec 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 3, 2019

Uh oh!

cloud-fan commented Dec 3, 2019

Uh oh!

SparkQA commented Dec 3, 2019

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

LucaCanali commented Nov 27, 2019 •

edited

Loading

HyukjinKwon Nov 29, 2019 •

edited

Loading

dongjoon-hyun Dec 3, 2019 •

edited

Loading