dimas-b commented on code in PR #4397:
URL: https://github.com/apache/polaris/pull/4397#discussion_r3344915300
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -236,20 +250,14 @@ public PolarisMetaStoreManager
getOrCreateMetaStoreManager(RealmContext realmCon
@Override
public BasePersistence getOrCreateSession(RealmContext realmContext) {
- String realmId = realmContext.getRealmIdentifier();
- RealmConfig realmConfig = new RealmConfigImpl(realmConfigurationSource,
realmContext);
- boolean fallbackOnDne =
-
realmConfig.getConfig(BehaviorChangeConfiguration.SCHEMA_VERSION_FALL_BACK_ON_DNE);
-
- // Verify bootstrap once per realm lifetime; skip on subsequent calls.
- // On cold start, multiple threads may verify concurrently — this is benign
- // (idempotent DB query), trading a few redundant queries for simpler code.
- if (!verifiedRealms.contains(realmId)) {
- checkPolarisServiceBootstrappedForRealm(realmContext, fallbackOnDne);
- }
+ return createJdbcPersistence(realmContext);
+ }
- // Stateless — create a fresh instance on every call; schemaVersion is
cached per realm
- return createSession(realmId, null, fallbackOnDne);
+ @Override
+ public MetricsPersistence getOrCreateMetricsPersistence(RealmContext
realmContext) {
+ // JdbcBasePersistenceImpl implements both BasePersistence and
MetricsPersistence, so reuse the
+ // session creation path rather than building a second, independent
persistence instance.
+ return (MetricsPersistence) getOrCreateSession(realmContext);
Review Comment:
Why not call `createJdbcPersistence()` and avoid the type cast?
##########
runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java:
##########
@@ -136,8 +137,14 @@ public CallContext polarisCallContext(
RealmContext realmContext,
RealmConfigurationSource configurationSource,
MetaStoreManagerFactory metaStoreManagerFactory) {
- BasePersistence metaStoreSession =
metaStoreManagerFactory.getOrCreateSession(realmContext);
- return new PolarisCallContext(realmContext, metaStoreSession,
configurationSource);
+ BasePersistence metaStore =
metaStoreManagerFactory.getOrCreateSession(realmContext);
+ // When the backend implements both SPIs on the same instance (e.g. JDBC,
in-memory), reuse the
+ // session instead of building a second persistence instance per request.
+ MetricsPersistence metricsPersistence =
+ (metaStore instanceof MetricsPersistence mp)
+ ? mp
+ :
metaStoreManagerFactory.getOrCreateMetricsPersistence(realmContext);
Review Comment:
This call is generally useless now. All functional implementations of
`getOrCreateMetricsPersistence()` produce the objects that support both the
`BasePersistence` and `MetricsPersistence` interfaces.
If we want to keep the type check logic on like 144, I think we ought to
remove `getOrCreateMetricsPersistence()` from `MetaStoreManagerFactory` for the
sake of clarity.
Alternatively, we can remove the type check on line 144 and rely on the
factory to perform optimizations like returning the same object for both
cases... when required.
As for JDBC, I do not think such an optimization is required. The returned
`JdbcBasePersistenceImpl` is a light-weight shell and creating two vs. one of
then will hardly be noticeable in runtime.
--
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]
