Deprecate external table location flag#5012
Conversation
There was a problem hiding this comment.
Pull request overview
This PR deprecates the ALLOW_EXTERNAL_TABLE_LOCATION feature flag as a legacy compatibility alias, clarifying that it relaxes metadata file location checks (not table location structure) and directing operators to the more specific flags.
Changes:
- Marked
FeatureConfiguration.ALLOW_EXTERNAL_TABLE_LOCATIONas deprecated and updated its description to point toALLOW_EXTERNAL_METADATA_FILE_LOCATION. - Updated production configuration guidance and the feature-flag reference docs to reflect the deprecation and correct semantics.
- Added a changelog entry documenting the deprecation and replacement guidance.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| site/content/in-dev/unreleased/configuration/configuring-polaris-for-production/_index.md | Updates production guidance to call out ALLOW_EXTERNAL_TABLE_LOCATION as deprecated and point to the correct replacement flag. |
| site/content/in-dev/unreleased/configuration/config-sections/flags-polaris_features.md | Updates the feature-flag reference text to clarify the deprecated flag’s behavior and recommended replacements. |
| polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java | Deprecates the legacy flag constant and updates its description to reduce operator confusion. |
| CHANGELOG.md | Documents the deprecation and the recommended replacement flag for external metadata file locations. |
| * retained as a compatibility alias for external metadata file locations. | ||
| */ | ||
| @SuppressWarnings("DeprecatedIsStillUsed") | ||
| @Deprecated |
There was a problem hiding this comment.
Is this helpful? If we do not want Polaris code to use it, let's just refactor to remove all usages.
If we want users to avoid using it, a code annotation is not going to be effective (doc change LGTM).
There was a problem hiding this comment.
I'm OK to remove the usage, but If we remove it, existing deployments using the old flag stop working immediately. That’s not deprecation anymore, it’s behavior removal. Are we OK to do so?
boolean allowEscape = realmConfig.getConfig(FeatureConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION);
if (!allowEscape
&& !realmConfig.getConfig(FeatureConfiguration.ALLOW_EXTERNAL_METADATA_FILE_LOCATION)) {
There was a problem hiding this comment.
I mean, let's deprecate it for end users in docs for now, but keep the code un-deprecated (because we have legit usage in current code). When we're ready to remove support, we'll simply drop the whole config constant.
There was a problem hiding this comment.
Sounds good to me. I think that's what the current PR does. Feel free to suggest any changes if I miss something.
There was a problem hiding this comment.
I'd propose removing this @Deprecated annotation (but keeping doc changes).
There was a problem hiding this comment.
step 1 SGTM. Do we have other steps? Is warning message still a blocker?
There was a problem hiding this comment.
Step 2: Add comments to "prod" code that refers to ALLOW_EXTERNAL_TABLE_LOCATION for backward-compatibility reasons. Explain why it still uses ALLOW_EXTERNAL_TABLE_LOCATION and suppress deprecation warnings in those specific cases.
That's all.
There was a problem hiding this comment.
After step 2 there should be no deprecation warnings related to ALLOW_EXTERNAL_TABLE_LOCATION in Polaris code.
There was a problem hiding this comment.
Fixed both steps in the new commit.
2735c1f to
0385fad
Compare
|
Had been busy with work lately. I will review later tonight. Sorry for the delay. |
| .buildFeatureConfiguration(); | ||
|
|
||
| /** | ||
| * @deprecated since 1.7.0, will be removed in 1.8.0. Use {@link |
There was a problem hiding this comment.
nit: Add since and forRemoval = true to @Deprecated?
There was a problem hiding this comment.
+1, maybe @Deprecated(since = "1.7.0", forRemoval = true)
github-project-automation
Bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
| .buildFeatureConfiguration(); | ||
|
|
||
| /** | ||
| * @deprecated since 1.7.0, will be removed in 1.8.0. Use {@link |
There was a problem hiding this comment.
+1, maybe @Deprecated(since = "1.7.0", forRemoval = true)
github-project-automation
Bot
moved this from Ready to merge
to Done
in Basic Kanban Board
Summary
Deprecates
ALLOW_EXTERNAL_TABLE_LOCATIONas a legacy compatibility alias for external metadata file locations.The current runtime behavior uses this flag to relax metadata file location checks, which overlaps with
ALLOW_EXTERNAL_METADATA_FILE_LOCATIONand makes it easy to confuse withALLOW_UNSTRUCTURED_TABLE_LOCATION. This PR keeps behavior unchanged, but points users to:ALLOW_EXTERNAL_METADATA_FILE_LOCATIONfor metadata files outside the table locationALLOW_UNSTRUCTURED_TABLE_LOCATIONfor table/view locations outside the structured namespace layoutChecklist
CHANGELOG.md.site/content/in-dev/unreleased.Prepared with AI assistance; I reviewed and own the changes.