Make ResolverFactory + ResolutionManifestFactory request-scoped #2540

XN137 · 2025-09-10T17:14:57Z

this avoids passing around the CallContext parameter

note that ideally the SecurityContext would also be injected from the request however our tests around PolarisAuthzTestBase are written in a way that does not easily support this currently.

dimas-b

LGTM 👍

dimas-b

I agree that leveraging CDI for request-scope object injection is preferable to manually passing context object as method arguments.

Thanks for you work on this @XN137 !

HonahX

Thanks for looking into these refactoring! I have some concerns that would appreciate your thoughts and feedback.

IMHO, a "factory" makes more sense to be an ApplicationScoped bean, that once initialized, other methods/threads could use it to generate the real instance.

Making factories request-scoped seem to indicate that we do not need actually need these factories, and we can directly have request-scoped Resolver and ResolutionManfiest bean. Otherwise we are generating an additional dispensible factory object for every request.

I think in general the factory design could allow us to add potential customization in the future and maybe some of them could be cached at realm-level instead of every request.

I completely understand the motivation to leverage CDI injection as much as possible. I’m just wondering if there are additional benefits to this approach that would make the refactoring worthwhile?

dimas-b · 2025-09-12T23:42:27Z

Good points, @HonahX !

From my POV, ResolverFactory could probably be replaced with a CDI producer for injecting request-scoped Resolver instances.

I tend to view this PR as an incremental step towards that goal :)

That said, I guess it might be preferable to continue this kind of design discussion on the dev ML. I believe it is more readable and easier to discover than GH issues discussions... especially if the number of replies is large :)

HonahX · 2025-09-13T22:08:24Z

Thanks @dimas-b for the explanation! +1 for a dev list discussion for this.

From my POV, ResolverFactory could probably be replaced with a CDI producer for injecting request-scoped Resolver instances.

I think this will be a good point to discuss. CDI producer could help simplify the code within the polaris, however I barely remembered that there was some discussion on making polaris-core a "library" that could be useful in other custom implementation/framework. Replacing the factory class with CDI producer might make it harder towards that goal. But this is just my perspective and would love to hear others' thoughts once it is on the dev list

XN137 · 2025-09-24T07:49:53Z

rebased after trivial conflict in PolarisAdminService.java

XN137 · 2025-09-24T13:35:41Z

sorry i was OOO for a week. thank you for the feedback everyone!

IMHO, a "factory" makes more sense to be an ApplicationScoped bean, that once initialized, other methods/threads could use it to generate the real instance.

i dont think there is a strict requirement to have factories be application scoped.
a factory is just serving as an injectable decision point about how specific instances of an interface are supposed to be created (for a particular subset of the system).
that doesnt immediately imply requirements on the lifetime scope of the factory or the produced objects.

for example on main branch the PolarisResolutionManifest uses the ResolverFactory to produce a primaryResolver field immediately in the ctor and a "single-use" passthroughResolver on the fly in getPassthroughResolvedPath.

generally ResolverFactory is only used by PolarisResolutionManifest and IcebergCatalogAdapter both of which afaict have a lifetime bound to a single request.
this is indicated by both of them having a CallContext field, which gets initialized at construction time.

Making factories request-scoped seem to indicate that we do not need actually need these factories, and we can directly have request-scoped Resolver and ResolutionManfiest bean. Otherwise we are generating an additional dispensible factory object for every request.

i would disagree with this because as shown in this PR both ResolverFactory and ResolutionManifestFactory cant be used without a CallContext which is only available in a request-scope.
also, as pointed out above, the factory is also used on the fly to generate multiple instances inside a single object / the "same request".

so while the factory currently is application-scoped it cant be used outside of a request-scope and this means all callers must pass all the required parameters around all the time, which this PR tries to avoid/clean up.

I think in general the factory design could allow us to add potential customization in the future and maybe some of them could be cached at realm-level instead of every request.

correct me if i am wrong but i think even with making the factories request-scope (which matches their usage in the codebase) the implementation of that factory is still free to implement the object creation however it wants.
so if something is supposed to be cached at realm-level the factory is still free to do that.
(for example by injecting an application scoped bean that handles shared realm state).
but this custom implementation decision does not need to be part of the factory usage in the codebase.
a request-scoped factory can also return a singleton object if it really wants.

XN137 · 2025-09-24T13:39:17Z

From my POV, ResolverFactory could probably be replaced with a CDI producer for injecting request-scoped Resolver instances.

not sure this is possible as explained in how PolarisResolutionManifest uses the ResolverFactory i.e. one resolver instance being created in the ctor and one on the fly in a method call.
so we cant inject Resolver instances directly (apart from maybe the primaryResolver one, assuming its state not being shared by multiple unrelated usage spots).

