Hi Folks, Thanks for chiming in! I agree that renaming while keeping the interface intact is the better option. I see realm more as an atomic concept, and renaming helps maintain consistency across the system. Please take a look at this renaming PR: https://github.com/apache/polaris/pull/899.
Since the topic of extending the Realm interface came up, here are my thoughts: There are two potential approaches for extension: 1. Adding new fields directly to the Realm interface. 1. Works well for private or generated fields. 2. Public fields can also be added carefully, though I share Robert’s concern that some custom beans may not include the new fields. In that case, we should make sure that existing functionality remains intact, even if new fields are empty, so that the backward compatibility is still kept. 2. Creating a new entity that wraps around Realm. This avoids modifying Realm directly but requires updating all interfaces that interact with the new entity, making it also a significant change. Yufei On Mon, Jan 27, 2025 at 9:37 AM Robert Stupp <sn...@snazy.de> wrote: > I'd prefer to keep RealmId for just the ID then, and add a separate > Realm type when there's a use case for it in Polaris. > > Custom realm-specific additions can always produced when needed - e.g. > using `@Produces @RequestScoped MyRealmData produceMyRealmData(RealmId > id, OtherBeanOne one, AnotherBean two)` - and then injected where it's > needed. That allows an extension to reuse existing realm-resolution > mechanisms. > > As mentioned in my previous post, I'm concerned about custom code that > extends a Polaris interface. That approach requires to hook into all > existing code that produces a (not yet existing) `Realm` type - it would > have to replace it everywhere - even in components that an extension > does not know about. In other words: that approach makes an extension > mechanism of Polaris really difficult. > > On 27.01.25 17:58, Dmitri Bourlatchkov wrote: > > Good point about entity injection. I still have reservations, but I guess > > we can address any specific issues when we hit them. > > > > If we go with entity injection, I'd think the refactoring has to go > beyond > > the class/interface renaming and aim at removing `Realm` method > parameters. > > I believe Realm will have to be an instance field on the affected beans. > > Consequently, all beans referencing Realm will have to become > > request-scoped (if they are not such already). > > > > Cheers, > > Dmitri. > > > > On Mon, Jan 27, 2025 at 11:10 AM Alex Dutra > <alex.du...@dremio.com.invalid> > > wrote: > > > >> Hi all, > >> > >> I guess we need to align our vision of what "RealmId" (or whatever we > call > >> it eventually) actually represents. > >> > >> Some of us seem to consider that this component is really just the > unique > >> identifier of a realm entity, pretty much like a primary key. Under that > >> vision, the component is not expected to exhibit any other state and is > and > >> will ever be a wrapper around the realm's string id. > >> > >> Others tend to consider that this component represents the whole realm > >> entity, thus potentially exposing more properties in the future (I > >> mentioned the "state" property as an example of that). > >> > >> I personally would prefer this component to be the full entity, > injected as > >> such. This is to avoid injecting things such as primary keys / > identifiers, > >> forcing consumers to lookup the entity at some point to know more about > it. > >> If we could just inject a proxy to the whole entity directly, that's > imho > >> easier. > >> > >> Thanks, > >> > >> Alex > >> > >> On Mon, Jan 27, 2025 at 4:05 PM Dmitri Bourlatchkov <di...@apache.org> > >> wrote: > >> > >>> Thanks for starting this discussion, Yufei! I think it is important to > >>> clarify this sooner rather than later. > >>> > >>> From my POV option 2 (String) is cumbersome. The biggest reasons being > >> more > >>> complex CDI injection (as already noted by other people) but also more > >>> complicated usage searches in IDEs. > >>> > >>> As for option 1, I support the renaming to "realm" in CLI options and > >> other > >>> user-settable parameters (such as config options). My thinking is that > >> from > >>> the user's perspective, the intent is clear that the user wants to > >> select a > >>> particular realm by its ID (or name). > >>> > >>> As for java class changes, RealmId makes more sense to me because it > >>> indicates that the object holds the ID and not the whole realm (as I > >>> commented on the PR). > >>> > >>> If, for example, we're going to manage Realms as first-class entities > in > >>> Polaris, there will be a need to represent administrative realm data in > >>> memory. Then, there will have to make a distinction between Real ID > >>> extracted from request parameters and Realm object holding persisted > >> data. > >>> That said, I do not feel too strongly about renaming RealmId to Realm > if > >>> other people agree on that. > >>> > >>> Cheers, > >>> Dmitri. > >>> > >>> On Fri, Jan 24, 2025 at 8:51 PM Yufei Gu <flyrain...@gmail.com> wrote: > >>> > >>>> Hi Folks > >>>> > >>>> I wanted to share my thoughts on the ongoing discussion in PR #741 > >>>> <https://github.com/apache/polaris/pull/741>. about whether to use > >> Realm > >>>> or > >>>> RealmId. > >>>> > >>>> The more I consider it, the more it seems that the name Realm is the > >> more > >>>> natural choice. It is more an atomic concept, much like the concept of > >> a > >>>> region in AWS. > >>>> > >>>> If we take a closer look at the current usage across Polaris—in > >>>> documentation, error messages, and configurations—realm and realmId > are > >>>> already used interchangeably. In fact, in most cases, we simply use > >> realm > >>>> rather than realmId. > >>>> > >>>> Here are a few examples in the Polaris repo: > >>>> > >>>> Error Messages: > >>>> > >>>> > >>>> - realm: <realm> root principal credentials: > >>>> <client-id>:<client-secret> > >>>> > >>>> Configurations: > >>>> > >>>> - Polaris.realm-context.realms > >>>> - jdbc:h2:file:./build/test_data/polaris/{realm}/db > >>>> > >>>> Documentation: > >>>> <https://polaris.apache.org/in-dev/unreleased/metastores/> > >>>> > >>>> <https://polaris.apache.org/in-dev/unreleased/metastores/> > >>>> - <https://polaris.apache.org/in-dev/unreleased/metastores/ > >>>> Metastores > >>>> <https://polaris.apache.org/in-dev/unreleased/metastores/> > >>>> - Configuring Polaris for Production > >>>> < > >>>> > >> > https://polaris.apache.org/in-dev/unreleased/configuring-polaris-for-production/ > >>>> - Admin Tool < > >>> https://polaris.apache.org/in-dev/unreleased/admin-tool/> > >>>> Proposal > >>>> > >>>> Based on this consistency and the conceptual clarity it brings, I > >>> proposed > >>>> two options: > >>>> > >>>> Option 1, using the name realm instead of realmId throughout Polaris > >> and > >>>> still keeping the interface (public interface Realm) for dependency > >>>> injection purposes. The interface can be extensible in case we want to > >>> add > >>>> any subconcept to the realm, which may never happen to be honest. > >>>> > >>>> Option 2, purely using a string for realm instead of an interface, > this > >>> is > >>>> simpler ultimately, but not extensible and needs more effort to > >> refactor > >>>> the current code. > >>>> > >>>> > >>>> Looking forward to hearing your thoughts on this. > >>>> > >>>> > >>>> Yufei > >>>> > -- > Robert Stupp > @snazy > >
