Mark Deprecated with removal for join-pushdown.with-expressions

[chenjian2664]

Description

We should following below steps to retire or remove config.
Removing a configuration property
When a configuration is being fully removed, the following steps should be applied:

1. Mark the configuration as deprecated using @Deprecated(forRemoval = true).
2. Remove the configuration code + add the removed configuration key to @DefunctConfig (if applicable)

Renaming a configuration property
When a configuration is renamed (with or without a future removal plan), apply the following steps:

1. Mark the old configuration with @Deprecated and @LegacyConfig for old configuration key. If the old name is expected to be removed eventually, set forRemoval = true in @Deprecated.
2. if with removal old configuration:

• Add @LegacyConfig on the old configuration with replacedBy pointing to the new name.(also we could revert @Deprecated(forRemoval = true) back to @Deprecated or even remove @Deprecated fully)
• Add the removed configuration key to @DefunctConfig (if applicable)

The join-pushdown.with-expressions original in #27498 was renamed but actually is going to be removed, thus we follow the removed instructions.
in pr: revert the rename(removed the prefix "deprecated.") and mark @Deprecated(forRemoval = true) at current.

Additional context and related issues

#27153 (comment)

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

## Section
* Fix some things. ({issue}`issuenumber`)

[findepi]

initially #27498 just made the property @Deprecated. this triggers a startup warning when property is used. obviously some people don't read startup warnings...

then @kokosing proposed in in #27498 (comment) to force server startup failure when the property is used -- simply by renaming the property

now, if we add @LegacyConfig are we back to situation when using the property generates just a startup warning?
if that's what we really want then

• seek @kokosing 's approval, because he explicitly wanted otherwise AND
• revert the rename of the property. we don't need @LegacyConfig. Just @Deprecated triggers a warning and is more appropriate to this situation.

[kokosing]

I suggest rename here too.

[martint]

We already have a mechanism for deprecating config options. It’s LegacyConfig.

Renaming a property for the purpose of deprecation forces a breaking change twice: once when it’s renamed, and once when it’s removed. That’s a worse user experience.

If the warning on the console and a mention of the deprecation in the release notes is not enough, then I would argue that the user doesn’t care enough about hygiene and diligence.

[chenjian2664]

It seems we need to establish a clear agreement on how configuration options should be retired and removed. Should we follow a rename-then-remove approach, mark them as deprecated first, or introduce a LegacyConfig path before eventual removal ?
Each option has its own trade-offs, so we should finalize a consistent policy.
What is the appropriate next step for driving this forward?
Should I open a community or slack discussion, schedule a meeting, or take another approach?
anyway, we should finalize it @martint @findepi @kokosing

[findepi]

We already have a mechanism for deprecating config options. It’s LegacyConfig.

We actually have two: @LegacyConfig and @Deprecated

when a used property is both @Config and @Deprecated:

==========

Configuration should be updated:

1) Configuration property 'query.client.timeout' is deprecated and should not be used

==========

when a used property is both @Config and @Deprecated(forRemoval = true):

==========

Configuration should be updated:

1) Configuration property 'query.client.timeout' is deprecated and will be removed in the future

==========

when a used property is both @Config and @LegacyConfig and the legacy name is used, we print this

==========

Configuration should be updated:

1) Configuration property 'query.client.timeout' has been replaced. Use 'query.client.timeout.new' instead.

==========

So

• @LegacyConfig is the way to go when we our goal is to rename a property or replace with a better config toggle. We want users to use the new name.
• @Deprecated is the way to go when our goal is to remove a property.

Renaming a property for the purpose of deprecation forces a breaking change twice: once when it’s renamed, and once when it’s removed. That’s a worse user experience.

it has one advantage though
the first breakage acts like the last boarding call for a train. It gives operators a chance to come back to us and say they actually need particular property before it's removed along with the code it was configuring.

i am not saying this is super important. in particular, i don't think this is the case with this property, but we can't say this doesn't exist.

If the warning on the console and a mention of the deprecation in the release notes is not enough, then I would argue that the user doesn’t care enough about hygiene and diligence.

i wish all peoples followed the same principles, then there wouldn't be disagreements

however, i know of many people who don't study release notes and don't pay attention to warnings. this is not to say we should do a particular process, but acknowledging their existence might be helpful

[findepi]

that was long. @chenjian2664 if you want to avoid the breakage -- i assume this was the intention of this PR -- the correct way to do it is

1. revert the rename, i.e. drop "deprecated." prefix that was added in Deprecate join-pushdown.with-expressions config property #27498
2. keep the property as @Deprecated

[findepi]

Updated output examples above to include @Deprecated(forRemoval = true).
Addition of forRemoval = true triggers a better startup warning.
(i don't really get why the distinction. to me deprecation is always with the intent of eventual removal)

[chenjian2664]

Thanks, @findepi . My understanding is as follows: when we rename a configuration, we should use @Deprecated together with @LegacyConfig. If we plan to remove the old configuration name (which is likely in most cases), we can later mark the @Deprecated with forRemoval = true. As a final step, we can either remove the legacy code entirely or use replacedBy in @LegacyConfig to provide a clear migration path before the actual removal
We should use replacedBy in @LegacyConfig to provide a clear migration path before the actual removal

For removing config, just added @Drepcated(forRemoval = true) at first, then for final removal is the same as rename, use ReplacedBy in @LegacyConfig then remove the code

[chenjian2664]

with @Drepcated(forRemoval = true) + @LegacyConfig:

==========

Configuration should be updated:

1) Configuration property 'join-pushdown.with-expressions' has been replaced. Use 'deprecated.join-pushdown.with-expressions' instead.
2) Configuration property 'join-pushdown.with-expressions' is deprecated and will be removed in the future

==========

[findepi]

Airlift has a checker that if the setter is @Deprecated , the getter is also deprecated.
That's why both are deprecated. Let's mark both with forRemoval for consistency.

[findepi]

This needs approval from @kokosing per #27619 (comment)

[findepi]

Given than 479 came out without this PR, this would be now another breaking change.
IMO we should drop this PR now.

[chenjian2664]

@findepi I was thought i was merged. Then the release note now needs to be fixed since we don't apply this pr, I think we should add missing release note for it, declare there is a breaking change we rename the join-pushdown.with-expressions WDYT?

[findepi]

sure, lets update release notes. they should definitely reflect what we shipped
like here

• Warn about S3 permission breaking change in release notes #27699

[chenjian2664]

Let me close it, since we fixed the release note in #27718