I think, we should stick with `RealmId` as it clearly means "this is the
ID of a realm". It is also a concrete type and can therefore
unambiguously @Inject'ed - whereas using CDI qualifiers for simple types
like j.l.String does not work well in practice.
I do not mind having another type or service to get more information
about a realm.
Adding additional functionality like `MyPrivateRealm extends Realm` is
practically not going to end well. What if some other component produces
a `Realm` and your private extension expects `MyPrivateRealm`? Wanna do
an `instanceof` - but what if that yields `false`?
I'd be fine about just renaming `RealmId` to `Realm` but sticking with
containing just the ID of the realm. OTOH, not sure it's worth such a
large change (again) for nomenclature.
On 25.01.25 22:15, Jean-Baptiste Onofré wrote:
Hi Yufei
Thanks for starting this discussion.
My preference is for option 1, both for the naming but also because
the interface can host additional "functions" for the realm (in the
future).
Regards
JB
On Sat, Jan 25, 2025 at 2:51 AM 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
