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