From c495080f5805a2ada51676f278eda7e7b0140fb0 Mon Sep 17 00:00:00 2001 From: Dmitri Bourlatchkov Date: Fri, 23 May 2025 19:12:07 -0400 Subject: [PATCH 1/4] Make the method supporting old config names "first class" code. * Rename `catalogConfigUnsafe` to `legacyCatalogConfig` * Remove deprecation, because this method has a purpose in allowing backward compatibility when properties are renamed. * Remove the INFO log about calls to this method in Polaris, because users cannot do anything about these messages. Phasing out old property names required coordination with users (e.g. release notes), so it is not a matter of merely avoiding calls to that method in Polaris code. Fixes #1666 --- .../core/config/FeatureConfiguration.java | 16 ++++++++-------- .../core/config/PolarisConfiguration.java | 9 ++------- .../config/DefaultConfigurationStoreTest.java | 4 ++-- 3 files changed, 12 insertions(+), 17 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java index 7546212282..61b0f5ac70 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java @@ -87,7 +87,7 @@ public static void enforceFeatureEnabledOrThrow( PolarisConfiguration.builder() .key("ALLOW_TABLE_LOCATION_OVERLAP") .catalogConfig("polaris.config.allow.overlapping.table.location") - .catalogConfigUnsafe("allow.overlapping.table.location") + .legacyCatalogConfig("allow.overlapping.table.location") .description( "If set to true, allow one table's location to reside within another table's location. " + "This is only enforced within a given namespace.") @@ -122,7 +122,7 @@ public static void enforceFeatureEnabledOrThrow( PolarisConfiguration.builder() .key("ALLOW_UNSTRUCTURED_TABLE_LOCATION") .catalogConfig("polaris.config.allow.unstructured.table.location") - .catalogConfigUnsafe("allow.unstructured.table.location") + .legacyCatalogConfig("allow.unstructured.table.location") .description("If set to true, allows unstructured table locations.") .defaultValue(false) .buildFeatureConfiguration(); @@ -131,7 +131,7 @@ public static void enforceFeatureEnabledOrThrow( PolarisConfiguration.builder() .key("ALLOW_EXTERNAL_TABLE_LOCATION") .catalogConfig("polaris.config.allow.external.table.location") - .catalogConfigUnsafe("allow.external.table.location") + .legacyCatalogConfig("allow.external.table.location") .description( "If set to true, allows tables to have external locations outside the default structure.") .defaultValue(false) @@ -141,7 +141,7 @@ public static void enforceFeatureEnabledOrThrow( PolarisConfiguration.builder() .key("ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING") .catalogConfig("polaris.config.enable.credential.vending") - .catalogConfigUnsafe("enable.credential.vending") + .legacyCatalogConfig("enable.credential.vending") .description("If set to true, allow credential vending for external catalogs.") .defaultValue(true) .buildFeatureConfiguration(); @@ -150,7 +150,7 @@ public static void enforceFeatureEnabledOrThrow( PolarisConfiguration.>builder() .key("SUPPORTED_CATALOG_STORAGE_TYPES") .catalogConfig("polaris.config.supported.storage.types") - .catalogConfigUnsafe("supported.storage.types") + .legacyCatalogConfig("supported.storage.types") .description("The list of supported storage types for a catalog") .defaultValue( List.of( @@ -163,7 +163,7 @@ public static void enforceFeatureEnabledOrThrow( PolarisConfiguration.builder() .key("CLEANUP_ON_NAMESPACE_DROP") .catalogConfig("polaris.config.cleanup.on.namespace.drop") - .catalogConfigUnsafe("cleanup.on.namespace.drop") + .legacyCatalogConfig("cleanup.on.namespace.drop") .description("If set to true, clean up data when a namespace is dropped") .defaultValue(false) .buildFeatureConfiguration(); @@ -172,7 +172,7 @@ public static void enforceFeatureEnabledOrThrow( PolarisConfiguration.builder() .key("CLEANUP_ON_CATALOG_DROP") .catalogConfig("polaris.config.cleanup.on.catalog.drop") - .catalogConfigUnsafe("cleanup.on.catalog.drop") + .legacyCatalogConfig("cleanup.on.catalog.drop") .description("If set to true, clean up data when a catalog is dropped") .defaultValue(false) .buildFeatureConfiguration(); @@ -181,7 +181,7 @@ public static void enforceFeatureEnabledOrThrow( PolarisConfiguration.builder() .key("DROP_WITH_PURGE_ENABLED") .catalogConfig("polaris.config.drop-with-purge.enabled") - .catalogConfigUnsafe("drop-with-purge.enabled") + .legacyCatalogConfig("drop-with-purge.enabled") .description( "If set to true, allows tables to be dropped with the purge parameter set to true.") .defaultValue(false) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java index fd983e6e88..ad60f7743d 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java @@ -162,13 +162,8 @@ public Builder catalogConfig(String catalogConfig) { return this; } - /** - * Used to support backwards compatability before there were reserved properties. Usage of this - * method should be removed over time. - */ - @Deprecated - public Builder catalogConfigUnsafe(String catalogConfig) { - LOGGER.info("catalogConfigUnsafe is deprecated! Use catalogConfig() instead."); + /** Used to support backwards compatability when config properties get renamed. */ + public Builder legacyCatalogConfig(String catalogConfig) { if (catalogConfig.startsWith(SAFE_CATALOG_CONFIG_PREFIX)) { throw new IllegalArgumentException( "Unsafe catalog configs are not expected to start with " + SAFE_CATALOG_CONFIG_PREFIX); diff --git a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/config/DefaultConfigurationStoreTest.java b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/config/DefaultConfigurationStoreTest.java index 67a8e8e6a6..bbf9150a43 100644 --- a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/config/DefaultConfigurationStoreTest.java +++ b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/config/DefaultConfigurationStoreTest.java @@ -192,7 +192,7 @@ public void testRegisterAndUseFeatureConfigurations() { FeatureConfiguration unsafeConfig = FeatureConfiguration.builder() .key(String.format("%s_unsafe", prefix)) - .catalogConfigUnsafe(String.format("%s.unsafe", prefix)) + .legacyCatalogConfig(String.format("%s.unsafe", prefix)) .defaultValue(true) .description(prefix) .buildFeatureConfiguration(); @@ -201,7 +201,7 @@ public void testRegisterAndUseFeatureConfigurations() { FeatureConfiguration.builder() .key(String.format("%s_both", prefix)) .catalogConfig(String.format("polaris.config.%s.both", prefix)) - .catalogConfigUnsafe(String.format("%s.both", prefix)) + .legacyCatalogConfig(String.format("%s.both", prefix)) .defaultValue(true) .description(prefix) .buildFeatureConfiguration(); From cfa7a922d465bdf53f8d2f458bf9c753247884b4 Mon Sep 17 00:00:00 2001 From: Dmitri Bourlatchkov Date: Mon, 26 May 2025 15:09:54 -0400 Subject: [PATCH 2/4] review: rename back to catalogConfigUnsafe --- .../core/config/FeatureConfiguration.java | 16 ++++++++-------- .../core/config/PolarisConfiguration.java | 7 +++++-- .../config/DefaultConfigurationStoreTest.java | 4 ++-- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java index 61b0f5ac70..7546212282 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java @@ -87,7 +87,7 @@ public static void enforceFeatureEnabledOrThrow( PolarisConfiguration.builder() .key("ALLOW_TABLE_LOCATION_OVERLAP") .catalogConfig("polaris.config.allow.overlapping.table.location") - .legacyCatalogConfig("allow.overlapping.table.location") + .catalogConfigUnsafe("allow.overlapping.table.location") .description( "If set to true, allow one table's location to reside within another table's location. " + "This is only enforced within a given namespace.") @@ -122,7 +122,7 @@ public static void enforceFeatureEnabledOrThrow( PolarisConfiguration.builder() .key("ALLOW_UNSTRUCTURED_TABLE_LOCATION") .catalogConfig("polaris.config.allow.unstructured.table.location") - .legacyCatalogConfig("allow.unstructured.table.location") + .catalogConfigUnsafe("allow.unstructured.table.location") .description("If set to true, allows unstructured table locations.") .defaultValue(false) .buildFeatureConfiguration(); @@ -131,7 +131,7 @@ public static void enforceFeatureEnabledOrThrow( PolarisConfiguration.builder() .key("ALLOW_EXTERNAL_TABLE_LOCATION") .catalogConfig("polaris.config.allow.external.table.location") - .legacyCatalogConfig("allow.external.table.location") + .catalogConfigUnsafe("allow.external.table.location") .description( "If set to true, allows tables to have external locations outside the default structure.") .defaultValue(false) @@ -141,7 +141,7 @@ public static void enforceFeatureEnabledOrThrow( PolarisConfiguration.builder() .key("ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING") .catalogConfig("polaris.config.enable.credential.vending") - .legacyCatalogConfig("enable.credential.vending") + .catalogConfigUnsafe("enable.credential.vending") .description("If set to true, allow credential vending for external catalogs.") .defaultValue(true) .buildFeatureConfiguration(); @@ -150,7 +150,7 @@ public static void enforceFeatureEnabledOrThrow( PolarisConfiguration.>builder() .key("SUPPORTED_CATALOG_STORAGE_TYPES") .catalogConfig("polaris.config.supported.storage.types") - .legacyCatalogConfig("supported.storage.types") + .catalogConfigUnsafe("supported.storage.types") .description("The list of supported storage types for a catalog") .defaultValue( List.of( @@ -163,7 +163,7 @@ public static void enforceFeatureEnabledOrThrow( PolarisConfiguration.builder() .key("CLEANUP_ON_NAMESPACE_DROP") .catalogConfig("polaris.config.cleanup.on.namespace.drop") - .legacyCatalogConfig("cleanup.on.namespace.drop") + .catalogConfigUnsafe("cleanup.on.namespace.drop") .description("If set to true, clean up data when a namespace is dropped") .defaultValue(false) .buildFeatureConfiguration(); @@ -172,7 +172,7 @@ public static void enforceFeatureEnabledOrThrow( PolarisConfiguration.builder() .key("CLEANUP_ON_CATALOG_DROP") .catalogConfig("polaris.config.cleanup.on.catalog.drop") - .legacyCatalogConfig("cleanup.on.catalog.drop") + .catalogConfigUnsafe("cleanup.on.catalog.drop") .description("If set to true, clean up data when a catalog is dropped") .defaultValue(false) .buildFeatureConfiguration(); @@ -181,7 +181,7 @@ public static void enforceFeatureEnabledOrThrow( PolarisConfiguration.builder() .key("DROP_WITH_PURGE_ENABLED") .catalogConfig("polaris.config.drop-with-purge.enabled") - .legacyCatalogConfig("drop-with-purge.enabled") + .catalogConfigUnsafe("drop-with-purge.enabled") .description( "If set to true, allows tables to be dropped with the purge parameter set to true.") .defaultValue(false) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java index ad60f7743d..2abab82159 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java @@ -162,8 +162,11 @@ public Builder catalogConfig(String catalogConfig) { return this; } - /** Used to support backwards compatability when config properties get renamed. */ - public Builder legacyCatalogConfig(String catalogConfig) { + /** + * This method is to be used to support backwards compatability when config properties get + * renamed. It should not be used for current (or canonical) configuration names. + */ + public Builder catalogConfigUnsafe(String catalogConfig) { if (catalogConfig.startsWith(SAFE_CATALOG_CONFIG_PREFIX)) { throw new IllegalArgumentException( "Unsafe catalog configs are not expected to start with " + SAFE_CATALOG_CONFIG_PREFIX); diff --git a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/config/DefaultConfigurationStoreTest.java b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/config/DefaultConfigurationStoreTest.java index bbf9150a43..67a8e8e6a6 100644 --- a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/config/DefaultConfigurationStoreTest.java +++ b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/config/DefaultConfigurationStoreTest.java @@ -192,7 +192,7 @@ public void testRegisterAndUseFeatureConfigurations() { FeatureConfiguration unsafeConfig = FeatureConfiguration.builder() .key(String.format("%s_unsafe", prefix)) - .legacyCatalogConfig(String.format("%s.unsafe", prefix)) + .catalogConfigUnsafe(String.format("%s.unsafe", prefix)) .defaultValue(true) .description(prefix) .buildFeatureConfiguration(); @@ -201,7 +201,7 @@ public void testRegisterAndUseFeatureConfigurations() { FeatureConfiguration.builder() .key(String.format("%s_both", prefix)) .catalogConfig(String.format("polaris.config.%s.both", prefix)) - .legacyCatalogConfig(String.format("%s.both", prefix)) + .catalogConfigUnsafe(String.format("%s.both", prefix)) .defaultValue(true) .description(prefix) .buildFeatureConfiguration(); From 499dde8a8ae743ebad734f1d5620bd600c570e9f Mon Sep 17 00:00:00 2001 From: Dmitri Bourlatchkov Date: Tue, 27 May 2025 10:06:00 -0400 Subject: [PATCH 3/4] review: restore deprecation and javadoc --- .../org/apache/polaris/core/config/PolarisConfiguration.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java index 2abab82159..daf678dea3 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java @@ -163,9 +163,10 @@ public Builder catalogConfig(String catalogConfig) { } /** - * This method is to be used to support backwards compatability when config properties get - * renamed. It should not be used for current (or canonical) configuration names. + * Used to support backwards compatability before there were reserved properties. Usage of this + * method should be removed over time. */ + @Deprecated public Builder catalogConfigUnsafe(String catalogConfig) { if (catalogConfig.startsWith(SAFE_CATALOG_CONFIG_PREFIX)) { throw new IllegalArgumentException( From 52c68b4fd141a916b1d155bf28d2689af431dbe8 Mon Sep 17 00:00:00 2001 From: Dmitri Bourlatchkov Date: Tue, 27 May 2025 10:10:14 -0400 Subject: [PATCH 4/4] add javadoc suggestion for alternatives to the deprecated method --- .../org/apache/polaris/core/config/PolarisConfiguration.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java index daf678dea3..ab7d6f2b64 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java @@ -165,6 +165,8 @@ public Builder catalogConfig(String catalogConfig) { /** * Used to support backwards compatability before there were reserved properties. Usage of this * method should be removed over time. + * + * @deprecated Use {@link #catalogConfig()} instead. */ @Deprecated public Builder catalogConfigUnsafe(String catalogConfig) {