-
Notifications
You must be signed in to change notification settings - Fork 236
Add DefaultConfigurationStoreDecorator to support inject callContext for DefaultConfigurationStore #1505
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
@Decorator | ||
@Priority(1) | ||
public class DefaultConfigurationStoreDecorator implements PolarisConfigurationStore { |
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 think this needs to be @RequestScope
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.
@collado-mike I am not able to mark this class as Request scoped, if I mark the class as request scoped, I encounter error like the following
[error]: Build step io.quarkus.arc.deployment.ArcProcessor#registerBeans threw an exception: jakarta.enterprise.inject.spi.DefinitionException: A decorator must be @Dependent but org.apache.polaris.service.config.DefaultConfigurationStoreDecorator declares jakarta.enterprise.context.RequestScoped
at io.quarkus.arc.processor.Decorators.createDecorator(Decorators.java:69)
at io.quarkus.arc.processor.BeanDeployment.findDecorators(BeanDeployment.java:1640)
According to the doc
In CDI, decorators must have the same scope or a compatible scope as the beans they decorate.
so if the DefaultConfigurationStore is Application scoped, the DefaultConfigurationStoreDecorator is also Application scoped.
If that is the case does that mean i can not have the callContext to be injected as part of the constructor?
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.
Based on the offline discussion, now we are injecting the request scoped object into the application scoped objects, and things seems working fine now
314994a
to
7f07054
Compare
...ce/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusFeaturesConfiguration.java
Outdated
Show resolved
Hide resolved
(T) | ||
realmOverrides |
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.
Can we use Class.cast
like in PolarisConfiguration
?
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.
are you referring to this line here https://github.com/apache/polaris/blob/main/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java#L125.
For PolarisConfiguration, the type is declared on definition, for example
PolarisConfiguration<Boolean>
so it knows which type to cast to. However, for this function in ConfigurationStore, it doesn't known which type to cast to, so we will not be able to do the same thing. Or are you referring to something else?
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.
That's true, but why are we taking a config name here? Can we offer an overloaded version that takes a PolarisConfiguration
, and there we have safe casting available?
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.
If we want to provide another interface for access the configuration and deprecate the old usage, i think we should do that in a separate PR. The current PR is only intended to do refactor to remove the getCurrentCallContext. I would like to keep the responsibility of this PR small and concise.
9bbfd48
to
844beb5
Compare
e689270
to
cf50fd6
Compare
The goal of this PR is to get rid of the getCurrentContext call in DefaultConfigurationStore: