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 > > > > > >
