[SPARK-34571][SQL][CORE] Provide a more convenient way to deprecate/remove/alternate configs

[Ngone51]

What changes were proposed in this pull request?

This pr proposes to add deprecated()/ removed() / alternated() functions to ConfigBuilder in order to provide more convenient ways to do these stuff.

Why are the changes needed?

Currently, Spark maintains the active configs and deprecated/alternated/removed configs separately,
which is not convenient when developers want to deprecate configs and not good to maintain for the long term.

After this PR, developers can deprecate/alternate/remove a config like this:

val SOME_CONFIG =
  buildConf("spark.xxx.yyy.zzz")
    .doc(".....")")
    .version("2.3.0")
    .deprecated("3.0", "because...")
    // .removed("3.0", "because...")
    // .alternated(("3.0", "spark.aaa.bbb.ccc"))
    .booleanConf
    .createWithDefault(false)

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Manually tested.

[Ngone51]

cc @cloud-fan @HyukjinKwon @MaxGekk any idea about this?

[MaxGekk]

This pr proposes to add deprecated()/ removed() / alternated() functions to ConfigBuilder in order to provide more convenient ways to do these stuff.

I think if you propose new approach, you should restore all removed configs and make them as .removed() then. Don't think it is good idea to maintain two approach at the same time.

maintains the active configs and deprecated/alternated/removed configs separately,
which is not convenient when developers want to deprecate configs and not good to maintain for the long term.

IMHO, gathering all removed/deprecated configs in one place is convenient. Currently, removed configs are actually removed but you propose to keep them forever (just mark them as removed), am I right?

[SparkQA]

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40144/

[SparkQA]

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40144/

[SparkQA]

Test build #135563 has finished for PR 31684 at commit 6e375d1.

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

[Ngone51]

I think if you propose new approach, you should restore all removed configs and make them as .removed() then. Don't think it is good idea to maintain two approach at the same time.

Yeah, I was thinking that too. But I'd like to see people's feedback before going further.

IMHO, gathering all removed/deprecated configs in one place is convenient.

I think it's convenient (fast) for people to know what are deprecated or removed configs now. I think u But it's not convenient for developers to maintain. For example, to deprecate a config with the current way, a developer needs to manually add DepracatedConfig to the deprecated list. And if the config is going to be removed in the future, the developer has to remove the DepracatedConfig from the deprecated list and add RemovedConfig to the removed list. And this process could be problematic because these two config types are separated in two places. e.g., developers may forget to remove the DepracatedConfig one. While with the proposed way in this PR, the developer only needs to add deprecated() and change from deprecated() to removed() later.

Currently, removed configs are actually removed but you propose to keep them forever (just mark them as removed), am I right?

Yes. But those removed configs don't disappear from Spark totally as they are still maintained in the way of RemovedConfig.

[Ngone51]

Besides, I'm thinking we may be able to extend the SQL configuration document with the new proposal. For example, we can add the current status of a conf, e.g., whether it's deprecated or removed, etc. like the version info does. cc @beliefer

[HyukjinKwon]

nit but alternated -> alternative

[HyukjinKwon]

Let's fix all Alternate -> Alternative

[HyukjinKwon]

Can we add deprecate and alternative ones first? For removal one, I would encourage to do it in a separate PR.

[cloud-fan]

Agree with @HyukjinKwon to do deprecation first. For the removed ones, we don't really need to keep the config entries for them anymore. We just need the names of removed configs (and the successors) to throw better error messages.

[beliefer]

@Ngone51 Record the version when deciding deprecate config, seems good.

[Ngone51]

Thanks for all the feedbacks..I'll start from the depreaction and alternatives first.

[SparkQA]

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40248/

[SparkQA]

Test build #135666 has finished for PR 31684 at commit 4a3497c.

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

[SparkQA]

Test build #135696 has finished for PR 31684 at commit efbee39.

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

[github-actions]

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!