dimas-b commented on code in PR #4054:
URL: https://github.com/apache/polaris/pull/4054#discussion_r3023676157
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -93,42 +107,28 @@ protected PolarisMetaStoreManager
createNewMetaStoreManager() {
return new AtomicOperationMetaStoreManager(clock, diagnostics);
}
- private void initializeForRealm(
- DatasourceOperations datasourceOperations,
- RealmContext realmContext,
- RootCredentialsSet rootCredentialsSet) {
- // Materialize realmId so that background tasks that don't have an active
- // RealmContext (request-scoped bean) can still create a
JdbcBasePersistenceImpl
- String realmId = realmContext.getRealmIdentifier();
- // determine schemaVersion once per realm
- final int schemaVersion =
- JdbcBasePersistenceImpl.loadSchemaVersion(
- datasourceOperations,
-
realmConfig.getConfig(BehaviorChangeConfiguration.SCHEMA_VERSION_FALL_BACK_ON_DNE));
-
- sessionSupplierMap.put(
+ /** Loads and caches the schema version for the given realm (DB hit only on
first call). */
+ private int getOrLoadSchemaVersion(String realmId) {
+ return schemaVersionCache.computeIfAbsent(
realmId,
- () ->
- new JdbcBasePersistenceImpl(
- diagnostics,
+ k ->
+ JdbcBasePersistenceImpl.loadSchemaVersion(
datasourceOperations,
- secretsGenerator(realmId, rootCredentialsSet),
- storageIntegrationProvider,
- realmId,
- schemaVersion));
-
- PolarisMetaStoreManager metaStoreManager = createNewMetaStoreManager();
- metaStoreManagerMap.put(realmId, metaStoreManager);
+ realmConfig.getConfig(
Review Comment:
`realmConfig` is request-scoped. It must be aligned with the `realmId`
parameter (represent the same realm). It believe this assumption holds in
current code, but it is hard to follow and make sure.
Would you mind obtaining the `SCHEMA_VERSION_FALL_BACK_ON_DNE` value at the
same place where `realmId` is obtained and passing both as parameters?
I believe this way the code will be more robust WRT request-scoped data
handling.
Note: in `purgeRealms` we'll have to make a hard-coded decision about
`SCHEMA_VERSION_FALL_BACK_ON_DNE` because there is no true injected realm
context there (cf. #3411). I believe it should be ok for now. It's not a
behaviour change, but will make actual behaviour explicit.
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -67,19 +69,31 @@ public class JdbcMetaStoreManagerFactory implements
MetaStoreManagerFactory {
private static final Logger LOGGER =
LoggerFactory.getLogger(JdbcMetaStoreManagerFactory.class);
- final Map<String, PolarisMetaStoreManager> metaStoreManagerMap = new
HashMap<>();
- final Map<String, EntityCache> entityCacheMap = new HashMap<>();
- final Map<String, Supplier<BasePersistence>> sessionSupplierMap = new
HashMap<>();
+ // Stateful per-realm cache — InMemoryEntityCache accumulates entries across
requests
+ final Map<String, EntityCache> entityCacheMap = new ConcurrentHashMap<>();
Review Comment:
`private`?
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -67,19 +69,31 @@ public class JdbcMetaStoreManagerFactory implements
MetaStoreManagerFactory {
private static final Logger LOGGER =
LoggerFactory.getLogger(JdbcMetaStoreManagerFactory.class);
- final Map<String, PolarisMetaStoreManager> metaStoreManagerMap = new
HashMap<>();
- final Map<String, EntityCache> entityCacheMap = new HashMap<>();
- final Map<String, Supplier<BasePersistence>> sessionSupplierMap = new
HashMap<>();
+ // Stateful per-realm cache — InMemoryEntityCache accumulates entries across
requests
+ final Map<String, EntityCache> entityCacheMap = new ConcurrentHashMap<>();
+
+ // Cached per-realm schema version — loaded from DB once, stable at runtime
+ private final ConcurrentHashMap<String, Integer> schemaVersionCache = new
ConcurrentHashMap<>();
+
+ // Tracks realms that have already passed the bootstrap verification check
+ // (checkPolarisServiceBootstrappedForRealm), avoiding redundant DB hits on
subsequent calls.
+ private final Set<String> verifiedRealms = ConcurrentHashMap.newKeySet();
@Inject Clock clock;
@Inject PolarisDiagnostics diagnostics;
@Inject PolarisStorageIntegrationProvider storageIntegrationProvider;
- @Inject Instance<DataSource> dataSource;
- @Inject RelationalJdbcConfiguration relationalJdbcConfiguration;
+ @Inject DatasourceOperations datasourceOperations;
@Inject RealmConfig realmConfig;
protected JdbcMetaStoreManagerFactory() {}
+ @Produces
+ @ApplicationScoped
+ DatasourceOperations produceDatasourceOperations(
Review Comment:
Should it be `static`? `JdbcMetaStoreManagerFactory` itself requires
`DatasourceOperations` to be injected... it looks like a dependency loop 🤔
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -67,19 +69,31 @@ public class JdbcMetaStoreManagerFactory implements
MetaStoreManagerFactory {
private static final Logger LOGGER =
LoggerFactory.getLogger(JdbcMetaStoreManagerFactory.class);
- final Map<String, PolarisMetaStoreManager> metaStoreManagerMap = new
HashMap<>();
- final Map<String, EntityCache> entityCacheMap = new HashMap<>();
- final Map<String, Supplier<BasePersistence>> sessionSupplierMap = new
HashMap<>();
+ // Stateful per-realm cache — InMemoryEntityCache accumulates entries across
requests
+ final Map<String, EntityCache> entityCacheMap = new ConcurrentHashMap<>();
+
+ // Cached per-realm schema version — loaded from DB once, stable at runtime
+ private final ConcurrentHashMap<String, Integer> schemaVersionCache = new
ConcurrentHashMap<>();
+
+ // Tracks realms that have already passed the bootstrap verification check
+ // (checkPolarisServiceBootstrappedForRealm), avoiding redundant DB hits on
subsequent calls.
+ private final Set<String> verifiedRealms = ConcurrentHashMap.newKeySet();
@Inject Clock clock;
@Inject PolarisDiagnostics diagnostics;
@Inject PolarisStorageIntegrationProvider storageIntegrationProvider;
- @Inject Instance<DataSource> dataSource;
- @Inject RelationalJdbcConfiguration relationalJdbcConfiguration;
+ @Inject DatasourceOperations datasourceOperations;
@Inject RealmConfig realmConfig;
protected JdbcMetaStoreManagerFactory() {}
+ @Produces
+ @ApplicationScoped
+ DatasourceOperations produceDatasourceOperations(
Review Comment:
Reuse by other classes may need to be addressed in #3960 🤔 For this PR it
looks ok to me.
--
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]
