Refactor `RealmContext` to `RealmId` #741

snazy · 2025-01-14T12:25:18Z

RealmContext is not an actual context but just a container for the ID of the realm. This change makes this explicit.

helm/polaris/README.md

adutra · 2025-01-14T12:44:58Z

...src/main/java/org/apache/polaris/service/quarkus/metrics/QuarkusValueExpressionResolver.java

This class exists only because the old RealmContext did not have a meaningful toString() representation.

When this class is passed as a method argument to a generated API method:

@Context @MeterTag(key="realm_id",expression="realmIdentifier") RealmContext realmContext

I had to use an expression + expression resolver in order to extract the id from RealmContext.

Question: is it possible for RealmId to have a custom toString() representation that simply returns the realm ID? In this case we can get rid of the expression + resolver.

Let me check...

meh - does not work :(

Okay, let's at least change the expression from "realmIdentifier" to "id" – I mean, for now it doesn't matter (it could be any string), but if we ever introduce CEL, then the expression must be valid.

shouldn't it be "realm.id" ?

Hmm yes, that's probably better 👍

polaris-core/src/main/java/org/apache/polaris/core/context/RealmId.java

dimas-b

The new RealmId name does look more appropriate given its usage.

`RealmContext` is not an actual context but just a container for the ID of the realm. This change makes this explicit.

flyrain · 2025-01-14T22:59:00Z

polaris-core/src/main/java/org/apache/polaris/core/context/RealmId.java

+@PolarisImmutable
+@JsonSerialize(as = ImmutableRealmId.class)
+@JsonDeserialize(as = ImmutableRealmId.class)
+public interface RealmId {


If we were making this change, why don't we call it Realm, instead of RealmId?

Because it's the ID of a realm - not the realm itself.

The realmId used to be a simple string, but it has now been replaced by a RealmId type, which contains a field called id—essentially an id of an id. While this approach works, it feels cumbersome and lacks extensibility. For instance, if we want to add new fields like a description for a realm, we would have to either include it under this interface or create a separate interface, such as Realm or RealmContext.

Adding new fields directly under RealmId feels odd since these fields typically represent a concept that is part of a realm rather than the realmId. On the other hand, introducing a separate interface adds complexity by requiring an additional structure to be defined.

Additionally, I’m questioning whether the concepts of realm and realmId are truly distinct or interchangeable. If they are interchangeable, using the name realm instead of realmId might be a more concise and descriptive choice, simplifying the naming and making the code easier to understand.

flyrain

This is a breaking change and is not compatible with the existing deployment. I'm fine with moving forward if others are comfortable with it, but I’d like to hear more input from the team first. cc @dennishuo @collado-mike @eric-maynard

collado-mike

This seems innocuous enough. My main concern would be if application users suddenly didn't see the keys they were looking for in the logs or the headers suddenly stopped working, bug AFAICT, there are no such changes

flyrain · 2025-01-16T01:31:00Z

Please don’t merge a PR if there are open comments. It’s important to work through all feedback, especially for something as impactful as a breaking change. Plus this isn’t an urgent fix, and just filed 2 days ago, we should take the time to think it through carefully before merging.
cc @RussellSpitzer @jbonofre @eric-maynard @collado-mike @dennishuo

collado-mike · 2025-01-16T01:54:00Z

Please don’t merge a PR if there are open comments. It’s important to work through all feedback, especially for something as impactful as a breaking change. Plus this isn’t an urgent fix, and just filed 2 days ago, we should take the time to think it through carefully before merging.

+1 There are a lot of changes to interfaces and user-facing APIs being pushed and some of them are not really more than cosmetic. I'm all for refactoring and nailing down the right APIs, but these changes should have appropriate review and feedback. If there are open comments, people shouldn't be merging without closing the conversation first.

jbonofre · 2025-01-16T05:50:48Z

Please don’t merge a PR if there are open comments. It’s important to work through all feedback, especially for something as impactful as a breaking change. Plus this isn’t an urgent fix, and just filed 2 days ago, we should take the time to think it through carefully before merging.

cc @RussellSpitzer @jbonofre @eric-maynard @collado-mike @dennishuo

Absolutely ! It's required in an Apache project to have collaboration. If it helps we can enabled blocking PR merge if all discussions are not resolved.

snazy · 2025-01-16T08:10:15Z

Please don’t merge a PR if there are open comments. It’s important to work through all feedback, especially for something as impactful as a breaking change. Plus this isn’t an urgent fix, and just filed 2 days ago, we should take the time to think it through carefully before merging.

@flyrain @jbonofre The PR got two approvals. The comment with the approval from @collado-mike answering your question: "This seems innocuous enough. My main concern would be if application users suddenly didn't see the keys they were looking for in the logs or the headers suddenly stopped working, bug AFAICT, there are no such changes" (emphasis mine)

BTW: This RealmId is still an interface - so I do not understand why this could not be extended. The documentation did and still clearly states that this type is used to identify the realm from a REST request - and from a REST request all you have is the ID.

jbonofre · 2025-01-16T08:25:03Z

Please don’t merge a PR if there are open comments. It’s important to work through all feedback, especially for something as impactful as a breaking change. Plus this isn’t an urgent fix, and just filed 2 days ago, we should take the time to think it through carefully before merging.

@flyrain @jbonofre The PR got two approvals. The comment with the approval from @collado-mike answering your question: "This seems innocuous enough. My main concern would be if application users suddenly didn't see the keys they were looking for in the logs or the headers suddenly stopped working, bug AFAICT, there are no such changes" (emphasis mine)

BTW: This RealmId is still an interface - so I do not understand why this could not be extended. The documentation did and still clearly states that this type is used to identify the realm from a REST request - and from a REST request all you have is the ID.

My previous comment was a general one: if there are comments before merge it should be considered. For this PR in particular, we had approval from two committers and no non resolved discussions (before the merge) so we are probably fine. If it needs to be revisited later, a committer can open a revert PR or an update PR that's fine.

collado-mike · 2025-01-16T18:29:36Z

The comment with the approval from @collado-mike answering your question

I explicitly did not mark the PR as approved, as I was expecting for other folks to chime in. The change does seem innocuous, but I think asking to wait for feedback and the expectation to wait for @flyrain to resolve the conversation before merging seems reasonable

collado-mike · 2025-01-16T18:44:17Z

I explicitly did not mark the PR as approved, as I was expecting for other folks to chime in

Shoot. I didn't mean to click approve. Sorry about the confusion. I meant for others to be able to chime in

sfc-gh-ygu · 2025-01-17T05:24:03Z

no non resolved discussions (before the merge)

@jbonofre This comment wasn't resolved before the merge, #741 (comment).

@snazy, we don’t really need an interface for an ID. An ID is just a string, which I see as an atomic concept—simple and self-contained, without properties. Adding sub-properties to an ID interface (e.g., RealmId) feels conceptually incorrect and somewhat counterintuitive. Given this, I believe names like RealmContext or Realm are much more fitting for the interface, as they better reflect the broader context or functionality it represents. This assumes we need an interface here. For any use case just requiring an id, we could use a string type instead of an interface.

snazy · 2025-01-17T06:46:27Z

A distinct type is needed to eventually inject it via CDI. Further, a distinct type makes it very clear that this string is the realm-id. I also noted above that the docs for this interface clearly state that this type represents the realm-ID extracted from a REST request - and there is only the ID that can be extracted from a REST request. That can then be used to get more information.

flyrain · 2025-01-17T07:50:42Z

If the ID is the only element we need to extract, realm is a better choice than RealmId due to its simplicity and conciseness. For instance, we wouldn't use nameId for name, cityId for city, or ageId for age when they all inherently refer to the same concept.

snazy · 2025-01-17T07:58:23Z

If the ID is the only element we need to extract, realm is a better choice than RealmId due to its simplicity and conciseness. For instance, we wouldn't use nameId for name, cityId for city, or ageId for age when they all inherently refer to the same concept.

That's your personal preference, right?

flyrain · 2025-01-17T17:41:30Z

If my name is Bob, I will be uncomfortable if others call me BobID.

eric-maynard · 2025-01-17T17:44:39Z

Is the RealmId object itself the identifier, or does RealmId.id return the identifier? It can't be both.

dimas-b · 2025-01-17T18:48:50Z

I'll add my 2 cents, since I commented above saying that the renaming made sense :)

It's true that RealmId by itself is not used as the realm's identifier in code. However, the real identifier is a String, so to add type safety and simplify usage searches (in IDEs), I still think using a dedicated "holder" class for that ID makes sense.

As to Realm vs. RealmId, I think the former implies that the realm's data or content is held by the object, which is not the case. Only the ID is held inside that object.

Now, I see that RealmId.id may look awkward. I think it would also be ok for follow-up PRs to rename that. I'm personally ok with RealmId.id(), but RealmId.asString() would also work from my POV (but it's more verbose).

* Revert "Copy RealmId when passing it to TaskExecutorImpl (#879)" This reverts commit febe4e8. * Revert #741

This reverts commit 390f1fa.

snazy force-pushed the rename-realm-context branch from 988c7ef to f4352b5 Compare January 14, 2025 12:27

snazy marked this pull request as ready for review January 14, 2025 12:27

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

adutra reviewed Jan 14, 2025

View reviewed changes

snazy force-pushed the rename-realm-context branch 3 times, most recently from b8de206 to 166b8ed Compare January 14, 2025 13:17

adutra reviewed Jan 14, 2025

View reviewed changes

polaris-core/src/main/java/org/apache/polaris/core/context/RealmId.java Outdated Show resolved Hide resolved

snazy force-pushed the rename-realm-context branch 2 times, most recently from cdd336e to e273f1f Compare January 14, 2025 13:51

dimas-b approved these changes Jan 14, 2025

View reviewed changes

snazy added 2 commits January 14, 2025 22:47

Refactor RealmContext to RealmId

a389338

`RealmContext` is not an actual context but just a container for the ID of the realm. This change makes this explicit.

realmIdentifier -> realm.id

be51c21

snazy force-pushed the rename-realm-context branch from e273f1f to be51c21 Compare January 14, 2025 21:49

dimas-b approved these changes Jan 14, 2025

View reviewed changes

flyrain reviewed Jan 14, 2025

View reviewed changes

collado-mike approved these changes Jan 14, 2025

View reviewed changes

snazy merged commit 390f1fa into apache:main Jan 15, 2025
5 checks passed

snazy deleted the rename-realm-context branch January 15, 2025 06:16

This was referenced Jan 29, 2025

Proposal: Use of Realm Instead of RealmId #910

Closed

Rename "RealmId" to "Realm" #899

Closed

Revert "Refactor RealmContext to RealmId (#741)" #920

Merged

flyrain added a commit that referenced this pull request Feb 3, 2025

Revert "Refactor RealmContext to RealmId (#741)" (#920)

f0c99a6

* Revert "Copy RealmId when passing it to TaskExecutorImpl (#879)" This reverts commit febe4e8. * Revert #741

eric-maynard added a commit to eric-maynard/polaris that referenced this pull request Feb 6, 2025

Revert "Refactor RealmContext to RealmId (apache#741)"

a9d7725

This reverts commit 390f1fa.

Refactor RealmContext to RealmId #741

Refactor RealmContext to RealmId #741

Uh oh!

Conversation

snazy commented Jan 14, 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!

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!

Uh oh!

dimas-b left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

flyrain left a comment

Choose a reason for hiding this comment

Uh oh!

collado-mike left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

flyrain commented Jan 16, 2025

Uh oh!

collado-mike commented Jan 16, 2025

Uh oh!

jbonofre commented Jan 16, 2025

Uh oh!

snazy commented Jan 16, 2025

Uh oh!

jbonofre commented Jan 16, 2025

Uh oh!

collado-mike commented Jan 16, 2025

Uh oh!

collado-mike commented Jan 16, 2025

Uh oh!

sfc-gh-ygu commented Jan 17, 2025

Uh oh!

snazy commented Jan 17, 2025

Uh oh!

flyrain commented Jan 17, 2025

Uh oh!

snazy commented Jan 17, 2025

Uh oh!

flyrain commented Jan 17, 2025

Uh oh!

eric-maynard commented Jan 17, 2025

Uh oh!

dimas-b commented Jan 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Refactor `RealmContext` to `RealmId` #741

Refactor `RealmContext` to `RealmId` #741

dimas-b left a comment •

edited

Loading