dimas-b commented on code in PR #1780:
URL: https://github.com/apache/polaris/pull/1780#discussion_r2122345231
##########
polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java:
##########
@@ -133,32 +133,31 @@ public interface PolarisConfigurationStore {
}
/**
- * 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
+ * Retrieve the current value for a configuration.
*
- * @param ctx the current call context
+ * @param realmContext the current realm context
* @param config the configuration to load
* @return the current value set for the configuration key or null if not set
* @param <T> the type of the configuration value
*/
- default <T> @Nonnull T getConfiguration(PolarisCallContext ctx,
PolarisConfiguration<T> config) {
- T result = getConfiguration(ctx, config.key, config.defaultValue);
+ default <T> @Nonnull T getConfiguration(
+ RealmContext realmContext, PolarisConfiguration<T> config) {
+ T result = getConfiguration(realmContext, config.key, config.defaultValue);
return tryCast(config, result);
}
/**
* 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 Github
- * issue https://github.com/apache/polaris/issues/1775
+ * present.
*
- * @param ctx the current call context
+ * @param realmContext the current realm context
* @param catalogEntity the catalog to check for an override
* @param config the configuration to load
* @return the current value set for the configuration key or null if not set
* @param <T> the type of the configuration value
*/
default <T> @Nonnull T getConfiguration(
- PolarisCallContext ctx,
+ RealmContext realmContext,
Review Comment:
nit: why not keep the old param name?
##########
polaris-core/src/test/java/org/apache/polaris/service/storage/PolarisConfigurationStoreTest.java:
##########
@@ -164,22 +163,25 @@ public void testEntityOverrides() {
PolarisConfigurationStore store =
new PolarisConfigurationStore() {
@Override
- public <T> @Nullable T getConfiguration(PolarisCallContext ctx,
String configName) {
+ public <T> @Nullable T getConfiguration(RealmContext realmContext,
String configName) {
//noinspection unchecked
return (T) Map.of("key2", "config-value2").get(configName);
}
};
- PolarisCallContext ctx = null;
CatalogEntity entity =
new CatalogEntity.Builder()
.addProperty("polaris.config.catalog-key3", "entity-new3")
.addProperty("legacy-key4", "entity-legacy4")
.build();
- Assertions.assertEquals("test-default1", store.getConfiguration(ctx,
entity, cfg.apply(1)));
- Assertions.assertEquals("config-value2", store.getConfiguration(ctx,
entity, cfg.apply(2)));
- Assertions.assertEquals("entity-new3", store.getConfiguration(ctx, entity,
cfg.apply(3)));
- Assertions.assertEquals("entity-legacy4", store.getConfiguration(ctx,
entity, cfg.apply(4)));
+ Assertions.assertEquals(
+ "test-default1", store.getConfiguration(testRealmContext, entity,
cfg.apply(1)));
Review Comment:
`testRealmContext` is not relevant here and does not affect the test's
behaviour. I'd prefer to keep `null`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
