Introduce RealmConfigurationSource by dimas-b · Pull Request #3573 · apache/polaris

dimas-b · 2026-01-27T19:54:19Z

The existing PolarisConfigurationStore interface is a fixture of SPI methods providing config values from the environment and utility methods for various lookup parameter permutations and type casts.

This change adds RealmConfigurationSource for the SPI part and moves lookup logic into RealmConfigImpl.

Note that existing service code always accesses config via RealmConfig.

The old PolarisConfigurationStore interface remains for backward compatibility and but redirects actual config lookup to RealmConfigImpl.

Checklist

🛡️ Don't disclose security issues! (contact security@apache.org)
🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
🧪 Added/updated tests with good coverage, or manually tested (and explained how)
💡 Added comments for complex logic
🧾 Updated CHANGELOG.md (if needed)
📚 Updated documentation in site/content/in-dev/unreleased (if needed)

snazy · 2026-02-04T13:24:51Z

polaris-core/src/main/java/org/apache/polaris/core/config/RealmConfigurationSource.java

+public interface RealmConfigurationSource {
+  RealmConfigurationSource EMPTY_CONFIG = (rc, name) -> null;
+
+  static RealmConfigurationSource global(Map<String, ?> config) {


Suggested change

static RealmConfigurationSource global(Map<String, ?> config) {

@VisibleForTesting

static RealmConfigurationSource global(Map<String, ?> config) {

?

snazy · 2026-02-04T13:31:50Z

polaris-core/src/main/java/org/apache/polaris/core/config/RealmConfigImpl.java

+    } else if (config.defaultValue() instanceof Double) {
+      return config.cast(Double.valueOf(String.valueOf(value)));
+    } else if (config.defaultValue() instanceof List<?>) {
+      return config.cast(new ArrayList<>((List<?>) value));


Not new to this PR, but shouldn't this yield an immutable/non-modifiable list instead?

Probably, but I did not want to mix pure refactoring with logic changes 😅

snazy · 2026-02-04T13:34:11Z

polaris-core/src/main/java/org/apache/polaris/core/config/RealmConfigImpl.java

+    }
+
+    if (config.defaultValue() instanceof Boolean) {
+      return config.cast(Boolean.valueOf(String.valueOf(value)));


Not new to this PR:
Looks like this function converts every value to a string and then parses that string even if the type matches, which feels overly expensive.

I did not want to mix pure refactoring with logic changes 😅 This code was moved "as is".

Yea - but I think it needs to be tackled in a follow-up.
Configurations are often used on hot code path.
Same for below.

adutra · 2026-02-05T16:35:32Z

polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java

+   * @deprecated Use {@link RealmConfig}.
   */
+  @SuppressWarnings({"DeprecatedIsStillUsed", "removal"})
+  @Deprecated(forRemoval = true)


Should the deprecation target the whole class?

Good point 👍

adutra · 2026-02-05T16:39:26Z

...ris-core/src/test/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheTest.java

-                  }
-                },
+                (rc, name) ->
+                    Map.of(INCLUDE_PRINCIPAL_NAME_IN_SUBSCOPED_CREDENTIAL.key(), "true").get(name),


nit: isn't this simpler?

Suggested change

Map.of(INCLUDE_PRINCIPAL_NAME_IN_SUBSCOPED_CREDENTIAL.key(), "true").get(name),

INCLUDE_PRINCIPAL_NAME_IN_SUBSCOPED_CREDENTIAL.key().equals(name) ? "true" : null,

I've seen other similar occurrences in this PR.

With our formatter, current code is 1 line, the proposed code will take 3 lines 😅

Maybe, but Alex' way is clearer.

(Troll: Without a formatter, we could squeeze the whole file into a single line :P )

The existing `PolarisConfigurationStore` interface is a fixture of SPI methods providing config values from the environment and utility methods for various lookup parameter permutations and type casts. This change adds `RealmConfigurationSource` for the SPI part and moves lookup logic into `RealmConfigImpl`. Note that existing service code always accesses config via `RealmConfig`. The old `PolarisConfigurationStore` interface remain for backward compatibility and but redirects actual config lookup to `RealmConfigImpl`.

dimas-b · 2026-02-05T19:14:57Z

polaris-core/src/main/java/org/apache/polaris/core/PolarisCallContext.java

      @Nonnull RealmContext realmContext,
      @Nonnull BasePersistence metaStore,
-      @Nonnull PolarisConfigurationStore configurationStore) {
+      @Nonnull org.apache.polaris.core.config.PolarisConfigurationStore configurationStore) {


note: avoiding imports because IntelliJ warns about deprecation on the import statement and that warning cannot be suppressed 🤷

IntelliJ warns about deprecation on the import statement

This is strange, because that Java 8 legacy was removed via JEP 211.

snazy · 2026-02-06T06:58:43Z

polaris-core/src/main/java/org/apache/polaris/core/PolarisCallContext.java

      @Nonnull RealmContext realmContext,
      @Nonnull BasePersistence metaStore,
-      @Nonnull PolarisConfigurationStore configurationStore) {
+      @Nonnull org.apache.polaris.core.config.PolarisConfigurationStore configurationStore) {


IntelliJ warns about deprecation on the import statement

This is strange, because that Java 8 legacy was removed via JEP 211.

snazy · 2026-02-06T06:59:46Z

polaris-core/src/main/java/org/apache/polaris/core/config/RealmConfigImpl.java

+    }
+
+    if (config.defaultValue() instanceof Boolean) {
+      return config.cast(Boolean.valueOf(String.valueOf(value)));


Yea - but I think it needs to be tackled in a follow-up.
Configurations are often used on hot code path.
Same for below.

snazy · 2026-02-06T07:02:27Z

...ris-core/src/test/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheTest.java

-                  }
-                },
+                (rc, name) ->
+                    Map.of(INCLUDE_PRINCIPAL_NAME_IN_SUBSCOPED_CREDENTIAL.key(), "true").get(name),


