Skip to content

Handle RequestScoped instance injection gracefully for DefaultConfigurationStore #1758

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jun 2, 2025
Merged

Handle RequestScoped instance injection gracefully for DefaultConfigurationStore #1758

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d8d760f
handle scope injection gracefully
gh-yzou May 30, 2025
4db92bf
updatelogger
gh-yzou May 30, 2025
d4acf9f
add new getConfiguration Context
gh-yzou May 31, 2025
15bc55f
add new api
gh-yzou Jun 1, 2025
e431b5f
update comment
gh-yzou Jun 1, 2025
a1dc761
add todo
gh-yzou Jun 2, 2025
e417244
update interface
gh-yzou Jun 2, 2025
d8a01cd
update comment
gh-yzou Jun 2, 2025
a9fa5c8
update commebt
gh-yzou Jun 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 48 additions & 3 deletions polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -39,11 +40,18 @@ public interface PolarisConfigurationStore {
/**
* Retrieve the current value for a configuration key. May be null if not set.
*
* <p>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. 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
* @return the current value set for the configuration key or null if not set
* @param <T> the type of the configuration value
*/
@Deprecated
default <T> @Nullable T getConfiguration(PolarisCallContext ctx, String configName) {
return null;
}
Expand All @@ -52,19 +60,54 @@ public interface PolarisConfigurationStore {
* Retrieve the current value for a configuration key. If not set, return the non-null default
* value.
*
* <p>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.
* 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
* @param defaultValue the default value if the configuration key has no value
* @return the current value or the supplied default value
* @param <T> the type of the configuration value
*/
@Deprecated
default <T> @Nonnull T getConfiguration(
PolarisCallContext ctx, String configName, @Nonnull T defaultValue) {
Preconditions.checkNotNull(defaultValue, "Cannot pass null as a default value");
T configValue = getConfiguration(ctx, configName);
return configValue != null ? configValue : defaultValue;
}

/**
* Retrieve the current value for a configuration key for a given realm. May be null if not set.
*
* @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 <T> the type of the configuration value
*/
default <T> @Nullable T getConfiguration(@Nonnull RealmContext realmContext, String configName) {
Copy link
Contributor

@dimas-b dimas-b Jun 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go with RealmContext, I believe it should be present in all methods.

As discussed in another thread, PolarisCallContext implies a particular realm, but does not expose the realm ID. Therefore, consistent behaviour is not guaranteed across methods using PolarisCallContext and RealmContext.

-1 This is a critical issue for this PR from my POV.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear: having convenience lookup methods based on CallContext would be fine, because we have CallContext.getRealmContext()

Copy link
Contributor

@dimas-b dimas-b Jun 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing -1 in the interest of making progress. There is only one implementation, which appears to processes both lookup call paths correctly (as far as I can tell from the diff). I'd be fine merging in this state after the other minor comments are addressed.

@gh-yzou : Would you mind opening a GH issue for followup?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PolarisCallContext is actually not used anywhere during getConfiguration, and only realmContext is needed. So i think we should deprecate the usage of the methods that uses PolarisCallContext, and i plan to remove the usages step by step in follow up PRs, remove all of them in one PR makes the change really big and touches persistent. i can mark the other APIs as deprecated for now if that helps, would that work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also added a TODO to all other functions that we will update them to use RealmContext instead of PolarisCallContext

Copy link
Contributor

@dimas-b dimas-b Jun 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with improving config lookup with a series of PRs. I'd still appreciate a GH issue for visibility. I believe it would be a 1.0 blocker due to large lookup API impact.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dimas-b yes, absolutely. i created an issue here #1775, i will also add this in the comment. That will be my next highest priority on my tasks

return null;
}

/**
* Retrieve the current value for a configuration key for the given realm. If not set, return the
* non-null default value.
*
* @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 <T> the type of the configuration value
*/
default <T> @Nonnull T getConfiguration(
RealmContext realmContext, String configName, @Nonnull T defaultValue) {
Preconditions.checkNotNull(defaultValue, "Cannot pass null as a default value");
T configValue = getConfiguration(realmContext, 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`
Expand All @@ -90,7 +133,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. Github issue https://github.com/apache/polaris/issues/1775
*
* @param ctx the current call context
* @param config the configuration to load
Expand All @@ -104,7 +148,8 @@ 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 Github
* issue https://github.com/apache/polaris/issues/1775
*
* @param ctx the current call context
* @param catalogEntity the catalog to check for an override
Expand Down
34 changes: 32 additions & 2 deletions ...rc/test/java/org/apache/polaris/service/quarkus/config/DefaultConfigurationStoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ public Map<String, String> getConfigOverrides() {
}

private PolarisCallContext polarisContext;
private RealmContext realmContext;

@Inject MetaStoreManagerFactory managerFactory;
@Inject PolarisConfigurationStore configurationStore;
Expand All @@ -87,8 +88,7 @@ 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 = () -> realmName;
polarisContext =
new PolarisCallContext(
managerFactory.getOrCreateSessionSupplier(realmContext).get(),
Expand All @@ -97,8 +97,16 @@ public void before(TestInfo testInfo) {
Clock.systemDefaultZone());
}

@Test
public void testGetConfigurationWithNoRealmContext() {
Assertions.assertThatThrownBy(
() -> configurationStore.getConfiguration(polarisContext, "missingKeyWithoutDefault"))
.isInstanceOf(IllegalStateException.class);
}

@Test
public void testGetConfiguration() {
QuarkusMock.installMockForType(realmContext, RealmContext.class);
Object value = configurationStore.getConfiguration(polarisContext, "missingKeyWithoutDefault");
assertThat(value).isNull();
Object defaultValue =
Expand Down Expand Up @@ -137,8 +145,28 @@ 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(realmOneContext, falseByDefaultKey))
.isTrue();
// the trueByDefaultKey is set to `false` for all realms, no overwrite for realmOne
assertThat((Boolean) configurationStore.getConfiguration(realmOneContext, trueByDefaultKey))
.isTrue();

// the falseByDefaultKey is set to `false` for all realms, no overwrite for realmTwo
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(realmTwoContext, trueByDefaultKey))
.isFalse();
}

@Test
public void testInjectedConfigurationStore() {
QuarkusMock.installMockForType(realmContext, RealmContext.class);
// the default value for trueByDefaultKey is `true`
boolean featureDefaultValue =
configurationStore.getConfiguration(polarisContext, trueByDefaultKey);
Expand All @@ -159,6 +187,7 @@ public void testInjectedConfigurationStore() {

@Test
public void testInjectedFeaturesConfiguration() {
QuarkusMock.installMockForType(realmContext, RealmContext.class);
assertThat(featuresConfiguration).isInstanceOf(QuarkusResolvedFeaturesConfiguration.class);

assertThat(featuresConfiguration.defaults())
Expand All @@ -179,6 +208,7 @@ public void testInjectedFeaturesConfiguration() {

@Test
public void testRegisterAndUseFeatureConfigurations() {
QuarkusMock.installMockForType(realmContext, RealmContext.class);
String prefix = "testRegisterAndUseFeatureConfigurations";

FeatureConfiguration<Boolean> safeConfig =
Expand Down
2 changes: 1 addition & 1 deletion service/common/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public static AccessConfig refreshAccessConfig(

boolean skipCredentialSubscopingIndirection =
configurationStore.getConfiguration(
callContext.getPolarisCallContext(),
callContext.getRealmContext(),
FeatureConfiguration.SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION.key,
FeatureConfiguration.SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION.defaultValue);
if (skipCredentialSubscopingIndirection) {
Expand Down
23 changes: 14 additions & 9 deletions ...ice/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,18 +49,23 @@ public DefaultConfigurationStore(
this.realmOverrides = Map.copyOf(configurations.parseRealmOverrides(objectMapper));
}

@Override
public <T> @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 =
(T)
Optional.ofNullable(realmOverrides.getOrDefault(realm, Map.of()).get(configName))
.orElseGet(() -> getDefaultConfiguration(configName));
return confgValue;
}

@Override
public <T> @Nullable T getConfiguration(@Nonnull PolarisCallContext ctx, String configName) {
if (!realmContextInstance.isUnsatisfied()) {
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;
return getConfiguration(realmContext, configName);
} else {
LOGGER.debug(
"No RealmContext is injected when lookup value for configuration {} ", configName);
Expand Down
12 changes: 4 additions & 8 deletions service/common/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,12 +115,7 @@ public boolean handleTask(TaskEntity cleanupTask, CallContext callContext) {

Stream<TaskEntity> metadataFileCleanupTasks =
getMetadataTaskStream(
cleanupTask,
tableMetadata,
fileIO,
tableEntity,
metaStoreManager,
polarisCallContext);
cleanupTask, tableMetadata, fileIO, tableEntity, metaStoreManager, callContext);

List<TaskEntity> taskEntities =
Stream.concat(manifestCleanupTasks, metadataFileCleanupTasks).toList();
Expand Down Expand Up @@ -206,11 +201,12 @@ private Stream<TaskEntity> getMetadataTaskStream(
FileIO fileIO,
IcebergTableLikeEntity tableEntity,
PolarisMetaStoreManager metaStoreManager,
PolarisCallContext polarisCallContext) {
CallContext callContext) {
PolarisCallContext polarisCallContext = callContext.getPolarisCallContext();
int batchSize =
polarisCallContext
.getConfigurationStore()
.getConfiguration(polarisCallContext, BATCH_SIZE_CONFIG_KEY, 10);
.getConfiguration(callContext.getRealmContext(), BATCH_SIZE_CONFIG_KEY, 10);
return getMetadataFileBatches(tableMetadata, batchSize).stream()
.map(
metadataBatch -> {
Expand Down
Loading