Hi Alex,
I was saying that people might *NOT immediately* connect a realmId with a
realm. They will figure it out in various ways for sure, like checking
code/doc, but that's an inconsistency and unnecessary cognitive burden we
can easily avoid.
Yufei
On Thu, Jan 30, 2025 at 7:20 AM Alex Dutra <alex.du...@dremio.com.invalid>
wrote:
> Hi,
>
> I must say, I am more and more confused by this discussion. This came as a
> surprise to me:
>
> people may keep thinking "realm" and "realmId" are different things
>
>
> Well, for me they are different things. As I said before, the notion behind
> the name, regardless of what we call it, seems to change from person to
> person.
>
> But assuming that we rename the current "RealmId" to "Realm", and keep that
> object as a wrapper around an identifier; what are we going to call the
> fully-fledged object that represents a realm with its identifier,
> properties, and internal state? "RealmEntity"? Because we're going to need
> that object sooner than later.
>
> Thanks,
> Alex
>
> On Wed, Jan 29, 2025 at 10:54 PM Yufei Gu <flyrain...@gmail.com> wrote:
>
> > Having a consistent name across the system is a huge benefit for users
> and
> > developers. There would be less confusion and less cognitive burden for
> > everyone. Otherwise, people may keep thinking "realm" and "realmId" are
> > different things. As I said in the proposal, the name "realm" is already
> > widely used. I don't think we are going to change them to `realmId`.
> >
> > 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/>
> >
> >
> > Yufei
> >
> >
> > On Wed, Jan 29, 2025 at 1:17 PM Robert Stupp <sn...@snazy.de> wrote:
> >
> > > Thinking more about this, having a huge change just for a nomenclature
> > > thing is really not worth the effort, so I'm against such a rename.
> > >
> > > On 29.01.25 11:02, Robert Stupp wrote:
> > > > Let's take a step back.
> > > >
> > > > The documented contract of the old `RealmContext` was to provide the
> > > > realm-ID from a REST request - nothing more, nothing less - that's
> all
> > > > what's been implemented. And this is exactly the reason why the
> > > > current type is called `RealmId`.
> > > >
> > > > Renaming `RealmId` to `Realm` and then also add another `RealmXyz`
> > > > type makes it difficult to distinguish, causing unnecessary more
> > > > confusion.
> > > >
> > > > Please keep in mind that the code that extracts the realm-ID from a
> > > > REST request needs to be self-contained. Adding more functionality to
> > > > that unnecessarily complicates the code to extract the realm-ID from
> a
> > > > REST request.
> > > >
> > > > If more information is needed, there can always be a CDI-producer
> that
> > > > provides that in another request-scoped bean based on `RealmId`.
> > > >
> > > > On 29.01.25 00:59, Yufei Gu wrote:
> > > >> 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
> > > >>>
> > > >>>
> > > --
> > > Robert Stupp
> > > @snazy
> > >
> > >
> >
>