Maybe, but Alex' way is clearer.

(Troll: Without a formatter, we could squeeze the whole file into a single line :P )

snazy · 2026-02-06T07:08:46Z

...service/src/main/java/org/apache/polaris/service/config/PolarisConfigurationStoreBridge.java

+
+@SuppressWarnings("removal")
+@ApplicationScoped
+public class PolarisConfigurationStoreBridge


What's the use case of this application-scoped bean?
I only see one usage in a test. Maybe refactor the test or move this type to testFixtures.

dimas-b requested review from adutra, dennishuo and snazy January 27, 2026 19:54

github-project-automation bot added this to Basic Kanban Board Jan 27, 2026

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Jan 27, 2026

dimas-b mentioned this pull request Jan 27, 2026

Refactor PolarisConfigurationStore to streamline value typing #3324

Closed

6 tasks

snazy reviewed Feb 4, 2026

View reviewed changes

dimas-b force-pushed the realm-config-source branch from 8e18796 to 075a34c Compare February 4, 2026 18:00

adutra previously approved these changes Feb 5, 2026

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Feb 5, 2026

dimas-b added 3 commits February 5, 2026 13:34

review: inline RealmConfigurationSource.global()

3f69904

review: deprecate PolarisConfigurationStore

2d6a1ab

dimas-b dismissed adutra’s stale review via 2d6a1ab February 5, 2026 19:12

dimas-b force-pushed the realm-config-source branch from 075a34c to 2d6a1ab Compare February 5, 2026 19:12

dimas-b commented Feb 5, 2026

View reviewed changes

snazy reviewed Feb 6, 2026

View reviewed changes

	static RealmConfigurationSource global(Map<String, ?> config) {
	@VisibleForTesting
	static RealmConfigurationSource global(Map<String, ?> config) {

	Map.of(INCLUDE_PRINCIPAL_NAME_IN_SUBSCOPED_CREDENTIAL.key(), "true").get(name),
	INCLUDE_PRINCIPAL_NAME_IN_SUBSCOPED_CREDENTIAL.key().equals(name) ? "true" : null,

Conversation

dimas-b commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dimas-b commented Jan 27, 2026 •

edited

Loading