dimas-b commented on code in PR #3385:
URL: https://github.com/apache/polaris/pull/3385#discussion_r2771450614


##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -247,6 +248,21 @@ public synchronized EntityCache getOrCreateEntityCache(
     return entityCacheMap.get(realmContext.getRealmIdentifier());
   }
 
+  @Override
+  public synchronized MetricsPersistence 
getOrCreateMetricsPersistence(RealmContext realmContext) {

Review Comment:
   As I mentioned by 
[email](https://lists.apache.org/thread/jod4yczlp6n28gcto51rv3f8pp574xpb), I 
tend to think it would be nice to handle Metrics Persistence independently of 
Entity Persistence (the MetaStore).
   
   That is to say, would it be possible to make `MetricsPersistence` directly 
from CDI producers?
   
   Also, I'm personally very hesitant to use the "get or create" pattern as it 
kind of forces the implementation to maintain state. With pure injection, the 
impl. would be free to use stateful objects or create on demand... whatever 
works best.
   
   In this case, I'd think `MetricsPersistence` is probably going to be a 
request-scoped bean credeted on demand... WDYT?
   
   `RealmContext` would be resolved / provided by CDI too.



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

Reply via email to