flyrain commented on code in PR #2887:
URL: https://github.com/apache/polaris/pull/2887#discussion_r2484947428
##########
runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java:
##########
@@ -405,4 +407,11 @@ public PolarisCredentialManager polarisCredentialManager(
public void closeTaskExecutor(@Disposes @Identifier("task-executor")
ManagedExecutor executor) {
executor.close();
}
+
+ @Produces
+ @RequestScoped
+ public MetricsReporter metricsReporter(
+ MetricsReportingConfiguration config, @Any Instance<MetricsReporter>
reporters) {
+ return reporters.select(Identifier.Literal.of(config.type())).get();
Review Comment:
This producer is annotated `@ApplicationScoped` but it returns a
`PolarisMetricsReporter`, which is `@RequestScoped`. That’s problematic in CDI
because an `@ApplicationScoped` bean lives for the entire application
lifecycle, while a `@RequestScoped` bean only lives for a single request.
If the reporter (or a future implementation) ever injects request-scoped
beans like `RealmContext` or `SecurityContext`, this setup would either fail at
deployment or produce invalid references once the request ends.
Instead of producing a reporter directly in an app-scoped producer,
introduce a small factory:
```java
@ApplicationScoped
public class PolarisMetricsReporterFactory {
public PolarisMetricsReporter getOrCreate(String type) {
}
}
```
Similar to `MetaStoreManagerFactory`, which ensure the scope correct, while
avoid creating a new object for every request. It is also more flexible when we
introduce more reporter types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
