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

Reply via email to