MetaStoreManagerFactory: make purgeRealms() return purge results #889

adutra · 2025-01-27T17:40:45Z

Make the purgeRealms method return the purge results.
These changes are a prerequisite for #878 and #887.

snazy · 2025-01-27T19:02:08Z

...e/src/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java

-                realmId, metaStoreManagerMap.get(realmId.id()));
-        results.put(realmId.id(), secretsResult);
+                realm, metaStoreManagerMap.get(realm.id()));
+        results.put(ImmutableRealmId.copyOf(realm), secretsResult);


why copying? it's immutable

It's RequestScoped, so I think it can get lost.

It's potentially a CDI bean, and these are poor choices as map keys: the CDI specs do not say anything about how CDI proxies should handle equals() and hashCode(). Such beans can have surprising effects when put in a set or a map.

I worry that this is an extremely easy mistake to make. I dislike proxies for this and other reasons. A person has to have a lot of context in their head before they do simple things like add keys to maps.

@eric-maynard @adutra fair point. However, the realm-IDs here are parameters for bootstrap?

Unfortunately the bootstrapRealms method can be called both from a bootstrap command (in which case RealmId is not a CDI proxy) and from "regular" server code (in which case it is a proxy). (The "regular" path only happens with the in-memory metastore though.)

eric-maynard · 2025-01-27T20:22:08Z

quarkus/admin/src/main/java/org/apache/polaris/admintool/RealmIdConverter.java

