[SPARK-26482][CORE] Use ConfigEntry for hardcoded configs for ui categories #23423

HeartSaVioR · 2019-01-01T13:58:25Z

What changes were proposed in this pull request?

The PR makes hardcoded configs below to use ConfigEntry.

spark.ui
spark.ssl
spark.authenticate
spark.master.rest
spark.master.ui
spark.metrics
spark.admin
spark.modify.acl

This patch doesn't change configs which are not relevant to SparkConf (e.g. system properties).

How was this patch tested?

Existing tests.

SparkQA · 2019-01-01T14:09:14Z

Test build #100619 has finished for PR 23423 at commit 016ebe0.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
.doc(\"Comma separated list of filter class names to apply to the Spark Web UI.\")
.doc(\"Comma-separated list of class names for \"plugins\" implementing \" +

SparkQA · 2019-01-01T14:24:58Z

Test build #100621 has finished for PR 23423 at commit 1c9ee36.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-01-01T14:31:55Z

Test build #100622 has finished for PR 23423 at commit 5973c59.

This patch fails to build.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2019-01-01T14:47:47Z

Test build #100624 has finished for PR 23423 at commit 8d8721d.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-01-01T23:04:38Z

Test build #100626 has finished for PR 23423 at commit 6dc46aa.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-01-02T00:41:09Z

Test build #100627 has finished for PR 23423 at commit 15a8955.

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

HeartSaVioR · 2019-01-02T02:14:23Z

Strange error occurred after rebasing. Changed to WIP. Will fix the error and change the title back.

SparkQA · 2019-01-02T15:48:12Z

Test build #100638 has finished for PR 23423 at commit caa79d1.

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

SparkQA · 2019-01-02T23:04:59Z

Test build #100646 has finished for PR 23423 at commit 2569a75.

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

vanzin · 2019-01-02T23:40:30Z

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

Could you put UI-related options in a new file (e.g. UI.scala)?

I was going to suggest WebUI.scala but that already exists (albeit in a different place).

Thanks for the nice suggestion. Will address.

SparkQA · 2019-01-03T04:49:30Z

Test build #100667 has finished for PR 23423 at commit 31ba1d6.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
.doc(\"Comma separated list of filter class names to apply to the Spark Web UI.\")

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

Test build #100671 has finished for PR 23423 at commit 38dc7be.

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

HeartSaVioR · 2019-01-03T08:27:50Z

retest this, please

SparkQA · 2019-01-03T13:18:18Z

Test build #100678 has finished for PR 23423 at commit 38dc7be.

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

vanzin · 2019-01-03T23:49:05Z

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

In spite of the name, all of these ACL-related options are UI options.

I thought these options are common thing between UI and History, but looks like History has its own ACL configurations. Will move to UI.

vanzin · 2019-01-03T23:51:24Z

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

I'm leaning a bit on the side of removing the UI_ prefix here, and always referencing these as UI.foo in other code.

But don't really feel strongly either way.

Sounds nice. I'd also like to ensure "how to use configurations" be consistent though: breaking down config package object into multiple (like what we do with UI, History, Kafka, etc) and referring configuration as <object name>.<configuration name> all the cases.

Would it make sense to file issues for cleanup (more groups, change the way to refer configurations) as part of SPARK-26442?

Someone filed SPARK-22194 a while ago that is in that same spirit.

As for consistency, it may be better to be even more explicit (e.g. config.Blah.CONSTANT) but that is pretty verbose. Some parts of the code already do that.

vanzin · 2019-01-03T23:54:04Z

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

Since you're replacing them with the constant anyway, I'd rather have the tests use the new constant.

Then this backwards compatibility could be added to the deprecated stuff in the SparkConf object. But really, given the comment in SecurityManager, maybe we could even drop it altogether.

+1 to drop it, since it's for backward compatibility with 1.x and target version of this patch is 3.0 (new major version). Will drop it.

vanzin · 2019-01-03T23:55:31Z

core/src/main/scala/org/apache/spark/ui/SparkUI.scala

Can SparkUI.DEFAULT_PORT go away?

Yes it is only used for default value on configuration for UI_PORT at least in Spark codebase, so I guess it can be removed once UI_PORT has default value.

vanzin · 2019-01-03T23:57:21Z

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

These could be in a Standalone object, but since they're few, probably ok to have them here for now.

I'm OK in either way. I might miss some more configurations on standalone mode so we may have chance to move them in future.

SparkQA · 2019-01-04T08:05:02Z

Test build #100721 has finished for PR 23423 at commit 6bb772d.

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

HeartSaVioR · 2019-01-04T08:13:07Z

retest this, please

SparkQA · 2019-01-04T13:16:32Z

Test build #100725 has finished for PR 23423 at commit 6bb772d.

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

kiszk · 2019-01-06T18:45:11Z

core/src/main/scala/org/apache/spark/deploy/history/HistoryServer.scala

How about config.set(ACLS_ENABLE, false)?

Nice finding! Will address.

HeartSaVioR · 2019-01-07T01:35:10Z

@vanzin @kiszk
Addressed review comments. Seems like PRs for subtask of SPARK-26442 can make each other being conflicted, so it might be ideal if we could take review and merge a bit faster, and address some kinds of TODOs (like cleaning up) after all done.

SparkQA · 2019-01-07T06:18:41Z

Test build #100855 has finished for PR 23423 at commit 6edcc6f.

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

vanzin · 2019-01-08T00:06:21Z

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

This could use .toSequence too and clean up call sites a little bit.

Thanks for the suggestion. Looks like others which are used along with stringToSet can be applied. I'll see which configurations can be applied and make changes.

gaborgsomogyi · 2019-01-09T17:01:34Z

Just wanted to check, could you merge the master?

HeartSaVioR · 2019-01-09T21:01:53Z

Just rebased (with addressing config changes to the new code), still need to reflect @vanzin's latest comment.

HeartSaVioR · 2019-01-10T00:24:24Z

Applied .toSequence to some configurations in UI and History which are later split by "," in call site.

SparkQA · 2019-01-10T00:29:26Z

Test build #100993 has finished for PR 23423 at commit 2942c32.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-01-10T02:05:10Z

Test build #100987 has finished for PR 23423 at commit a3f06e8.

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

SparkQA · 2019-01-10T04:26:06Z

Test build #100990 has finished for PR 23423 at commit b3526ff.

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

SparkQA · 2019-01-10T05:30:50Z

Test build #100994 has finished for PR 23423 at commit 8d61242.

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

HeartSaVioR · 2019-01-10T05:33:39Z

@vanzin @kiszk
I guess I addressed review comments and the build is green.
Could you please take a look again? Thanks in advance!

@vanzin

…gories * Rename conflicted configuration field names * Move out UI related configurations to separate object * Address review comments from @vanzin

HeartSaVioR · 2019-01-10T13:27:50Z

Just rebased again.

SparkQA · 2019-01-10T17:46:53Z

Test build #101022 has finished for PR 23423 at commit f5a5772.

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

HeartSaVioR · 2019-01-10T20:34:41Z

retest this, please

vanzin · 2019-01-10T22:46:41Z

Looks good pending tests.

SparkQA · 2019-01-11T01:50:10Z

Test build #101036 has finished for PR 23423 at commit f5a5772.

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

vanzin · 2019-01-11T18:17:46Z

Merging to master.

dongjoon-hyun · 2019-01-12T01:35:15Z

...ts/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/KubernetesTestComponents.scala

-      .set("spark.ui.enabled", "true")
-      .set(IS_TESTING.key, "false")
+      .set(IS_TESTING, false)
+      .set(UI_ENABLED, true)


Hi, all. This breaks the K8S integration test.
I'll make a followup PR.

…gories ## What changes were proposed in this pull request? The PR makes hardcoded configs below to use `ConfigEntry`. * spark.ui * spark.ssl * spark.authenticate * spark.master.rest * spark.master.ui * spark.metrics * spark.admin * spark.modify.acl This patch doesn't change configs which are not relevant to SparkConf (e.g. system properties). ## How was this patch tested? Existing tests. Closes apache#23423 from HeartSaVioR/SPARK-26466. Authored-by: Jungtaek Lim (HeartSaVioR) <[email protected]> Signed-off-by: Marcelo Vanzin <[email protected]>

## What changes were proposed in this pull request? This fixes K8S integration test compilation failure introduced by apache#23423 . ```scala $ build/sbt -Pkubernetes-integration-tests test:package ... [error] /Users/dongjoon/APACHE/spark/resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/KubernetesTestComponents.scala:71: type mismatch; [error] found : org.apache.spark.internal.config.OptionalConfigEntry[Boolean] [error] required: String [error] .set(IS_TESTING, false) [error] ^ [error] /Users/dongjoon/APACHE/spark/resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/KubernetesTestComponents.scala:71: type mismatch; [error] found : Boolean(false) [error] required: String [error] .set(IS_TESTING, false) [error] ^ [error] two errors found ``` ## How was this patch tested? Pass the K8S integration test. Closes apache#23527 from dongjoon-hyun/SPARK-26482. Authored-by: Dongjoon Hyun <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

HeartSaVioR force-pushed the SPARK-26466 branch from c9af5ec to 016ebe0 Compare January 1, 2019 13:59

HeartSaVioR force-pushed the SPARK-26466 branch from 5973c59 to 8d8721d Compare January 1, 2019 14:37

HeartSaVioR force-pushed the SPARK-26466 branch from 8d8721d to 6dc46aa Compare January 1, 2019 23:00

HeartSaVioR force-pushed the SPARK-26466 branch from 6dc46aa to 15a8955 Compare January 1, 2019 23:09

HeartSaVioR changed the title ~~[SPARK-26482][CORE] Use ConfigEntry for hardcoded configs for ui categories~~ [WIP][SPARK-26482][CORE] Use ConfigEntry for hardcoded configs for ui categories Jan 2, 2019

HeartSaVioR changed the title ~~[WIP][SPARK-26482][CORE] Use ConfigEntry for hardcoded configs for ui categories~~ [SPARK-26482][CORE] Use ConfigEntry for hardcoded configs for ui categories Jan 2, 2019

vanzin reviewed Jan 2, 2019

View reviewed changes

vanzin reviewed Jan 3, 2019

View reviewed changes

kiszk reviewed Jan 6, 2019

View reviewed changes

HeartSaVioR force-pushed the SPARK-26466 branch from 6bb772d to 6edcc6f Compare January 7, 2019 01:22

vanzin reviewed Jan 8, 2019

View reviewed changes

HeartSaVioR force-pushed the SPARK-26466 branch from 6edcc6f to a3f06e8 Compare January 9, 2019 21:01

HeartSaVioR force-pushed the SPARK-26466 branch from 2942c32 to 8d61242 Compare January 10, 2019 00:40

HeartSaVioR added 4 commits January 10, 2019 22:26

[SPARK-26482][CORE] Use ConfigEntry for hardcoded configs for ui cate…

8eeb9ed

…gories * Rename conflicted configuration field names * Move out UI related configurations to separate object * Address review comments from @vanzin

Address @kiszk review comment

0ebe432

Address review comment from @vanzin

5c4ced7

Also apply .toSequence to configurations in UI and History

f5a5772

HeartSaVioR force-pushed the SPARK-26466 branch from 8d61242 to f5a5772 Compare January 10, 2019 13:26

asfgit closed this in d9e4cf6 Jan 11, 2019

dongjoon-hyun reviewed Jan 12, 2019

View reviewed changes

dongjoon-hyun mentioned this pull request Jan 12, 2019

[SPARK-26482][K8S][TEST][FOLLOWUP] Fix compile failure #23527

Closed

HeartSaVioR deleted the SPARK-26466 branch January 14, 2019 02:50

[SPARK-26482][CORE] Use ConfigEntry for hardcoded configs for ui categories #23423

[SPARK-26482][CORE] Use ConfigEntry for hardcoded configs for ui categories #23423

Uh oh!

Conversation

HeartSaVioR commented Jan 1, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jan 1, 2019

Uh oh!

SparkQA commented Jan 1, 2019

Uh oh!

SparkQA commented Jan 1, 2019

Uh oh!

SparkQA commented Jan 1, 2019

Uh oh!

SparkQA commented Jan 1, 2019

Uh oh!

SparkQA commented Jan 2, 2019

Uh oh!

HeartSaVioR commented Jan 2, 2019

Uh oh!

SparkQA commented Jan 2, 2019

Uh oh!

SparkQA commented Jan 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 3, 2019

Uh oh!

SparkQA commented Jan 3, 2019

Uh oh!

HeartSaVioR commented Jan 3, 2019

Uh oh!

SparkQA commented Jan 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR Jan 4, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 4, 2019

Uh oh!

HeartSaVioR commented Jan 4, 2019

Uh oh!

SparkQA commented Jan 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR commented Jan 7, 2019

Uh oh!

SparkQA commented Jan 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR Jan 4, 2019 •

edited

Loading

HeartSaVioR commented Jan 10, 2019 •

edited

Loading