From d8d760f9e3ca89523d5c51172fb6274cc9092aea Mon Sep 17 00:00:00 2001 From: Yun Zou Date: Thu, 29 May 2025 22:35:43 -0700 Subject: [PATCH 1/9] handle scope injection gracefully --- .../config/DefaultConfigurationStoreTest.java | 25 ++++++++++++-- .../config/DefaultConfigurationStore.java | 33 ++++++++++++------- 2 files changed, 44 insertions(+), 14 deletions(-) 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..2ab7b8446b 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 @@ -24,6 +24,7 @@ import io.quarkus.test.junit.QuarkusTest; import io.quarkus.test.junit.QuarkusTestProfile; import io.quarkus.test.junit.TestProfile; +import jakarta.enterprise.context.control.RequestContextController; import jakarta.inject.Inject; import java.time.Clock; import java.util.Map; @@ -74,11 +75,13 @@ public Map getConfigOverrides() { } private PolarisCallContext polarisContext; + private RealmContext realmContext; @Inject MetaStoreManagerFactory managerFactory; @Inject PolarisConfigurationStore configurationStore; @Inject PolarisDiagnostics diagServices; @Inject FeaturesConfiguration featuresConfiguration; + @Inject RequestContextController contextController; @BeforeEach public void before(TestInfo testInfo) { @@ -87,8 +90,8 @@ public void before(TestInfo testInfo) { .formatted( testInfo.getTestMethod().map(java.lang.reflect.Method::getName).orElse("test"), System.nanoTime()); - RealmContext realmContext = () -> realmName; - QuarkusMock.installMockForType(realmContext, RealmContext.class); + // RealmContext realmContext = () -> realmName; + realmContext = () -> realmName; polarisContext = new PolarisCallContext( managerFactory.getOrCreateSessionSupplier(realmContext).get(), @@ -97,8 +100,23 @@ public void before(TestInfo testInfo) { Clock.systemDefaultZone()); } + @Test + public void testGetConfigurationWithNoRealmContext() { + Object value = configurationStore.getConfiguration(polarisContext, "missingKeyWithoutDefault"); + assertThat(value).isNull(); + Object defaultValue = + configurationStore.getConfiguration( + polarisContext, "missingKeyWithDefault", "defaultValue"); + assertThat(defaultValue).isEqualTo("defaultValue"); + + // the falseByDefaultKey is set to false for all realms in Profile.getConfigOverrides + assertThat((Boolean) configurationStore.getConfiguration(polarisContext, falseByDefaultKey)) + .isFalse(); + } + @Test public void testGetConfiguration() { + QuarkusMock.installMockForType(realmContext, RealmContext.class); Object value = configurationStore.getConfiguration(polarisContext, "missingKeyWithoutDefault"); assertThat(value).isNull(); Object defaultValue = @@ -139,6 +157,7 @@ public void testGetRealmConfiguration() { @Test public void testInjectedConfigurationStore() { + QuarkusMock.installMockForType(realmContext, RealmContext.class); // the default value for trueByDefaultKey is `true` boolean featureDefaultValue = configurationStore.getConfiguration(polarisContext, trueByDefaultKey); @@ -159,6 +178,7 @@ public void testInjectedConfigurationStore() { @Test public void testInjectedFeaturesConfiguration() { + QuarkusMock.installMockForType(realmContext, RealmContext.class); assertThat(featuresConfiguration).isInstanceOf(QuarkusResolvedFeaturesConfiguration.class); assertThat(featuresConfiguration.defaults()) @@ -179,6 +199,7 @@ public void testInjectedFeaturesConfiguration() { @Test public void testRegisterAndUseFeatureConfigurations() { + QuarkusMock.installMockForType(realmContext, RealmContext.class); String prefix = "testRegisterAndUseFeatureConfigurations"; FeatureConfiguration safeConfig = diff --git a/service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java b/service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java index 5bc4fe51ca..355dd9c4a9 100644 --- a/service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java +++ b/service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java @@ -51,19 +51,28 @@ public DefaultConfigurationStore( @Override public @Nullable T getConfiguration(@Nonnull PolarisCallContext ctx, String configName) { - if (!realmContextInstance.isUnsatisfied()) { - RealmContext realmContext = realmContextInstance.get(); - String realm = realmContext.getRealmIdentifier(); - LOGGER.debug("Get configuration value for {} with realm {}", configName, realm); - @SuppressWarnings("unchecked") - T confgValue = - (T) - Optional.ofNullable(realmOverrides.getOrDefault(realm, Map.of()).get(configName)) - .orElseGet(() -> getDefaultConfiguration(configName)); - return confgValue; - } else { + try { + if (realmContextInstance.isResolvable()) { + RealmContext realmContext = realmContextInstance.get(); + String realm = realmContext.getRealmIdentifier(); + LOGGER.debug("Get configuration value for {} with realm {}", configName, realm); + @SuppressWarnings("unchecked") + T confgValue = + (T) + Optional.ofNullable(realmOverrides.getOrDefault(realm, Map.of()).get(configName)) + .orElseGet(() -> getDefaultConfiguration(configName)); + return confgValue; + } else { + LOGGER.debug( + "No RealmContext is injected when lookup value for configuration {} ", configName); + return getDefaultConfiguration(configName); + } + } catch (Exception e) { LOGGER.debug( - "No RealmContext is injected when lookup value for configuration {} ", configName); + "Exception encountered when lookup value for configuration {} with exception {}", + configName, + e); + // fall back to the default behavior return getDefaultConfiguration(configName); } } From 4db92bf5139398970a805a29ea3854d9b6042ea0 Mon Sep 17 00:00:00 2001 From: Yun Zou Date: Thu, 29 May 2025 23:25:12 -0700 Subject: [PATCH 2/9] updatelogger --- .../polaris/service/config/DefaultConfigurationStore.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java b/service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java index 355dd9c4a9..60aa52ea56 100644 --- a/service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java +++ b/service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java @@ -68,10 +68,7 @@ public DefaultConfigurationStore( return getDefaultConfiguration(configName); } } catch (Exception e) { - LOGGER.debug( - "Exception encountered when lookup value for configuration {} with exception {}", - configName, - e); + LOGGER.debug("Exception encountered when lookup value for configuration {}", configName, e); // fall back to the default behavior return getDefaultConfiguration(configName); } From d4acf9f4d92a93d0724c1e7a631c6264bf72e66d Mon Sep 17 00:00:00 2001 From: Yun Zou Date: Fri, 30 May 2025 18:06:43 -0700 Subject: [PATCH 3/9] add new getConfiguration Context --- .../config/PolarisConfigurationStore.java | 43 +++++++++++++++++++ .../service/catalog/io/FileIOUtil.java | 2 +- .../config/DefaultConfigurationStore.java | 37 ++++++++-------- .../service/task/TableCleanupTaskHandler.java | 8 ++-- 4 files changed, 67 insertions(+), 23 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java index 662a88d72d..a29f87b748 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java @@ -43,7 +43,14 @@ public interface PolarisConfigurationStore { * @param configName the name of the configuration key to check * @return the current value set for the configuration key or null if not set * @param the type of the configuration value + * + * @deprecated this function is going to be deprecated, please use the following + * function to get the configuration value in a more robust way: + * getConfiguration(String realm, String configName) + * This function can not be called outside of active request scope, such as + * background tasks (TaskExecutor). */ + @Deprecated default @Nullable T getConfiguration(PolarisCallContext ctx, String configName) { return null; } @@ -57,7 +64,14 @@ public interface PolarisConfigurationStore { * @param defaultValue the default value if the configuration key has no value * @return the current value or the supplied default value * @param the type of the configuration value + * + * @deprecated this function is going to be deprecated, please use the following + * function to get the configuration value in a more robust way: + * getConfiguration(String realm, String configName, @Nonnull T defaultValue) + * This function can not be called outside of active request scope, such as + * background tasks (TaskExecutor). */ + @Deprecated default @Nonnull T getConfiguration( PolarisCallContext ctx, String configName, @Nonnull T defaultValue) { Preconditions.checkNotNull(defaultValue, "Cannot pass null as a default value"); @@ -65,6 +79,35 @@ public interface PolarisConfigurationStore { return configValue != null ? configValue : defaultValue; } + /** + * Retrieve the current value for a configuration key for a given realm. May be null if not set. + * + * @param realm the realm identifier + * @param configName the name of the configuration key to check + * @return the current value set for the configuration key for the given realm, or null if not set + * @param the type of the configuration value + */ + default @Nullable T getConfiguration(String realm, String configName) { + return null; + } + + /** + * Retrieve the current value for a configuration key for the given realm. If not set, + * return the non-null default value. + * + * @param realm the realm identifier + * @param configName the name of the configuration key to check + * @param defaultValue the default value if the configuration key has no value + * @return the current value or the supplied default value + * @param the type of the configuration value + */ + default @Nonnull T getConfiguration( + String realm, String configName, @Nonnull T defaultValue) { + Preconditions.checkNotNull(defaultValue, "Cannot pass null as a default value"); + T configValue = getConfiguration(realm, configName); + return configValue != null ? configValue : defaultValue; + } + /** * In some cases, we may extract a value that doesn't match the expected type for a config. This * method can be used to attempt to force-cast it using `String.valueOf` diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java b/service/common/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java index e6c02ee542..a1961d60f8 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java @@ -87,7 +87,7 @@ public static AccessConfig refreshAccessConfig( boolean skipCredentialSubscopingIndirection = configurationStore.getConfiguration( - callContext.getPolarisCallContext(), + callContext.getRealmContext().getRealmIdentifier(), FeatureConfiguration.SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION.key, FeatureConfiguration.SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION.defaultValue); if (skipCredentialSubscopingIndirection) { diff --git a/service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java b/service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java index 60aa52ea56..fc27c85a43 100644 --- a/service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java +++ b/service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java @@ -49,27 +49,26 @@ public DefaultConfigurationStore( this.realmOverrides = Map.copyOf(configurations.parseRealmOverrides(objectMapper)); } + @Override + public @Nullable T getConfiguration(String realm, String configName) { + LOGGER.debug("Get configuration value for {} with realm {}", configName, realm); + @SuppressWarnings("unchecked") + T confgValue = + (T) + Optional.ofNullable(realmOverrides.getOrDefault(realm, Map.of()).get(configName)) + .orElseGet(() -> getDefaultConfiguration(configName)); + return confgValue; + } + @Override public @Nullable T getConfiguration(@Nonnull PolarisCallContext ctx, String configName) { - try { - if (realmContextInstance.isResolvable()) { - RealmContext realmContext = realmContextInstance.get(); - String realm = realmContext.getRealmIdentifier(); - LOGGER.debug("Get configuration value for {} with realm {}", configName, realm); - @SuppressWarnings("unchecked") - T confgValue = - (T) - Optional.ofNullable(realmOverrides.getOrDefault(realm, Map.of()).get(configName)) - .orElseGet(() -> getDefaultConfiguration(configName)); - return confgValue; - } else { - LOGGER.debug( - "No RealmContext is injected when lookup value for configuration {} ", configName); - return getDefaultConfiguration(configName); - } - } catch (Exception e) { - LOGGER.debug("Exception encountered when lookup value for configuration {}", configName, e); - // fall back to the default behavior + if (realmContextInstance.isResolvable()) { + RealmContext realmContext = realmContextInstance.get(); + String realm = realmContext.getRealmIdentifier(); + return getConfiguration(realm, configName); + } else { + LOGGER.debug( + "No RealmContext is injected when lookup value for configuration {} ", configName); return getDefaultConfiguration(configName); } } diff --git a/service/common/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java b/service/common/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java index ff791bf188..cc34120711 100644 --- a/service/common/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java +++ b/service/common/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java @@ -120,7 +120,8 @@ public boolean handleTask(TaskEntity cleanupTask, CallContext callContext) { fileIO, tableEntity, metaStoreManager, - polarisCallContext); + polarisCallContext, + callContext.getRealmContext().getRealmIdentifier()); List taskEntities = Stream.concat(manifestCleanupTasks, metadataFileCleanupTasks).toList(); @@ -206,11 +207,12 @@ private Stream getMetadataTaskStream( FileIO fileIO, IcebergTableLikeEntity tableEntity, PolarisMetaStoreManager metaStoreManager, - PolarisCallContext polarisCallContext) { + PolarisCallContext polarisCallContext, + String realm) { int batchSize = polarisCallContext .getConfigurationStore() - .getConfiguration(polarisCallContext, BATCH_SIZE_CONFIG_KEY, 10); + .getConfiguration(realm, BATCH_SIZE_CONFIG_KEY, 10); return getMetadataFileBatches(tableMetadata, batchSize).stream() .map( metadataBatch -> { From 15bc55f77e8c720e2e8aefd65966f5da1a1b29ad Mon Sep 17 00:00:00 2001 From: Yun Zou Date: Sun, 1 Jun 2025 14:56:30 -0700 Subject: [PATCH 4/9] add new api --- .../config/PolarisConfigurationStore.java | 26 +++++++-------- .../config/DefaultConfigurationStoreTest.java | 32 +++++++++++-------- .../config/DefaultConfigurationStore.java | 2 +- 3 files changed, 31 insertions(+), 29 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java index a29f87b748..c0341d5d84 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java @@ -43,12 +43,10 @@ public interface PolarisConfigurationStore { * @param configName the name of the configuration key to check * @return the current value set for the configuration key or null if not set * @param the type of the configuration value - * - * @deprecated this function is going to be deprecated, please use the following - * function to get the configuration value in a more robust way: - * getConfiguration(String realm, String configName) - * This function can not be called outside of active request scope, such as - * background tasks (TaskExecutor). + * @deprecated this function is going to be deprecated, please use the following function to get + * the configuration value in a more robust way: getConfiguration(String realm, String + * configName) This function can not be called outside of active request scope, such as + * background tasks (TaskExecutor). */ @Deprecated default @Nullable T getConfiguration(PolarisCallContext ctx, String configName) { @@ -64,12 +62,10 @@ public interface PolarisConfigurationStore { * @param defaultValue the default value if the configuration key has no value * @return the current value or the supplied default value * @param the type of the configuration value - * - * @deprecated this function is going to be deprecated, please use the following - * function to get the configuration value in a more robust way: - * getConfiguration(String realm, String configName, @Nonnull T defaultValue) - * This function can not be called outside of active request scope, such as - * background tasks (TaskExecutor). + * @deprecated this function is going to be deprecated, please use the following function to get + * the configuration value in a more robust way: getConfiguration(String realm, String + * configName, @Nonnull T defaultValue) This function can not be called outside of active + * request scope, such as background tasks (TaskExecutor). */ @Deprecated default @Nonnull T getConfiguration( @@ -87,13 +83,13 @@ public interface PolarisConfigurationStore { * @return the current value set for the configuration key for the given realm, or null if not set * @param the type of the configuration value */ - default @Nullable T getConfiguration(String realm, String configName) { + default @Nullable T getConfiguration(String realm, String configName) { return null; } /** - * Retrieve the current value for a configuration key for the given realm. If not set, - * return the non-null default value. + * Retrieve the current value for a configuration key for the given realm. If not set, return the + * non-null default value. * * @param realm the realm identifier * @param configName the name of the configuration key to check 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 2ab7b8446b..03bc63994b 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 @@ -24,7 +24,6 @@ import io.quarkus.test.junit.QuarkusTest; import io.quarkus.test.junit.QuarkusTestProfile; import io.quarkus.test.junit.TestProfile; -import jakarta.enterprise.context.control.RequestContextController; import jakarta.inject.Inject; import java.time.Clock; import java.util.Map; @@ -81,7 +80,6 @@ public Map getConfigOverrides() { @Inject PolarisConfigurationStore configurationStore; @Inject PolarisDiagnostics diagServices; @Inject FeaturesConfiguration featuresConfiguration; - @Inject RequestContextController contextController; @BeforeEach public void before(TestInfo testInfo) { @@ -90,7 +88,6 @@ public void before(TestInfo testInfo) { .formatted( testInfo.getTestMethod().map(java.lang.reflect.Method::getName).orElse("test"), System.nanoTime()); - // RealmContext realmContext = () -> realmName; realmContext = () -> realmName; polarisContext = new PolarisCallContext( @@ -102,16 +99,9 @@ public void before(TestInfo testInfo) { @Test public void testGetConfigurationWithNoRealmContext() { - Object value = configurationStore.getConfiguration(polarisContext, "missingKeyWithoutDefault"); - assertThat(value).isNull(); - Object defaultValue = - configurationStore.getConfiguration( - polarisContext, "missingKeyWithDefault", "defaultValue"); - assertThat(defaultValue).isEqualTo("defaultValue"); - - // the falseByDefaultKey is set to false for all realms in Profile.getConfigOverrides - assertThat((Boolean) configurationStore.getConfiguration(polarisContext, falseByDefaultKey)) - .isFalse(); + Assertions.assertThatThrownBy( + () -> configurationStore.getConfiguration(polarisContext, "missingKeyWithoutDefault")) + .isInstanceOf(IllegalStateException.class); } @Test @@ -155,6 +145,22 @@ public void testGetRealmConfiguration() { .isFalse(); } + @Test + void testGetConfigurationWithRealm() { + // the falseByDefaultKey is set to `false` for all realms, but overwrite with value `true` for + // realmOne. + assertThat((Boolean) configurationStore.getConfiguration(realmOne, falseByDefaultKey)).isTrue(); + // the trueByDefaultKey is set to `false` for all realms, no overwrite for realmOne + assertThat((Boolean) configurationStore.getConfiguration(realmOne, trueByDefaultKey)).isTrue(); + + // the falseByDefaultKey is set to `false` for all realms, no overwrite for realmTwo + assertThat((Boolean) configurationStore.getConfiguration(realmTwo, falseByDefaultKey)) + .isFalse(); + // the trueByDefaultKey is set to `false` for all realms, and overwrite with value `false` for + // realmTwo + assertThat((Boolean) configurationStore.getConfiguration(realmTwo, trueByDefaultKey)).isFalse(); + } + @Test public void testInjectedConfigurationStore() { QuarkusMock.installMockForType(realmContext, RealmContext.class); diff --git a/service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java b/service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java index fc27c85a43..398e1c7bdf 100644 --- a/service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java +++ b/service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java @@ -50,7 +50,7 @@ public DefaultConfigurationStore( } @Override - public @Nullable T getConfiguration(String realm, String configName) { + public @Nullable T getConfiguration(String realm, String configName) { LOGGER.debug("Get configuration value for {} with realm {}", configName, realm); @SuppressWarnings("unchecked") T confgValue = From e431b5f0c627d3e0bdfda7b1b737645ff68450b2 Mon Sep 17 00:00:00 2001 From: Yun Zou Date: Sun, 1 Jun 2025 15:33:33 -0700 Subject: [PATCH 5/9] update comment --- .../core/config/PolarisConfigurationStore.java | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java index c0341d5d84..78064b9a72 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java @@ -43,12 +43,11 @@ public interface PolarisConfigurationStore { * @param configName the name of the configuration key to check * @return the current value set for the configuration key or null if not set * @param the type of the configuration value - * @deprecated this function is going to be deprecated, please use the following function to get - * the configuration value in a more robust way: getConfiguration(String realm, String - * configName) This function can not be called outside of active request scope, such as - * background tasks (TaskExecutor). + *

This function needs to be used with caution, it can not be called outside of active + * request scope, such as background tasks (TaskExecutor). Please use the function + * getConfiguration(String realm, String configName) to get the configuration value in a more + * robust way. */ - @Deprecated default @Nullable T getConfiguration(PolarisCallContext ctx, String configName) { return null; } @@ -62,12 +61,11 @@ public interface PolarisConfigurationStore { * @param defaultValue the default value if the configuration key has no value * @return the current value or the supplied default value * @param the type of the configuration value - * @deprecated this function is going to be deprecated, please use the following function to get - * the configuration value in a more robust way: getConfiguration(String realm, String - * configName, @Nonnull T defaultValue) This function can not be called outside of active - * request scope, such as background tasks (TaskExecutor). + *

This function needs to be used with caution, it can not be called outside of active + * request scope, such as background tasks (TaskExecutor). Please use the function + * getConfiguration(String realm, String configName, @Nonnull T defaultValue) to get the + * configuration value in a more robust way. */ - @Deprecated default @Nonnull T getConfiguration( PolarisCallContext ctx, String configName, @Nonnull T defaultValue) { Preconditions.checkNotNull(defaultValue, "Cannot pass null as a default value"); From a1dc76157623768d6de603dde85ef462da5e537b Mon Sep 17 00:00:00 2001 From: Yun Zou Date: Mon, 2 Jun 2025 10:25:57 -0700 Subject: [PATCH 6/9] add todo --- .../polaris/core/config/PolarisConfigurationStore.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java index 78064b9a72..a0a180bdea 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java @@ -46,7 +46,7 @@ public interface PolarisConfigurationStore { *

This function needs to be used with caution, it can not be called outside of active * request scope, such as background tasks (TaskExecutor). Please use the function * getConfiguration(String realm, String configName) to get the configuration value in a more - * robust way. + * robust way. TODO: Remove this function and replace the usage with the function takes realm */ default @Nullable T getConfiguration(PolarisCallContext ctx, String configName) { return null; @@ -64,7 +64,8 @@ public interface PolarisConfigurationStore { *

This function needs to be used with caution, it can not be called outside of active * request scope, such as background tasks (TaskExecutor). Please use the function * getConfiguration(String realm, String configName, @Nonnull T defaultValue) to get the - * configuration value in a more robust way. + * configuration value in a more robust way. TODO: Remove this function and replace the usage + * with the function takes realm */ default @Nonnull T getConfiguration( PolarisCallContext ctx, String configName, @Nonnull T defaultValue) { From e417244ec8814b0545ee04ba3f4d83a907cf72e6 Mon Sep 17 00:00:00 2001 From: Yun Zou Date: Mon, 2 Jun 2025 11:48:58 -0700 Subject: [PATCH 7/9] update interface --- .../config/PolarisConfigurationStore.java | 20 ++++++++++--------- .../config/DefaultConfigurationStoreTest.java | 11 ++++++---- .../service/catalog/io/FileIOUtil.java | 2 +- .../config/DefaultConfigurationStore.java | 6 +++--- .../service/task/TableCleanupTaskHandler.java | 14 ++++--------- 5 files changed, 26 insertions(+), 27 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java index a0a180bdea..d59a84859a 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java @@ -25,6 +25,7 @@ import java.util.List; import java.util.Map; import org.apache.polaris.core.PolarisCallContext; +import org.apache.polaris.core.context.RealmContext; import org.apache.polaris.core.entity.CatalogEntity; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -45,8 +46,9 @@ public interface PolarisConfigurationStore { * @param the type of the configuration value *

This function needs to be used with caution, it can not be called outside of active * request scope, such as background tasks (TaskExecutor). Please use the function - * getConfiguration(String realm, String configName) to get the configuration value in a more - * robust way. TODO: Remove this function and replace the usage with the function takes realm + * getConfiguration(RealmContext realmContext, String configName) to get the configuration + * value in a more robust way. TODO: Remove this function and replace the usage with the + * function takes realm. */ default @Nullable T getConfiguration(PolarisCallContext ctx, String configName) { return null; @@ -63,9 +65,9 @@ public interface PolarisConfigurationStore { * @param the type of the configuration value *

This function needs to be used with caution, it can not be called outside of active * request scope, such as background tasks (TaskExecutor). Please use the function - * getConfiguration(String realm, String configName, @Nonnull T defaultValue) to get the + * getConfiguration(RealmContext realmContext, String configName, T defaultValue) to get the * configuration value in a more robust way. TODO: Remove this function and replace the usage - * with the function takes realm + * with the function takes realm. */ default @Nonnull T getConfiguration( PolarisCallContext ctx, String configName, @Nonnull T defaultValue) { @@ -77,12 +79,12 @@ public interface PolarisConfigurationStore { /** * Retrieve the current value for a configuration key for a given realm. May be null if not set. * - * @param realm the realm identifier + * @param realmContext the realm context * @param configName the name of the configuration key to check * @return the current value set for the configuration key for the given realm, or null if not set * @param the type of the configuration value */ - default @Nullable T getConfiguration(String realm, String configName) { + default @Nullable T getConfiguration(@Nonnull RealmContext realmContext, String configName) { return null; } @@ -90,16 +92,16 @@ public interface PolarisConfigurationStore { * Retrieve the current value for a configuration key for the given realm. If not set, return the * non-null default value. * - * @param realm the realm identifier + * @param realmContext the realm context * @param configName the name of the configuration key to check * @param defaultValue the default value if the configuration key has no value * @return the current value or the supplied default value * @param the type of the configuration value */ default @Nonnull T getConfiguration( - String realm, String configName, @Nonnull T defaultValue) { + RealmContext realmContext, String configName, @Nonnull T defaultValue) { Preconditions.checkNotNull(defaultValue, "Cannot pass null as a default value"); - T configValue = getConfiguration(realm, configName); + T configValue = getConfiguration(realmContext, configName); return configValue != null ? configValue : defaultValue; } 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 03bc63994b..bfe91ad5a3 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 @@ -149,16 +149,19 @@ public void testGetRealmConfiguration() { void testGetConfigurationWithRealm() { // the falseByDefaultKey is set to `false` for all realms, but overwrite with value `true` for // realmOne. - assertThat((Boolean) configurationStore.getConfiguration(realmOne, falseByDefaultKey)).isTrue(); + assertThat((Boolean) configurationStore.getConfiguration(realmOneContext, falseByDefaultKey)) + .isTrue(); // the trueByDefaultKey is set to `false` for all realms, no overwrite for realmOne - assertThat((Boolean) configurationStore.getConfiguration(realmOne, trueByDefaultKey)).isTrue(); + assertThat((Boolean) configurationStore.getConfiguration(realmOneContext, trueByDefaultKey)) + .isTrue(); // the falseByDefaultKey is set to `false` for all realms, no overwrite for realmTwo - assertThat((Boolean) configurationStore.getConfiguration(realmTwo, falseByDefaultKey)) + assertThat((Boolean) configurationStore.getConfiguration(realmTwoContext, falseByDefaultKey)) .isFalse(); // the trueByDefaultKey is set to `false` for all realms, and overwrite with value `false` for // realmTwo - assertThat((Boolean) configurationStore.getConfiguration(realmTwo, trueByDefaultKey)).isFalse(); + assertThat((Boolean) configurationStore.getConfiguration(realmTwoContext, trueByDefaultKey)) + .isFalse(); } @Test diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java b/service/common/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java index a1961d60f8..0e62110765 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java @@ -87,7 +87,7 @@ public static AccessConfig refreshAccessConfig( boolean skipCredentialSubscopingIndirection = configurationStore.getConfiguration( - callContext.getRealmContext().getRealmIdentifier(), + callContext.getRealmContext(), FeatureConfiguration.SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION.key, FeatureConfiguration.SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION.defaultValue); if (skipCredentialSubscopingIndirection) { diff --git a/service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java b/service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java index 398e1c7bdf..8928291e82 100644 --- a/service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java +++ b/service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java @@ -50,7 +50,8 @@ public DefaultConfigurationStore( } @Override - public @Nullable T getConfiguration(String realm, String configName) { + public @Nullable T getConfiguration(@Nonnull RealmContext realmContext, String configName) { + String realm = realmContext.getRealmIdentifier(); LOGGER.debug("Get configuration value for {} with realm {}", configName, realm); @SuppressWarnings("unchecked") T confgValue = @@ -64,8 +65,7 @@ public DefaultConfigurationStore( public @Nullable T getConfiguration(@Nonnull PolarisCallContext ctx, String configName) { if (realmContextInstance.isResolvable()) { RealmContext realmContext = realmContextInstance.get(); - String realm = realmContext.getRealmIdentifier(); - return getConfiguration(realm, configName); + return getConfiguration(realmContext, configName); } else { LOGGER.debug( "No RealmContext is injected when lookup value for configuration {} ", configName); diff --git a/service/common/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java b/service/common/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java index cc34120711..0fd353c51e 100644 --- a/service/common/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java +++ b/service/common/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java @@ -115,13 +115,7 @@ public boolean handleTask(TaskEntity cleanupTask, CallContext callContext) { Stream metadataFileCleanupTasks = getMetadataTaskStream( - cleanupTask, - tableMetadata, - fileIO, - tableEntity, - metaStoreManager, - polarisCallContext, - callContext.getRealmContext().getRealmIdentifier()); + cleanupTask, tableMetadata, fileIO, tableEntity, metaStoreManager, callContext); List taskEntities = Stream.concat(manifestCleanupTasks, metadataFileCleanupTasks).toList(); @@ -207,12 +201,12 @@ private Stream getMetadataTaskStream( FileIO fileIO, IcebergTableLikeEntity tableEntity, PolarisMetaStoreManager metaStoreManager, - PolarisCallContext polarisCallContext, - String realm) { + CallContext callContext) { + PolarisCallContext polarisCallContext = callContext.getPolarisCallContext(); int batchSize = polarisCallContext .getConfigurationStore() - .getConfiguration(realm, BATCH_SIZE_CONFIG_KEY, 10); + .getConfiguration(callContext.getRealmContext(), BATCH_SIZE_CONFIG_KEY, 10); return getMetadataFileBatches(tableMetadata, batchSize).stream() .map( metadataBatch -> { From d8a01cd42164cae3b5b7bb977e5c8929d42776c8 Mon Sep 17 00:00:00 2001 From: Yun Zou Date: Mon, 2 Jun 2025 12:16:49 -0700 Subject: [PATCH 8/9] update comment --- .../config/PolarisConfigurationStore.java | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java index d59a84859a..42e1962df6 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java @@ -40,16 +40,17 @@ public interface PolarisConfigurationStore { /** * Retrieve the current value for a configuration key. May be null if not set. * + *

This function will be deprecated, it can not be called outside of active request scope, such + * as background tasks (TaskExecutor). Please use the function getConfiguration(RealmContext + * realmContext, String configName) to get the configuration value in a more robust way. TODO: + * Remove this function and replace the usage with the function takes realm. + * * @param ctx the current call context * @param configName the name of the configuration key to check * @return the current value set for the configuration key or null if not set * @param the type of the configuration value - *

This function needs to be used with caution, it can not be called outside of active - * request scope, such as background tasks (TaskExecutor). Please use the function - * getConfiguration(RealmContext realmContext, String configName) to get the configuration - * value in a more robust way. TODO: Remove this function and replace the usage with the - * function takes realm. */ + @Deprecated default @Nullable T getConfiguration(PolarisCallContext ctx, String configName) { return null; } @@ -58,17 +59,17 @@ public interface PolarisConfigurationStore { * Retrieve the current value for a configuration key. If not set, return the non-null default * value. * + *

This function will be deprecated, it can not be called outside of active request scope, such + * as background tasks (TaskExecutor). Please use the function getConfiguration(RealmContext + * realmContext, String configName, T defaultValue) to get the configuration value in a more + * robust way. TODO: Remove this function and replace the usage with the function takes realm. + * * @param ctx the current call context * @param configName the name of the configuration key to check * @param defaultValue the default value if the configuration key has no value * @return the current value or the supplied default value - * @param the type of the configuration value - *

This function needs to be used with caution, it can not be called outside of active - * request scope, such as background tasks (TaskExecutor). Please use the function - * getConfiguration(RealmContext realmContext, String configName, T defaultValue) to get the - * configuration value in a more robust way. TODO: Remove this function and replace the usage - * with the function takes realm. */ + @Deprecated default @Nonnull T getConfiguration( PolarisCallContext ctx, String configName, @Nonnull T defaultValue) { Preconditions.checkNotNull(defaultValue, "Cannot pass null as a default value"); @@ -130,7 +131,8 @@ public interface PolarisConfigurationStore { } /** - * Retrieve the current value for a configuration. + * Retrieve the current value for a configuration. TODO: update the function to take RealmContext + * instead of PolarisCallContext * * @param ctx the current call context * @param config the configuration to load @@ -144,7 +146,7 @@ public interface PolarisConfigurationStore { /** * Retrieve the current value for a configuration, overriding with a catalog config if it is - * present. + * present. TODO: update the function to take RealmContext instead of PolarisCallContext * * @param ctx the current call context * @param catalogEntity the catalog to check for an override From a9fa5c8b9a2bda6aa2ef05a937a41fbe0660920e Mon Sep 17 00:00:00 2001 From: Yun Zou Date: Mon, 2 Jun 2025 12:23:36 -0700 Subject: [PATCH 9/9] update commebt --- .../polaris/core/config/PolarisConfigurationStore.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java index 42e1962df6..719ef311c8 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java @@ -43,7 +43,8 @@ public interface PolarisConfigurationStore { *

This function will be deprecated, it can not be called outside of active request scope, such * as background tasks (TaskExecutor). Please use the function getConfiguration(RealmContext * realmContext, String configName) to get the configuration value in a more robust way. TODO: - * Remove this function and replace the usage with the function takes realm. + * Remove this function and replace the usage with the function takes realm. Github issue + * https://github.com/apache/polaris/issues/1775 * * @param ctx the current call context * @param configName the name of the configuration key to check @@ -63,6 +64,7 @@ public interface PolarisConfigurationStore { * as background tasks (TaskExecutor). Please use the function getConfiguration(RealmContext * realmContext, String configName, T defaultValue) to get the configuration value in a more * robust way. TODO: Remove this function and replace the usage with the function takes realm. + * Github issue https://github.com/apache/polaris/issues/1775 * * @param ctx the current call context * @param configName the name of the configuration key to check @@ -132,7 +134,7 @@ public interface PolarisConfigurationStore { /** * Retrieve the current value for a configuration. TODO: update the function to take RealmContext - * instead of PolarisCallContext + * instead of PolarisCallContext. Github issue https://github.com/apache/polaris/issues/1775 * * @param ctx the current call context * @param config the configuration to load @@ -146,7 +148,8 @@ public interface PolarisConfigurationStore { /** * Retrieve the current value for a configuration, overriding with a catalog config if it is - * present. TODO: update the function to take RealmContext instead of PolarisCallContext + * present. TODO: update the function to take RealmContext instead of PolarisCallContext Github + * issue https://github.com/apache/polaris/issues/1775 * * @param ctx the current call context * @param catalogEntity the catalog to check for an override