-
Notifications
You must be signed in to change notification settings - Fork 328
Make CallContextCatalogFactory request-scoped #2972
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
note the only non-test usage spot is `IcebergCatalogHandler#initializeCatalog` and `IcebergCatalogHandler` is getting created by `IcebergCatalogAdapter` which is already `@RequestScoped`.
b8e0012 to
4a2835b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes LGTM 👍 Thanks, @XN137 !
All changes are contained within the runtime/service module, so there's no impact to polaris-core users downstream.
Making the factory request-scoped improves alignment among service classes and reduces the dependency on realm-based caching in metaStoreManagerFactory.getOrCreateMetaStoreManager(RealmContext).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Nice improvement!
| import org.slf4j.LoggerFactory; | ||
|
|
||
| @ApplicationScoped | ||
| @RequestScoped |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A general remark: just because a CDI bean is being injected request-scoped beans, doesn't mean the bean itself must switch to request scope.
All is needed is that the request scope must be active when any of the injected request-scoped beans is accessed.
Here for instance, this is the case, so keeping @ApplicationScoped doesn't hurt. I tried, and all tests passed.
Keeping application scope whenever possible reduces the number of proxies created for each request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i thought we generally wanted to avoid injecting request-scoped beans into application-scoped beans...
so whats the guiding principle when something should be application or request scoped then for you?
i thought since the PolarisPrincipal is being derived from SecurityContext and is now a ctor parameter, we should mark this class as request-scoped, to make clear this factory is only usable while handling a request.
and as stated in the PR description afaict the factory is only ever injected into IcebergCatalogAdapter which is marked as request-scoped already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Injecting request-scoped PolarisPrincipal into an @ApplicationScoped bean will work in Quarkus, but the injected object will be a proxy, switching to the active context in runtime.
From my POV making this bean @RequestScoped is nicer because it makes bean grouping more explicit and this bean does not have to keep any global state.
note the only non-test usage spot is
IcebergCatalogHandler#initializeCatalogand
IcebergCatalogHandleris getting created byIcebergCatalogAdapterwhich is already
@RequestScoped.