Rename SUPPORTED_CATALOG_CONNECTION_TYPES #1959

poojanilangekar · 2025-06-26T23:25:44Z

To ensure backward compatibility, process to rename SUPPORTED_CATALOG_CONNECTION_TYPES will consist of two steps.

Add SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES.
Remove the legacy SUPPORTED_CATALOG_CONNECTION_TYPES.

As discussed in PR #1931 , for a while Polaris will initially support both SUPPORTED_CATALOG_CONNECTION_TYPES and SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES. Later, we will remove the SUPPORTED_CATALOG_CONNECTION_TYPES to make it consistent with AUTHENTICATION_TYPES.

To support the rename, this PR introduces a new PolarisConfiguration parameter called legacyKey.

poojanilangekar · 2025-06-26T23:30:15Z

CC @dimas-b @eric-maynard

dimas-b · 2025-06-27T00:19:26Z

Let's hold this until #1952 merges so that we could properly track the property rename in CHANGELOG.

dimas-b · 2025-06-28T00:58:11Z

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

@@ -266,10 +266,18 @@ public static void enforceFeatureEnabledOrThrow(
  public static final FeatureConfiguration<List<String>> SUPPORTED_CATALOG_CONNECTION_TYPES =
      PolarisConfiguration.<List<String>>builder()
          .key("SUPPORTED_CATALOG_CONNECTION_TYPES")
-          .description("The list of supported catalog connection types for federation")
+          .description("(Deprecated) The list of supported catalog connection types for federation")


I believe it would be more maintainable to have one FeatureConfiguration object which supported both the old and the new key names (cf. catalogConfigUnsafe()).

I see a few options:

Introduce a framework to support multiple config names, with some of them marked for deprecation, similar to catalogConfigUnsafe

Just leave the name alone

Just make the breaking change and switch the config name

Change the variable name but not the config name

Add a second config with a new name, change the logic to take the union of the two, and mark the old one deprecated

I think all of these are fine but (1) is definitely the most work. Looks like this is (5) but this random (Deprecated) comment is doing a lot of heavy lifting. We don't get logging like we would get with (1) and the code is less clean.

Done, I've implemented 1.

dimas-b

Please rebase and add a CHANGELOG note about the config property rename (probably under "Deprecations")

dimas-b · 2025-06-30T18:15:49Z

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

+      SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES =
+          PolarisConfiguration.<List<String>>builder()
+              .key("SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES")
+              .deprecatedKey("SUPPORTED_CATALOG_CONNECTION_TYPES")


I'd prefer a name like .legacyKey() or .compatibilityKey() for the sake of clarity. "Deprecated" key as a builder parameter does not convey a reason for this key to exist.

I think if we're really doing this we need to take multiple keys, so like
PolarisConfiguration.<>builder().keys(...). Maybe that should take some ConfigurationKey, or maybe we should have some other mechanism for marking which are deprecated.

Multiple keys SGTM.... but from the builder API POV, we could just accept multiple calls to .key(), .deprecatedKey() (or .legacyKey()), right?

I can make the changes. Just so we are on the same page, does the following approach make sense to both of you?:

Support a single key per config, because ideally at any given time, we want at most one active key per configuration.

Support multiple legacyKeys per config. So we can make multiple calls to legacyKey() while building a config.

Whether we use one key call that takes N strings or we allow N chained calls to key / deprecatedKey, the point is we should support N different keys rather than 2.

dimas-b · 2025-06-30T18:18:57Z

runtime/defaults/src/main/resources/application.properties

@@ -112,6 +112,7 @@ polaris.features."ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING"=false
 polaris.features."SUPPORTED_CATALOG_STORAGE_TYPES"=["S3","GCS","AZURE"]
 # polaris.features."ENABLE_CATALOG_FEDERATION"=true
 polaris.features."SUPPORTED_CATALOG_CONNECTION_TYPES"=["ICEBERG_REST"]
+polaris.features."ENABLE_EXTERNAL_CATALOG_CONNECTION_TYPES"=["ICEBERG_REST"]


Is this change relevant to current PR's scope (and title)?

Sorry about that. It was added by auto complete. I have removed it.

dimas-b

Current changes in this PR LGTM 👍 I'd be ok with merging now and enhancing later, but I'm also fine with implementing @eric-maynard 's suggestions in this PR.

poojanilangekar · 2025-07-02T15:04:22Z

Hi @eric-maynard and @dimas-b can you please take a look at the change to support multiple legacyKeys?

CHANGELOG.md

dimas-b · 2025-07-02T17:49:40Z

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

@@ -41,6 +43,7 @@ public abstract class PolarisConfiguration<T> {

  public final String key;
  public final String description;
+  public final Set<String> legacyKeys;


Is this value used for property config value lookup? Sorry, if I missed it in the diff 🤔

Yes, it is used when registering a new configuration.

Previously (before this change), we would go over existingConfigurations check if it matched any existingConfiguration's key. Now, we check if the new configuration (both key and legacyKeys) match either an existing key (line 61) or existing legacyKeys (line 65 - 76)

Correct... but we should also use it to find config values in case the key is not found, right?

The way I have implemented it, any configuration always has a key(I think that's a valid requirement). Is there a reason we would we keep a configuration with just legacyKeys?

Actually, I think I get what you mean. I am guessing you meant the aspect of reading in the properties. If so I will send a fix shortly.

Thx! To clarify: old config entries should keep working until we drop support for them (e.g. in 2.0). Let's add a WARN log message when we have to fall back to the "legacy" property name so that users are notified in runtime and are more aware of the need to migrating to the new key.

Co-authored-by: Dmitri Bourlatchkov <[email protected]>

dimas-b

LGTM, thanks, @poojanilangekar !

dimas-b · 2025-07-02T20:51:24Z

...ris-core/src/test/java/org/apache/polaris/service/storage/PolarisConfigurationStoreTest.java

+  }
+
+  @Test
+  public void testLegacyKeyWarningIsLogged() {


How does this test ensure the warning is logged?

I'd be ok without this test too :)

This was my mistake, I should have removed it, I was trying to get the log warning but couldn't capture it. I've removed it now. (The functionality is still tested in testLegacyKeyLookup)

dimas-b · 2025-07-02T20:58:05Z

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

@@ -41,6 +43,7 @@ public abstract class PolarisConfiguration<T> {

  public final String key;
  public final String description;
+  public final Set<String> legacyKeys;


Sorry, I did not notice this before, but I think having public fiends in general is not a good idea. Since you're toughing this code in current PR, would you mind following up with another PR to convert them to private fields?

Sure, I will send out a follow up PR.

dimas-b · 2025-07-02T21:04:54Z

@eric-maynard : since you had previous comments, PTAL.

eric-maynard · 2025-07-04T00:21:07Z

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

+  public String legacyKeys() {
+    if (legacyKeys.isEmpty()) {
+      throw new IllegalStateException(
+          "Attempted to read legacy keys from a configuration that doesn't have any.");


This doesn't look right to me; the check is there for catalogConfig because it's not correct to call the variant of getConfiguration which takes a catalog when a config doesn't have a catalogConfig. However we should always be checking legacyKeys every time we load any config.

I've removed it.

eric-maynard · 2025-07-04T00:21:30Z

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

+    return !legacyKeys.isEmpty();
+  }
+
+  public String legacyKeys() {


I would not expect this to return a String either.

eric-maynard · 2025-07-04T00:22:13Z

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

+    if (result == null && config.hasLegacyKeys()) {
+      for (String legacyKey : config.legacyKeys) {


If legacyKeys just returned a set (or there was a keys that returned a set) then the if statement here goes away.

eric-maynard · 2025-07-04T00:23:52Z

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

+              "Legacy configuration key '{}' is in use. Please use '{}' instead.",
+              legacyKey,
+              config.key);
+          break;


Not sure we want a break here

If multiple legacy keys really are in use, we should want to log for each of them

Barring the above, you could just early return rather than break

I don't think we can use an early return directly because we need to try and cast the value to its defined type. We could duplicate the code here but I think this is cleaner.

The cast is just tryCast(config, result) which is not a crazy amount of logic to replicate. But I wouldn't advocate for the early return either, because of reason (1) above

Regarding (1), I don't think that is accurate, even with the default key, there is an order of precedence in which properties are applied. When multiple keys are found, Quarkus picks the first property that is encounters. So I'm unsure why we'd want to apply multiple legacy keys. Unless you're suggesting simply logging multiple legacy keys while only accepting the fist legacy key. Please let me know.

If multiple legacy keys really are in use, we should want to log for each of them

Yes, I'm referring to the logging.

eric-maynard · 2025-07-08T00:47:08Z

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

@@ -34,11 +35,12 @@ public class BehaviorChangeConfiguration<T> extends PolarisConfiguration<T> {

  protected BehaviorChangeConfiguration(
      String key,


Why can you have multiple legacyKeys but not multiple keys? It seems like maybe both should be Sets that support multiple calls.

Here is my understanding: A PolarisConfiguration or a configuration for any quarkus app has a name (key in PoalrisConfiguration) that is used to identify it. The key/name is the identity of the configuration. The point of adding support for legacyKeys was to enable identity changes wherein we can for certain time support deprecated keys. I don't see why we should have multiple identifying keys mapping to a single entity. If you ever need multiple identities, then the legacy keys do support the need but eventually each entity must converge to a single identity.

Please let me know if I am missing something here.

It just seems odd that we would support multiple invalid keys but not multiple valid keys. In my original comment here I wrote:

Introduce a framework to support multiple config names, with some of them marked for deprecation, similar to catalogConfigUnsafe

rather than ... with all but one marked for deprecation ... because I had this in mind.

I do agree that the situation where you'd have multiple valid configs seems contrived, but so does the situation where you'd have multiple invalid configs or really even one invalid config. So if we're overhauling the whole key system, I think it makes sense to go all the way.

I don't think we support multiple invalid keys. We track multiple legacy keys.

From a semantic perspective, a PrimaryKey in a RDBMS, a keyin a key-value store or a Java Map (or any other programming language) maps to a single row, value or entity. In all these examples, two keys never map to the same entity.

What we have with the legacyKeys is a way to track historic versions of the key-value pair. In any DBMS or KV store that supports multi-versioning, it's pretty normal to have multiple entries (multiple versions) for a single key. When the primaryKey/key changes, the DBMS/KV store adds a tombstone for the old record and creates a new record with the new key. While changing keys in PolarisConfig, we are essentially creating a new key-value pair in itself but its nice to track the older key, hence the support for legacy keys. Essentially one could rewrite the entire PolarisConfig class with a Map where the key is the active key and the value is everything else. In which case, we'd track the legacyKeys as a part of the value.

The key is not a primary key though. There's no RDBMS or KV store storing these; they describe entries in a config file. It seems reasonable to expect that multiple different entries could be used to configure the same feature.

Consider, too, that the current code needs to deal with a situation where the key has value true but the legacyKey has value false. Or two legacy keys have different values. Clearly, the key(s) is not unique and the code needs to handle multiple keys for a given feature.

I disagree with this:

It seems reasonable to expect that multiple different entries could be used to configure the same feature.

You can have two configurations that store the same logical value and then let the specific module decide upon how to reconcile any conflicting values. But the config service should not register two configurations for the same feature.

Regarding the case where the key is true and legacyKey is false. I don't see how it is any different from a default value being defined say FeatureConfiguration.java, a value being defined in application.properties and then a JVM system property. In this case the system property takes precedence. Similarly the key takes precedence over legacyKey.

I disagree with this:

It seems reasonable to expect that multiple different entries could be used to configure the same feature.

Your current PR has this functionality... except only partially:

Keys Legacy Keys Multiple Entries OK?

0 0 No Yes

0 1 No Yes

0 2+ Yes Yes

1 0 No Yes

1 1 Yes Yes

1 2+ Yes Yes

2 0 Yes No

2 1 Yes No

2 2+ Yes No

What is the rationale behind this asymmetry? If, as stated before, it is because the key is thought of as a primary key then this is just not correct. You can write a valid config that violates the "primary key constraint" by duplicating an entry.

The case with 0 keys is not possible. We can't define a PolarisConfig without a key. That was always the case. Unless you'd prefer if we annotate the field with @NonNull in the constructor.

You can write a valid config that violates the "primary key constraint" by duplicating an entry.

The registerConfiguration function will actually disallow duplicate entries and throw an IllegalArgumentException. That check is a part of the existing code, this PR is not change it. This PR goes further and checks that any existing key or legacyKey does not overlap with a new configuration that is being registered (again both key and legacyKeys).

I think we're looking at two entirely different aspects of a configuration.

Declaring the configuration in FeatureConfiguration or BehaviorChangeConfiguration.

Assigning values to a configuration which is done in application.properties / JVM arguments.

The two are orthogonal to each other. We should impose constraints on how PolarisConfigs are declared. We can't however impose constraints on how the user defines these configs. If the user does have multiple definitions of the same config, then we apply the one with the highest priority and discard the rest. If indeed we should disallow multiple definitions, then any config that has a default value defined should essentially disallow any overwrites in application.properties. AFAIK, Quarkus doesn't do that and there isn't a way for the app to determine such a condition.

eric-maynard · 2025-07-09T18:59:35Z

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

-    T result = getConfiguration(realmContext, config.key, config.defaultValue);
+    T result = getConfiguration(realmContext, config.key);
+
+    if (result == null && config.hasLegacyKeys()) {


So the key overrides the legacy keys?

eric-maynard · 2025-07-09T18:59:59Z

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

+    T result = getConfiguration(realmContext, config.key);
+
+    if (result == null && config.hasLegacyKeys()) {
+      for (String legacyKey : config.legacyKeys()) {


This will lead to nondeterministic behavior if two legacy keys for a feature have different values

eric-maynard · 2025-07-09T19:00:25Z

...ris-core/src/test/java/org/apache/polaris/service/storage/PolarisConfigurationStoreTest.java

+  }
+
+  @Test
+  public void testMainKeyTakesPrecedenceOverLegacyKeys() {


If this happens, we should probably actually error

eric-maynard · 2025-07-09T19:00:34Z

...ris-core/src/test/java/org/apache/polaris/service/storage/PolarisConfigurationStoreTest.java

@@ -186,4 +185,78 @@ public void testEntityOverrides() {
    Assertions.assertEquals(
        "entity-legacy4", store.getConfiguration(testRealmContext, entity, cfg.apply(4)));
  }
+
+  @Test


It would be nice to add a test for conflicting legacy keys

eric-maynard

This PR is now doing more than just a rename, and we should figure out what model we actually want to do for configs before going ahead with the change.

There's also a bug when multiple legacy keys are used for one config.

poojanilangekar · 2025-07-10T22:43:28Z

Hey @eric-maynard, I agree that this change has gotten larger than what I had initially planned. I am planning to close this PR because I think it's orthogonal to the goal of the PR.

If @dimas-b and you are amenable to making the breaking change and doing the rename, I can update this PR.

Otherwise, we can leave the config SUPPORTED_CATALOG_CONNECTION_TYPES as is, so I can spend more time on other tasks.

poojanilangekar · 2025-07-10T22:49:29Z

I'd prefer doing something similar to #2000. (The PR adds a deprecated comment on the existing property and creates a new one.)

dimas-b · 2025-07-10T22:55:11Z

I'd prefer using the old config name (for now) to making a breaking change.

Appreciate your effort on this PR @poojanilangekar !

dimas-b · 2025-07-10T22:58:40Z

This PR deals with Polaris-specific config, so I'm not sure #2000 is applicable to this rename "as is". The tricky part is that Polaris config can be realm-specific and we cannot provide redirects in those cases (realm names are unknown at compile time).

dimas-b · 2025-07-10T23:00:48Z

It is possible to have custom code to deal with Smallrye configs at a low level and provide property redirects, but I think it'll be much more complex in the end.

github-project-automation bot added this to Basic Kanban Board Jun 26, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Jun 26, 2025

dimas-b requested review from eric-maynard and dimas-b June 27, 2025 00:20

dimas-b reviewed Jun 28, 2025

View reviewed changes

dimas-b reviewed Jun 30, 2025

View reviewed changes

poojanilangekar changed the title ~~Rename SUPPORTED_CATALOG_CONNECTION_TYPES (1/2)~~ Rename SUPPORTED_CATALOG_CONNECTION_TYPES Jun 30, 2025

poojanilangekar added 6 commits June 30, 2025 12:46

Rename SUPPORTED_CATALOG_CONNECTION_TYPES (Part 1)

7febf6e

Update description

742b6c8

Add deprecateKey to PolarisConfiguration

7d8f87b

deprecatedKey -> legacyKey

3222567

rename in application.properties

0236466

Update changelog

c51edb7

poojanilangekar force-pushed the supported-connections branch from fe026fc to c51edb7 Compare June 30, 2025 19:49

dimas-b previously approved these changes Jun 30, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jun 30, 2025

Support multiple legacyKeys

9e79d08

poojanilangekar dismissed dimas-b’s stale review via 9e79d08 July 1, 2025 16:59

dimas-b reviewed Jul 2, 2025

View reviewed changes

poojanilangekar and others added 2 commits July 2, 2025 10:55

Update CHANGELOG.md

fa45fde

Co-authored-by: Dmitri Bourlatchkov <[email protected]>

Support lookups based on legacyKeys

2147b66

poojanilangekar requested a review from dimas-b July 2, 2025 20:58

dimas-b previously approved these changes Jul 2, 2025

View reviewed changes

Remove incorect WARN test

6087435

poojanilangekar dismissed dimas-b’s stale review via 6087435 July 2, 2025 21:10

dimas-b previously approved these changes Jul 2, 2025

View reviewed changes

eric-maynard reviewed Jul 4, 2025

View reviewed changes

Address review comments

14076b5

poojanilangekar dismissed dimas-b’s stale review via 14076b5 July 4, 2025 00:49

Log WARNINGS about multiple legacy keys

d99408a

poojanilangekar mentioned this pull request Jul 7, 2025

Make PolarisConfiguration member variables private #2007

Merged

eric-maynard reviewed Jul 8, 2025

View reviewed changes

eric-maynard reviewed Jul 9, 2025

View reviewed changes

eric-maynard requested changes Jul 10, 2025

View reviewed changes

github-project-automation bot moved this from Ready to merge to PRs In Progress in Basic Kanban Board Jul 10, 2025

poojanilangekar closed this Jul 10, 2025

github-project-automation bot moved this from PRs In Progress to Done in Basic Kanban Board Jul 10, 2025

		if (result == null && config.hasLegacyKeys()) {
		for (String legacyKey : config.legacyKeys) {

		@@ -34,11 +35,12 @@ public class BehaviorChangeConfiguration<T> extends PolarisConfiguration<T> {

		protected BehaviorChangeConfiguration(
		String key,

Keys	Legacy Keys	Multiple Entries	OK?
0	0	No	Yes
0	1	No	Yes
0	2+	Yes	Yes
1	0	No	Yes
1	1	Yes	Yes
1	2+	Yes	Yes
2	0	Yes	No
2	1	Yes	No
2	2+	Yes	No

Keys	Legacy Keys	Multiple Entries	OK?
0	0	No	Yes
0	1	No	Yes
0	2+	Yes	Yes
1	0	No	Yes
1	1	Yes	Yes
1	2+	Yes	Yes
2	0	Yes	No
2	1	Yes	No
2	2+	Yes	No

Rename SUPPORTED_CATALOG_CONNECTION_TYPES #1959

Rename SUPPORTED_CATALOG_CONNECTION_TYPES #1959

Uh oh!

Conversation

poojanilangekar commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poojanilangekar commented Jun 26, 2025

Uh oh!

dimas-b commented Jun 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-maynard Jun 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b left a comment

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!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

poojanilangekar commented Jul 2, 2025

Uh oh!

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!

poojanilangekar Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dimas-b left a comment

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!

dimas-b commented Jul 2, 2025

Uh oh!

Choose a reason for hiding this comment

poojanilangekar commented Jun 26, 2025 •

edited

Loading

eric-maynard Jun 28, 2025 •

edited

Loading

poojanilangekar Jul 2, 2025 •

edited

Loading

eric-maynard Jul 7, 2025 •

edited

Loading

Keys	Legacy Keys	Multiple Entries	OK?
0	0	No	Yes
0	1	No	Yes
0	2+	Yes	Yes
1	0	No	Yes
1	1	Yes	Yes
1	2+	Yes	Yes
2	0	Yes	No
2	1	Yes	No
2	2+	Yes	No