Hi Eric, This discussion is starting to blend with the comment trail in PR 5012 a bit (link in my first message).
Feature flag deprecation is interesting, indeed. A "production readiness" checks can be effective in warning end users about altertering the deprecated feature flag's value via configuration settings (-D, files, env., etc.) The point I'm trying to emphasise in this thread is not so much about specific changes we need for PR 5012, but a general approach of applying the migration strategy for the deprecated flag at internal usage sites. For example, if existing tests use ALLOW_EXTERNAL_TABLE_LOCATION to allow metadata files to be placed at arbitrary locations, they could probably be updated to use ALLOW_EXTERNAL_METADATA_FILE_LOCATION instead. At least this appears to be the suggestion in the original deprecation email [1] Doing that removes the irritating build warnings and also informs downstream users about a possible migration path. On top of that we can suppress the warnings at usage sites that provide backward compatibility (e.g. in the production readiness checks). All in all, the end result is going to be nice and meaningful both for internal project developers and downstream users, I think. [1] https://lists.apache.org/thread/t2pn7t2h983qjpkzq7n08cxxv941n8pm Cheers, Dmitri. On Fri, Jul 10, 2026 at 6:59 PM Eric Maynard <[email protected]> wrote: > That’s a good distinction to make, though I think feature flags sit in a > somewhat unique position here. They are expressly intended for users to > configure runtime behavior without writing a line of code (and to keep us > honest about regressions & deprecations). > > We really want to discourage anyone — outside or inside the project — from > relying on flags we plan to deprecate and build warnings are one of the few > levers we have to do that. > > Instead of marking the flag itself as deprecated, I suppose what we really > want to deprecate is setting the flag to a nonstandard value (in this case > True). We could do that at runtime — the first time we read a nonstandard > value from a flag marked as deprecated, we could log. Does that work > better? > > —EM > > On Fri, Jul 10, 2026 at 3:20 PM Dmitri Bourlatchkov <[email protected]> > wrote: > > > Hi Eric, > > > > In my view, deprecating a method is a signal from a library (such as > > Polaris in this case) to its downstream users. In this context, it is > > indeed intended to produce build warnings to entice downstream users to > > update their code and use a supported alternative. > > > > Within a project itself there is little value in deprecating a method > > internally to cause build warnings. This creates nothing but distraction > > for people working with the project's source code. I believe all internal > > usages of the deprecated method should switch to the supported > alternative > > at the same time as the deprecation annotation is added. > > > > The only common exception is supporting existing _external_ users of the > > deprecated method / variable to allow them to migrate gradually. > > > > The gradual aspect, however, is not relevant to internal usages since at > > the time of deprecation a supported alternative must already exist, > > otherwise the deprecation is not meaningful. > > > > Cheers, > > Dmitri. > > > > On Fri, Jul 10, 2026 at 6:07 PM Eric Maynard <[email protected]> > > wrote: > > > > > If the community resolves to never introduce build warnings, what > exactly > > > would the utility of the @Deprecated annotation be? My understanding is > > > that it primarily exists to intentionally add a build warning, > prompting > > > downstream projects to not rely on some functionality that will later > be > > > removed. Is the resolution you’re actually advocating for just “don’t > use > > > @Deprecated”? > > > > > > —EM > > > > > > On Fri, Jul 10, 2026 at 3:00 PM Yufei Gu <[email protected]> wrote: > > > > > > > Respect between contributors and reviewers should be mutual. I don't > > > think > > > > introducing a warning for a well justified reason should block a PR. > If > > > the > > > > community believes that no new build warnings should ever be > > introduced, > > > > even for intentional deprecations, then I think we should make that > an > > > > explicit rule rather than an arbitrary behavior that reviewers apply > > on a > > > > case-by-case basis. > > > > > > > > Having a clear, documented rule gives contributors predictable > > > > expectations, avoids double standards, and ensures review decisions > > rely > > > on > > > > community agreed guidelines rather than individual reviewer > > preferences. > > > > That ultimately leads to a fairer and more consistent review process > > for > > > > everyone. > > > > > > > > Yufei > > > > > > > > > > > > On Thu, Jul 9, 2026 at 10:03 PM Adnan Hemani via dev < > > > > [email protected]> > > > > wrote: > > > > > > > > > Hi JB, > > > > > > > > > > Can you explain what you mean by "fully acceptable if explained"? > In > > > the > > > > > case that came up (in the PR linked in the original message), > > > > > the @Deprecated tag was being used to alert end users who may be > > using > > > a > > > > > particular config. In your opinion, is that a reasonable cause for > > > being > > > > > "fully acceptable"? > > > > > > > > > > Best, > > > > > Adnan Hemani > > > > > > > > > > On Thu, Jul 9, 2026 at 9:56 PM Jean-Baptiste Onofré < > [email protected] > > > > > > > > wrote: > > > > > > > > > > > Hi Dmitri > > > > > > > > > > > > I fully agree that we should avoid introducing build warnings. If > > we > > > > do, > > > > > we > > > > > > must clearly document the reasons (especially for the reviewer). > > > > > > > > > > > > It's an implicit good practice, in my humble opinion. Also, I > would > > > > > > consider it's up to the reviewer to remind the contributor of > that > > > good > > > > > > practice. > > > > > > It's certainly not a hard rule, but a good practice, and it's > fully > > > > > > acceptable if explained. > > > > > > > > > > > > Regards > > > > > > JB > > > > > > > > > > > > On Fri, Jul 10, 2026 at 3:28 AM Dmitri Bourlatchkov < > > > [email protected]> > > > > > > wrote: > > > > > > > > > > > > > Hi All, > > > > > > > > > > > > > > This is to follow-up on review comments in PR [5012], > > specifically > > > > [1] > > > > > > > > > > > > > > People working with the codebase on a regular basic have to pay > > > > > attention > > > > > > > to many factors to ensure code quality. This requires a > > significant > > > > > > > cognitive effort. > > > > > > > > > > > > > > One of the factors is the presence of build warnings. I believe > > it > > > > is a > > > > > > > generally good practice to avoid introducing new build warnings > > > when > > > > > > > technically possible. > > > > > > > > > > > > > > Thoughts? > > > > > > > > > > > > > > [1] > > > > https://github.com/apache/polaris/pull/5012#discussion_r3547988353 > > > > > > > > > > > > > > [5012] https://github.com/apache/polaris/pull/5012 > > > > > > > > > > > > > > Thanks, > > > > > > > Dmitri. > > > > > > > > > > > > > > > > > > > > > > > > > > > >