+import org.apache.polaris.core.context.RealmId;
+import picocli.CommandLine.ITypeConverter;
+
+public class RealmIdConverter implements ITypeConverter<RealmId> {


This class seems like it does nothing

It's used by Picocli to automatically convert Strings to RealmId.

I see, and what does that do?

It's removed anyways.

eric-maynard · 2025-01-27T20:22:16Z

quarkus/service/src/main/java/org/apache/polaris/service/quarkus/context/RealmIdConverter.java

+import org.apache.polaris.core.context.RealmId;
+import org.eclipse.microprofile.config.spi.Converter;
+
+public class RealmIdConverter implements Converter<RealmId> {


Same but this one is used by Quarkus / MicroProfile.

eric-maynard · 2025-01-27T20:22:54Z

...main/java/org/apache/polaris/service/persistence/InMemoryPolarisMetaStoreManagerFactory.java

-    for (String realmId : realms) {
-      PrincipalSecretsResult principalSecrets = results.get(realmId);
+    for (RealmId realm : results.keySet()) {
+      bootstrappedRealms.add(realm.id());


Shouldn't this use RealmId not String, so we don't need to call id()?

See my previous comment: CDI beans as map keys or set elements would have surprising effects here, notably containsKey() and addAll() wouldn't work as expected. We would need to copy the RealmId each time to be sure. In the end I think it's not worth it since these maps and collections are not public.

collado-mike

no general opposition to the PR, but it does highlight some concern regarding the use of RealmId as a plain ole java object. There's now some assumed base framework knowledge, that, AFACIT, isn't documented anywhere and will likely lead to issues in the future.

I also think we are missing a lot of basic javadocs that will help those of us who are ramping up on the frameworks being used here.

collado-mike · 2025-01-28T05:35:39Z

...e/src/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java

-                realmId, metaStoreManagerMap.get(realmId.id()));
-        results.put(realmId.id(), secretsResult);
+                realm, metaStoreManagerMap.get(realm.id()));
+        results.put(ImmutableRealmId.copyOf(realm), secretsResult);


I worry that this is an extremely easy mistake to make. I dislike proxies for this and other reasons. A person has to have a lot of context in their head before they do simple things like add keys to maps.

collado-mike · 2025-01-28T05:37:23Z

quarkus/admin/src/main/java/org/apache/polaris/admintool/RealmIdConverter.java

+import org.apache.polaris.core.context.RealmId;
+import picocli.CommandLine.ITypeConverter;
+
+public class RealmIdConverter implements ITypeConverter<RealmId> {


Javadoc comments are extremely helpful for people ramping up on the frameworks being used here

collado-mike · 2025-01-28T05:38:23Z

quarkus/service/src/main/java/org/apache/polaris/service/quarkus/context/RealmIdConverter.java

+import org.apache.polaris.core.context.RealmId;
+import org.eclipse.microprofile.config.spi.Converter;
+
+public class RealmIdConverter implements Converter<RealmId> {


Same re: javadoc comments

adutra · 2025-01-28T13:01:27Z

@collado-mike @eric-maynard @snazy

Since the mailing list discussion around RealmId is still ongoing, and given that you raised concerns about using RealmId as map keys or set elements:

Would it be better for you if we keep using String ids in the bootstrap/purge parts of the MetaStoreManagerFactory API?

I think I could make it work. The only change I really need to keep working on #887 is that purgeRealms should return the purge results.

snazy · 2025-01-29T09:14:56Z

Would it be better for you if we keep using String ids in the bootstrap/purge parts of the MetaStoreManagerFactory API?

WFM

collado-mike · 2025-01-31T17:28:47Z

Would it be better for you if we keep using String ids in the bootstrap/purge parts of the MetaStoreManagerFactory API?

I certainly like typed keys over String keys, but my worry is that it will be too easy to make mistakes. If it was a POJO, I'd say use the typed key, but I don't think people should have to wonder about whether a Realm passed in as a method argument was originally a CDI bean or if it's safe to use.

adutra · 2025-02-01T12:16:59Z

Alright, I reverted the map key changes and went back to String keys.

adutra · 2025-02-04T11:22:43Z

PR rebased.

With the revert from RealmId back to RealmContext, the scope of this PR faded to almost nothing. Indeed, RealmContext is even worse than RealmId as a map key or set element, because all its instances are just lambdas, and thus do not have any strong contract for equals() and hashCode(). It is therefore really a bad idea to use those in any collection.

The new scope of this PR is now focused around making the purgeRealms method return the purge results.

@snazy @collado-mike @eric-maynard can an I have another review please?

adutra requested review from ashvina, dennishuo, dimas-b, eric-maynard, jackye1995, jbonofre, vvcephei, collado-mike, snazy, RussellSpitzer, takidau, MonkeyCanCode, flyrain and ebyhr as code owners January 27, 2025 17:40

snazy reviewed Jan 27, 2025

View reviewed changes

eric-maynard reviewed Jan 27, 2025

View reviewed changes

collado-mike reviewed Jan 28, 2025

View reviewed changes

adutra force-pushed the metastore-improvements branch from 6850f7f to 540455d Compare February 1, 2025 12:16

adutra force-pushed the metastore-improvements branch from d45c626 to bfceff3 Compare February 4, 2025 11:17

adutra changed the title ~~MetaStoreManagerFactory API improvements~~ MetaStoreManagerFactory: make purgeRealms() return purge results Feb 4, 2025

adutra force-pushed the metastore-improvements branch from bfceff3 to e369c93 Compare February 4, 2025 11:23

MetaStoreManagerFactory: make purgeRealms() return purge results

85eb460

adutra force-pushed the metastore-improvements branch from a7c0ca0 to 85eb460 Compare February 5, 2025 08:18

adutra mentioned this pull request Feb 5, 2025

Ability to read root credentials from file when bootstrapping #940

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MetaStoreManagerFactory: make purgeRealms() return purge results #889

MetaStoreManagerFactory: make purgeRealms() return purge results #889

adutra commented Jan 27, 2025 •

edited

Loading

snazy Jan 27, 2025

eric-maynard Jan 27, 2025

adutra Jan 27, 2025

collado-mike Jan 28, 2025

snazy Jan 28, 2025

adutra Jan 28, 2025

eric-maynard Jan 27, 2025

adutra Jan 27, 2025

eric-maynard Feb 4, 2025

adutra Feb 5, 2025

eric-maynard Jan 27, 2025

adutra Jan 27, 2025

eric-maynard Jan 27, 2025

adutra Jan 27, 2025

collado-mike left a comment

collado-mike Jan 28, 2025

collado-mike Jan 28, 2025

collado-mike Jan 28, 2025

adutra commented Jan 28, 2025

snazy commented Jan 29, 2025

collado-mike commented Jan 31, 2025

adutra commented Feb 1, 2025

adutra commented Feb 4, 2025

MetaStoreManagerFactory: make purgeRealms() return purge results #889

Are you sure you want to change the base?

MetaStoreManagerFactory: make purgeRealms() return purge results #889

Conversation

adutra commented Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

collado-mike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adutra commented Jan 28, 2025

snazy commented Jan 29, 2025

collado-mike commented Jan 31, 2025

adutra commented Feb 1, 2025

adutra commented Feb 4, 2025

adutra commented Jan 27, 2025 •

edited

Loading