Copilot commented on code in PR #4397:
URL: https://github.com/apache/polaris/pull/4397#discussion_r3318924551
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -236,20 +250,12 @@ 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) {
+ return createJdbcPersistence(realmContext);
}
Review Comment:
For JDBC, `getOrCreateSession()` and `getOrCreateMetricsPersistence()` each
create a new `JdbcBasePersistenceImpl`. When callers request both (as many now
do), this doubles work and can repeat bootstrap verification/config evaluation
per request. Since `JdbcBasePersistenceImpl` already implements
`MetricsPersistence`, consider implementing `getOrCreateMetricsPersistence()`
by delegating to (and reusing) the session instance (e.g., obtain once and
return/cast) or restructure the producer sites to pass the same created session
as both metastore and metrics persistence.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##########
@@ -34,6 +35,13 @@ public interface MetaStoreManagerFactory {
BasePersistence getOrCreateSession(RealmContext realmContext);
+ /**
+ * Returns the per-realm {@link MetricsPersistence}. This SPI is decoupled
from {@link
+ * BasePersistence} so backends that do not implement metrics persistence
can simply return a
+ * no-op instance.
+ */
+ MetricsPersistence getOrCreateMetricsPersistence(RealmContext realmContext);
Review Comment:
Adding a new abstract method to a public SPI (`MetaStoreManagerFactory`) is
a source/binary breaking change for any out-of-tree implementations. Consider
making this a `default` method that returns a shared no-op `MetricsPersistence`
(and document overriding it for real implementations), so external backends
don’t fail to compile when upgrading.
##########
polaris-core/src/test/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManagerRefreshTest.java:
##########
@@ -57,7 +58,7 @@ public class AtomicOperationMetaStoreManagerRefreshTest {
public void setUp() {
diagnostics = new PolarisDefaultDiagServiceImpl();
metaStore = Mockito.mock(BasePersistence.class);
- callCtx = new PolarisCallContext(() -> "testRealm", metaStore);
+ callCtx = new PolarisCallContext(() -> "testRealm", metaStore, new
MetricsPersistence() {});
Review Comment:
For tests, prefer a reusable no-op constant or a
`Mockito.mock(MetricsPersistence.class)` over `new MetricsPersistence() {}`.
This makes intent clearer and avoids the test becoming brittle if
`MetricsPersistence` later adds non-default methods.
##########
runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java:
##########
@@ -136,8 +135,11 @@ public CallContext polarisCallContext(
RealmContext realmContext,
RealmConfigurationSource configurationSource,
MetaStoreManagerFactory metaStoreManagerFactory) {
- BasePersistence metaStoreSession =
metaStoreManagerFactory.getOrCreateSession(realmContext);
- return new PolarisCallContext(realmContext, metaStoreSession,
configurationSource);
+ return new PolarisCallContext(
+ realmContext,
+ metaStoreManagerFactory.getOrCreateSession(realmContext),
+ metaStoreManagerFactory.getOrCreateMetricsPersistence(realmContext),
+ configurationSource);
Review Comment:
This constructs the call context by calling the factory twice; for backends
where both methods create fresh stateless persistence instances (e.g., JDBC in
this PR), this results in two separate persistence objects per request. To
avoid redundant object creation/bootstrapping checks, consider creating the
session once and reusing it for metrics when it implements `MetricsPersistence`
(or adjust the factory API to return a single per-realm composite/holder so
both dependencies are sourced consistently).
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisCallContext.java:
##########
@@ -25,52 +25,93 @@
import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.context.RealmContext;
import org.apache.polaris.core.persistence.BasePersistence;
+import org.apache.polaris.core.persistence.metrics.MetricsPersistence;
import org.jspecify.annotations.NonNull;
/**
* The Call context is allocated each time a new REST request is processed. It
contains instances of
- * low-level services required to process that request
+ * low-level services required to process that request.
+ *
+ * <p>{@link BasePersistence} continues to carry the bulk of the metastore SPI
surface (and still
+ * extends {@code PolicyMappingPersistence} / acts as the {@code
IntegrationPersistence} via a
+ * runtime cast for now). {@link MetricsPersistence} is intentionally
decoupled and supplied
+ * separately so callers that only need metrics persistence do not have to
depend on {@link
+ * BasePersistence}.
*/
public class PolarisCallContext implements CallContext {
// meta store which is used to persist Polaris entity metadata
private final BasePersistence metaStore;
+ private final MetricsPersistence metricsPersistence;
private final RealmConfigurationSource configurationSource;
private final RealmContext realmContext;
private final RealmConfig realmConfig;
/**
* @deprecated Use {@link
PolarisCallContext#PolarisCallContext(RealmContext, BasePersistence,
- * RealmConfigurationSource)}.
+ * MetricsPersistence, RealmConfigurationSource)}.
*/
@SuppressWarnings("removal")
@Deprecated(forRemoval = true)
public PolarisCallContext(
@NonNull RealmContext realmContext,
@NonNull BasePersistence metaStore,
@NonNull PolarisConfigurationStore configurationStore) {
- this(realmContext, metaStore, configurationStore::getConfiguration);
+ this(
+ realmContext, metaStore, new MetricsPersistence() {},
configurationStore::getConfiguration);
+ }
Review Comment:
Several places instantiate anonymous no-op `MetricsPersistence` (`new
MetricsPersistence() {}`), which spreads the no-op policy and allocates new
instances. Consider introducing a single shared constant (e.g.,
`MetricsPersistence NO_OP = new MetricsPersistence() {};`) in a central place
and reusing it here (and in tests) to keep behavior consistent and avoid
repeated allocations.
--
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]
