fix(build): Fix deprecation warnings in FeatureConfiguration #1894
Conversation
adutra
requested review from
ashvina,
dennishuo,
dimas-b,
eric-maynard,
jackye1995,
jbonofre,
vvcephei,
collado-mike,
snazy,
RussellSpitzer and
takidau
as code owners
adutra
requested review from
MonkeyCanCode,
flyrain,
ebyhr,
ajantha-bhat,
HonahX,
singhpk234,
pingtimeout and
gh-yzou
as code owners
| @@ -83,6 +83,7 @@ public static void enforceFeatureEnabledOrThrow( | |||
| .defaultValue(false) | |||
| .buildFeatureConfiguration(); | |||
|
|
|||
| @SuppressWarnings("deprecation") | |||
There was a problem hiding this comment.
This does get rid of the warnings, but aren't the warnings the main feature of the @Deprecated annotation?
ref: #1672 (comment)
There was a problem hiding this comment.
The usage of the old config key is deprecated - using that should trigger a user-facing deprecation warning. The deprecation is triggering at the wrong site then.
There was a problem hiding this comment.
Thanks for the link I didn't know there was a previous conversation on this topic.
That said, I rather disagree: the annotation is valuable mainly for consumers of the API, and especially when it's well documented in terms of when it was deprecated, when it will be removed and how to replace it.
Here, we own both the source of the deprecation warnings (the catalogConfigUnsafe method) and the call sites where the deprecation warnings are triggered (in FeatureConfiguration class): IOW, keeping the warnings around is not useful for Polaris devs.
And there is no risk of "forgetting" to update FeatureConfiguration later on: as soon as we remove the deprecated method, the FeatureConfiguration class wouldn't compile anymore.
There was a problem hiding this comment.
My take on this: we should rather remove deprecation from catalogConfigUnsafe() and file issues to remove support for those legacy properties. The method itself has a valid use case in Polaris code, which is specifically to support migration from old config names to new name, so the method itself is not conceptually "deprecated".
There was a problem hiding this comment.
A io.smallrye.config.RelocateConfigSourceInterceptor implementation can be used to nag users to update the configs.
There was a problem hiding this comment.
There's already a log to remind users to remove the configs, so we are covered from that perspective. This was more intended to nag maintainers to remove the deprecated method at some point in time. Otherwise, why deprecate methods? Will we always add this suppression when a method is deprecated?
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
|
Looks like the PR needs a rebase |
adutra
force-pushed
the
fix-build-warnings
branch
from
29ea56f to
2da5794
Compare
|
+1 |
|
@eric-maynard is that OK to merge this? |
|
I think this is ready to be merged. |
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
|
@adutra my question wasn't really answered -- is the strategy to always suppress all deprecation warnings? |
|
@eric-maynard the strategy is to address deprecation coming from dependencies. If something needs to be done in the Polaris code base in the future, it's better to open an issue so it doesn't get lost. |
|
That still doesn’t answer my question. Will we suppress all deprecation warnings? |
|
@eric-maynard @snazy @adutra : I'd like to reopen my suggestion of removing deprecation from This method is not really intended to be called by "users" of Polaris. For internal calls (or calls from downstream projects) the purpose is to provide an upgrade path for users to gradually adjust to the new config names. So, in my mind the All in all, I think it is preferable to remove deprecation from |
|
That makes more sense to me. If we don't want to mark it as deprecated, that is a reasonable argument, but I don't really grok why we would mark it as deprecated but then disable the warning. My argument for leaving it as deprecated is mostly to make it clear that there should be no new use of this method. And, when the old configs are yanked, the method can accordingly be yanked as well. In that sense it is marked for deprecation/removal. |
As discussed in apache#1894, `catalogConfigUnsafe()` has a use case in the main codebase - that is to support gradual migration to new property names. Using legacy names in runtime already leads to a user-face WARN log message. Support for those legacy property names is to be dropped in 2.0.
|
Ok, I opened #1970 for my proposal. If accepted/merged, I'll follow up with an issue for dropping old config names in 2.0 |
No description provided.