Thanks, Alex! Very good points!
Re: metrics, I did a quick POC by adding `@Inject RealmContext rc`
to QuarkusValueExpressionResolver and it appears to work. That
is, QuarkusValueExpressionResolver can get the realm ID from the injected
RealmContext as opposed to the API parameter.
With that, we could probably change the interpretation of the
"realmIdentifier" expression to be the injected RealmContext, remove
explicit realm parameters from the generated API classes and move the
`@MeterTag(key="realm_id",expression="realmIdentifier")` annotation to the
method level.
WDYT?
Thanks,
Dmitri.
On Fri, Jun 6, 2025 at 12:41 PM Alex Dutra <alex.du...@dremio.com.invalid>
wrote:
> Hi Dmitri,
>
> Thanks for bringing up this important topic.
>
> I generally agree with your refactoring proposal, but with 2 caveats:
>
> 1) For an application-scoped bean exposing methods to retrieve realm-scoped
> components (e.g., the "factory" beans that expose methods like
> "getOrCreateXYZ") I think it's actually OK to receive the RealmContext as a
> method parameter, as opposed to having the RealmContext injected. This is
> because the injection, while possible, would only work if the request
> context is active at the moment where a method on that bean is invoked.
> This is hard to enforce at compile time and also hard to reason about imho.
> IOW I prefer to see this:
>
> @ApplicationScoped public class XYZFactory {
> public XYZ getOrCreateXYZ(RealmContext ctx) { return
> doSomeStuff(ctx.getRealmIdentifier()); }
> }
>
> Rather than this:
>
> @ApplicationScoped public class XYZFactory {
> @Inject RealmContext ctx;
> public XYZ getOrCreateXYZ() { return
> doSomeStuff(ctx.getRealmIdentifier()); }
> }
>
> 2) We do need a RealmContext parameter in the API classes generated from
> Mustache templates [1], because the RealmContext parameter is annotated as
> follows:
>
> @Context @MeterTag(key="realm_id",expression="realmIdentifier")
> RealmContext realmContext
>
> This is so that we can produce "realm_id" tags for API metrics. Thus we
> cannot remove it (unless there is another way to produce such tags, but I'm
> not aware of any).
>
> But we could remove it from the Mustache templates for *service* classes
> [2]. This would effectively remove RealmContext as a method parameter from
> virtually all components down the call stack, thus forcing them to inject
> RealmContext wherever necessary.
>
> Thanks,
>
> Alex
>
> [1]:
>
> https://github.com/apache/polaris/blob/1baef0edec7dfd5d6440dc1d177a713c3b29055c/server-templates/api.mustache#L96
> [2]:
>
> https://github.com/apache/polaris/blob/1baef0edec7dfd5d6440dc1d177a713c3b29055c/server-templates/apiService.mustache#L39
>
> On Fri, Jun 6, 2025 at 3:49 AM Dmitri Bourlatchkov <di...@apache.org>
> wrote:
>
> > An example "skew" in current code: IcebergCatalogAdapter gets
> RealmContext
> > injected as a field, but also as a method parameters in
> createNamespace().
> >
> > On Thu, Jun 5, 2025 at 8:35 PM Dmitri Bourlatchkov <di...@apache.org>
> > wrote:
> >
> > > Hi All,
> > >
> > > Recently a few runtime issues related to the resolution of RealmContex
> > got
> > > fixed (mostly by Yun's PRs).
> > >
> > > These fixes center around the idea of passing RealmContext as a
> parameter
> > > across various method calls.
> > >
> > > That approach is effective, however, I tend to think it goes against
> the
> > > grain of the dependency injection paradigm. The main point is that
> realm
> > is
> > > a contextual concept in Polaris REST APIs (it is not a parameter in any
> > API
> > > operations). Subsequently, all the service and core code logic is
> > normally
> > > realm-neutral. That is to say that services operate without having to
> > know
> > > what realm they are servicing.
> > >
> > > Realm IDs become prominent only at the persistence level (to ensure
> > > isolation of realm data).
> > >
> > > I'd like to propose refactoring Polaris code to remove RealmContext
> from
> > > method parameters and keep it in instance fields only where it is used.
> > >
> > > * MetaStore objects for REST API endpoints will be produced for a
> > > particular real with the request scope lifetime
> > > * Tasks will operate in a similar way, where PR [1817] provides a POC
> > > * The Quarkus runtime will use CDI for injection
> > > * The polaris-core module will remain free from CDI annotations, but
> some
> > > changes will probably be required to allow injection via constructors.
> > >
> > > WDYT?
> > >
> > > If there are no strong objections up front, I can work on a bigger POC
> PR
> > > to allow reviewing / exploring this idea in more depth.
> > >
> > > [1817] https://github.com/apache/polaris/pull/1817
> > >
> > > Cheers,
> > > Dmitri.
> > >
> >
>