XN137 · 2025-09-26T08:09:58Z

rebased again after trivial conflict in PolarisAdminService.java

dimas-b

This refactoring LGTM and I think it is aligned with our code "evolution" approach, so I think it can be merged without further dev ML discussions.

@HonahX : WDYT?

This, of course, does not preclude dev list discussions about API changes and how to deal with CDI beans and factories in general. I merely mean that this particular change looks reasonable to merge from my POV 🙂

XN137 · 2025-09-29T07:06:19Z

rebased again after trivial conflict in PolarisAdminService.java

this avoids passing around the `CallContext` parameter note that ideally the `SecurityContext` would also be injected from the request however our tests around `PolarisAuthzTestBase` are written in a way that does not easily support this currently.

dimas-b · 2025-10-01T14:32:11Z

@HonahX : Do you think we could merge this?

adutra · 2025-10-01T15:22:28Z

runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java

+  @RequestScoped
+  public ResolutionManifestFactory resolutionManifestFactory(
+      PolarisDiagnostics diagnostics, RealmContext realmContext, ResolverFactory resolverFactory) {
+    return new ResolutionManifestFactoryImpl(diagnostics, realmContext, resolverFactory);


Just a small nit: why do you need this bean to be produced by a producer method? Wouldn't it be enough to annotate ResolutionManifestFactoryImpl with @RequestScoped?

it's probably not strictly needed, this just keeps the same style the previous code used.
we can also do it the other way if you prefer.
i'd suggest to do it a followup if its ok with you?

dimas-b · 2025-10-02T13:16:48Z

Given some approvals I assume lazy consensus from people who comment before but did not formally approve... merging.

Open to follow-up (if required).

github-project-automation bot added this to Basic Kanban Board Sep 10, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Sep 10, 2025

dimas-b reviewed Sep 10, 2025

View reviewed changes

XN137 marked this pull request as ready for review September 11, 2025 09:57

XN137 force-pushed the request-scoped-ResolverFactory branch from 9b3c687 to 1abd4a4 Compare September 12, 2025 06:22

dimas-b requested review from HonahX, dennishuo and eric-maynard September 12, 2025 15:38

dimas-b previously approved these changes Sep 12, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Sep 12, 2025

HonahX reviewed Sep 12, 2025

View reviewed changes

XN137 dismissed dimas-b’s stale review via f86f02d September 24, 2025 07:49

XN137 force-pushed the request-scoped-ResolverFactory branch from 1abd4a4 to f86f02d Compare September 24, 2025 07:49

XN137 force-pushed the request-scoped-ResolverFactory branch from f86f02d to 5d2c56d Compare September 26, 2025 08:09

XN137 force-pushed the request-scoped-ResolverFactory branch from 5d2c56d to 703426c Compare September 26, 2025 09:27

dimas-b previously approved these changes Sep 26, 2025

View reviewed changes

XN137 dismissed dimas-b’s stale review via d143b73 September 29, 2025 07:06

XN137 force-pushed the request-scoped-ResolverFactory branch from 703426c to d143b73 Compare September 29, 2025 07:06

dimas-b approved these changes Sep 30, 2025

View reviewed changes

dimas-b requested a review from HonahX September 30, 2025 01:18

XN137 force-pushed the request-scoped-ResolverFactory branch from d143b73 to 45261df Compare October 1, 2025 06:01

snazy approved these changes Oct 1, 2025

View reviewed changes

adutra reviewed Oct 1, 2025

View reviewed changes

dimas-b merged commit 359388e into apache:main Oct 2, 2025
16 checks passed

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Oct 2, 2025

XN137 deleted the request-scoped-ResolverFactory branch October 2, 2025 13:17

Make ResolverFactory + ResolutionManifestFactory request-scoped #2540

Make ResolverFactory + ResolutionManifestFactory request-scoped #2540

Uh oh!

Conversation

XN137 commented Sep 10, 2025

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

HonahX left a comment

Choose a reason for hiding this comment

Uh oh!

dimas-b commented Sep 12, 2025

Uh oh!

HonahX commented Sep 13, 2025

Uh oh!

XN137 commented Sep 24, 2025

Uh oh!

XN137 commented Sep 24, 2025

Uh oh!

XN137 commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

XN137 commented Sep 26, 2025

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

XN137 commented Sep 29, 2025

Uh oh!

dimas-b commented Oct 1, 2025

Uh oh!

adutra Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

XN137 Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

dimas-b commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

XN137 commented Sep 24, 2025 •

edited

Loading

dimas-b commented Oct 2, 2025 •

edited

Loading