dimas-b commented on code in PR #3385: URL: https://github.com/apache/polaris/pull/3385#discussion_r2819640471
########## runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapMetricsCommand.java: ########## @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.admintool; + +import io.smallrye.common.annotation.Identifier; +import jakarta.inject.Inject; +import java.util.List; +import org.apache.polaris.core.persistence.metrics.MetricsSchemaBootstrap; +import picocli.CommandLine; + +/** + * CLI command to bootstrap the metrics schema independently from the entity schema. + * + * <p>This command allows operators to add metrics persistence support to an existing Polaris + * deployment without re-bootstrapping the entity schema. It is idempotent - running it multiple + * times on the same realm has no effect after the first successful run. + * + * <p>Example usage: + * + * <pre>{@code + * polaris-admin bootstrap-metrics -r my-realm + * polaris-admin bootstrap-metrics -r realm1 -r realm2 + * }</pre> + */ [email protected]( + name = "bootstrap-metrics", + mixinStandardHelpOptions = true, + description = "Bootstraps the metrics schema for existing realms.") +public class BootstrapMetricsCommand extends BaseCommand { + + @Inject + @Identifier("relational-jdbc") + MetricsSchemaBootstrap metricsSchemaBootstrap; + + @CommandLine.Option( + names = {"-r", "--realm"}, + paramLabel = "<realm>", + required = true, + description = "The name of a realm to bootstrap metrics for.") + List<String> realms; + + @Override + public Integer call() { + boolean success = true; + + for (String realm : realms) { + try { + if (metricsSchemaBootstrap.isBootstrapped(realm)) { + spec.commandLine() + .getOut() + .printf("Metrics schema already bootstrapped for realm '%s'. Skipping.%n", realm); + } else { + spec.commandLine() + .getOut() + .printf("Bootstrapping metrics schema for realm '%s'...%n", realm); + metricsSchemaBootstrap.bootstrap(realm); Review Comment: What about schema version?.. at some point we're going to have a v2 schema, I'm sure 🤔 I'd propose to add an optional version parameter and default to the latest version, if not specified. ########## persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetricsSchemaBootstrap.java: ########## @@ -0,0 +1,104 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.persistence.relational.jdbc; + +import java.sql.SQLException; +import java.util.List; +import org.apache.polaris.core.persistence.metrics.MetricsSchemaBootstrap; +import org.apache.polaris.persistence.relational.jdbc.models.MetricsSchemaVersion; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * JDBC implementation of {@link MetricsSchemaBootstrap}. + * + * <p>This implementation creates the metrics schema tables (scan_metrics_report, + * commit_metrics_report, metrics_version) in the configured JDBC database. + * + * <p>The metrics schema is separate from the entity schema and can be bootstrapped independently. + * This allows operators to add metrics support to existing Polaris deployments without + * re-bootstrapping the entity schema. + */ +public class JdbcMetricsSchemaBootstrap implements MetricsSchemaBootstrap { + + private static final Logger LOGGER = LoggerFactory.getLogger(JdbcMetricsSchemaBootstrap.class); + + /** Current metrics schema version. */ + private static final int METRICS_SCHEMA_VERSION = 1; + + private final DatasourceOperations datasourceOperations; + + public JdbcMetricsSchemaBootstrap(DatasourceOperations datasourceOperations) { + this.datasourceOperations = datasourceOperations; + } + + @Override + public void bootstrap(String realmId) { + if (isBootstrapped(realmId)) { + LOGGER.debug("Metrics schema already bootstrapped for realm: {}", realmId); + return; Review Comment: What about upgrading the schema? With the main metastore, it's usually done by re-invoking the bootstrap command via the Admin CLI. ########## runtime/service/src/test/java/org/apache/polaris/service/events/listeners/inmemory/InMemoryBufferEventListenerIntegrationTest.java: ########## @@ -117,10 +119,32 @@ public void setup( baseLocation = IntegrationTestsHelper.getTemporaryDirectory(tempDir).resolve(realm + "/"); } + /** + * Reset the database state before each test to ensure test isolation. The H2 in-memory database + * with DB_CLOSE_DELAY=-1 persists state across tests, so we need to clean up catalog-related + * entities while preserving the realm and principal entities set up in @BeforeAll. + */ + @BeforeEach + public void resetDatabaseState() { Review Comment: Is this needed specifically for metrics persistence? ########## runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java: ########## @@ -224,6 +227,55 @@ public PolarisMetaStoreManager polarisMetaStoreManager( return metaStoreManagerFactory.getOrCreateMetaStoreManager(realmContext); } + /** + * Produces a no-op {@link MetricsPersistence} bean. + * + * <p>This bean is selected when {@code polaris.persistence.metrics.type} is set to {@code "noop"} + * (the default). All write operations are silently ignored, and all query operations return empty + * pages. + * + * @return the no-op MetricsPersistence singleton + */ + @Produces + @Identifier("noop") + public MetricsPersistence noopMetricsPersistence() { + return MetricsPersistence.NOOP; + } + + /** + * Produces a no-op {@link MetricsSchemaBootstrap} bean. + * + * <p>This bean is selected for backends that don't support metrics schema bootstrap. The {@link + * MetricsSchemaBootstrap#bootstrap(String)} method does nothing, and {@link + * MetricsSchemaBootstrap#isBootstrapped(String)} always returns {@code true}. + * + * @return the no-op MetricsSchemaBootstrap singleton + */ + @Produces + @Identifier("noop") + public MetricsSchemaBootstrap noopMetricsSchemaBootstrap() { + return MetricsSchemaBootstrap.NOOP; + } + + /** + * Produces a {@link MetricsPersistence} bean for the current request. + * + * <p>This method selects a MetricsPersistence implementation based on the configured metrics + * persistence type. The type is configured independently from the entity metastore via {@code + * polaris.persistence.metrics.type}. + * + * @param config the metrics persistence configuration + * @param metricsPersistenceImpls all available MetricsPersistence implementations + * @return a MetricsPersistence implementation for the current realm + */ + @Produces + @RequestScoped + public MetricsPersistence metricsPersistence( + MetricsPersistenceConfiguration config, + @Any Instance<MetricsPersistence> metricsPersistenceImpls) { + return metricsPersistenceImpls.select(Identifier.Literal.of(config.type())).get(); Review Comment: Same for `metricsReporter` (sorry, I did not notice it before). ########## persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetricsPersistenceProducer.java: ########## @@ -0,0 +1,123 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.persistence.relational.jdbc; + +import io.smallrye.common.annotation.Identifier; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.enterprise.context.RequestScoped; +import jakarta.enterprise.inject.Instance; +import jakarta.enterprise.inject.Produces; +import jakarta.inject.Inject; +import java.sql.SQLException; +import javax.sql.DataSource; +import org.apache.polaris.core.config.BehaviorChangeConfiguration; +import org.apache.polaris.core.config.RealmConfig; +import org.apache.polaris.core.context.RealmContext; +import org.apache.polaris.core.persistence.metrics.MetricsPersistence; +import org.apache.polaris.core.persistence.metrics.MetricsSchemaBootstrap; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * CDI producer for {@link MetricsPersistence} in the JDBC persistence backend. + * + * <p>This producer creates {@link JdbcMetricsPersistence} instances when the JDBC persistence + * backend is in use. When metrics tables are not available (schema version < 4), the produced + * instance will report this via {@link JdbcMetricsPersistence#supportsMetricsPersistence()}. + */ +@ApplicationScoped +@Identifier("relational-jdbc") Review Comment: Is this `@Identifier` needed? It does not look like anything requests `JdbcMetricsPersistenceProducer` injected at all.... Quarkus should be able to find it by the `@Produces` annotation, I think 🤔 ########## persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java: ########## @@ -78,6 +79,10 @@ public class JdbcMetaStoreManagerFactory implements MetaStoreManagerFactory { @Inject RelationalJdbcConfiguration relationalJdbcConfiguration; @Inject RealmConfig realmConfig; + @Inject + @Identifier("relational-jdbc") + MetricsSchemaBootstrap metricsSchemaBootstrap; Review Comment: this appears to be unused 🤔 ########## persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetricsPersistence.java: ########## @@ -0,0 +1,475 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.persistence.relational.jdbc; + +import static org.apache.polaris.persistence.relational.jdbc.QueryGenerator.PreparedQuery; + +import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; +import java.sql.SQLException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.stream.Collectors; +import org.apache.polaris.core.persistence.metrics.CommitMetricsRecord; +import org.apache.polaris.core.persistence.metrics.MetricsPersistence; +import org.apache.polaris.core.persistence.metrics.MetricsQueryCriteria; +import org.apache.polaris.core.persistence.metrics.ReportIdToken; +import org.apache.polaris.core.persistence.metrics.ScanMetricsRecord; +import org.apache.polaris.core.persistence.pagination.Page; +import org.apache.polaris.core.persistence.pagination.PageToken; +import org.apache.polaris.core.persistence.pagination.Token; +import org.apache.polaris.persistence.relational.jdbc.models.ModelCommitMetricsReport; +import org.apache.polaris.persistence.relational.jdbc.models.ModelCommitMetricsReportConverter; +import org.apache.polaris.persistence.relational.jdbc.models.ModelScanMetricsReport; +import org.apache.polaris.persistence.relational.jdbc.models.ModelScanMetricsReportConverter; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * JDBC implementation of {@link MetricsPersistence}. + * + * <p>This class provides direct JDBC persistence for metrics reports, converting between SPI record + * types ({@link ScanMetricsRecord}, {@link CommitMetricsRecord}) and JDBC model types ({@link + * ModelScanMetricsReport}, {@link ModelCommitMetricsReport}). + * + * <p>Metrics tables (scan_metrics_report, commit_metrics_report) were introduced in schema version + * 4. On older schemas, all operations are no-ops. + */ +public class JdbcMetricsPersistence implements MetricsPersistence { + + private static final Logger LOGGER = LoggerFactory.getLogger(JdbcMetricsPersistence.class); + + // Minimum schema version that includes metrics tables + private static final int METRICS_TABLES_MIN_SCHEMA_VERSION = 4; + + private final DatasourceOperations datasourceOperations; + private final String realmId; + private final int schemaVersion; + + /** + * Creates a new JdbcMetricsPersistence instance. + * + * @param datasourceOperations the datasource operations for JDBC access + * @param realmId the realm ID for multi-tenancy + * @param schemaVersion the current schema version + */ + public JdbcMetricsPersistence( + DatasourceOperations datasourceOperations, String realmId, int schemaVersion) { + this.datasourceOperations = datasourceOperations; + this.realmId = realmId; + this.schemaVersion = schemaVersion; + } + + /** + * Returns true if the current schema version supports metrics persistence tables. + * + * @return true if schema version >= 4, false otherwise + */ + @Override + public boolean supportsMetricsPersistence() { + return this.schemaVersion >= METRICS_TABLES_MIN_SCHEMA_VERSION; + } + + @Override + public void writeScanReport(@Nonnull ScanMetricsRecord record) { + if (!supportsMetricsPersistence()) { + LOGGER.debug( + "Schema version {} does not support metrics tables. Skipping scan metrics write.", + schemaVersion); + return; + } + ModelScanMetricsReport model = SpiModelConverter.toModelScanReport(record, realmId); + writeScanMetricsReport(model); + } + + @Override + public void writeCommitReport(@Nonnull CommitMetricsRecord record) { + if (!supportsMetricsPersistence()) { + LOGGER.debug( Review Comment: Maybe WARN? It's probably a setup mistake, so the Admin user ought to be notified, I guess. If no persistence were intended, this code should not have been selected by CDI. ########## runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java: ########## @@ -224,6 +227,55 @@ public PolarisMetaStoreManager polarisMetaStoreManager( return metaStoreManagerFactory.getOrCreateMetaStoreManager(realmContext); } + /** + * Produces a no-op {@link MetricsPersistence} bean. + * + * <p>This bean is selected when {@code polaris.persistence.metrics.type} is set to {@code "noop"} + * (the default). All write operations are silently ignored, and all query operations return empty + * pages. + * + * @return the no-op MetricsPersistence singleton + */ + @Produces + @Identifier("noop") + public MetricsPersistence noopMetricsPersistence() { + return MetricsPersistence.NOOP; + } + + /** + * Produces a no-op {@link MetricsSchemaBootstrap} bean. + * + * <p>This bean is selected for backends that don't support metrics schema bootstrap. The {@link + * MetricsSchemaBootstrap#bootstrap(String)} method does nothing, and {@link + * MetricsSchemaBootstrap#isBootstrapped(String)} always returns {@code true}. + * + * @return the no-op MetricsSchemaBootstrap singleton + */ + @Produces + @Identifier("noop") + public MetricsSchemaBootstrap noopMetricsSchemaBootstrap() { + return MetricsSchemaBootstrap.NOOP; + } + + /** + * Produces a {@link MetricsPersistence} bean for the current request. + * + * <p>This method selects a MetricsPersistence implementation based on the configured metrics + * persistence type. The type is configured independently from the entity metastore via {@code + * polaris.persistence.metrics.type}. + * + * @param config the metrics persistence configuration + * @param metricsPersistenceImpls all available MetricsPersistence implementations + * @return a MetricsPersistence implementation for the current realm + */ + @Produces + @RequestScoped + public MetricsPersistence metricsPersistence( + MetricsPersistenceConfiguration config, + @Any Instance<MetricsPersistence> metricsPersistenceImpls) { + return metricsPersistenceImpls.select(Identifier.Literal.of(config.type())).get(); Review Comment: Since this bean is request-scoped, but config is application-scoped, it might be worth introducing an intermediate "builder" or "factory" object. The Builder will be @ApplicationScoped so that selectors only need to be resolved once per startup. The Builder will be making light-weight MetricsPersistence from already resolved beans. WDYT? Basically the idea is to resolve all "heavy" CDI piece once per startup (`DataSource` in this case), and have light-weight producers for request-scoped objects. ########## persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetricsSchemaBootstrap.java: ########## @@ -0,0 +1,104 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.persistence.relational.jdbc; + +import java.sql.SQLException; +import java.util.List; +import org.apache.polaris.core.persistence.metrics.MetricsSchemaBootstrap; +import org.apache.polaris.persistence.relational.jdbc.models.MetricsSchemaVersion; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * JDBC implementation of {@link MetricsSchemaBootstrap}. + * + * <p>This implementation creates the metrics schema tables (scan_metrics_report, + * commit_metrics_report, metrics_version) in the configured JDBC database. + * + * <p>The metrics schema is separate from the entity schema and can be bootstrapped independently. + * This allows operators to add metrics support to existing Polaris deployments without + * re-bootstrapping the entity schema. + */ +public class JdbcMetricsSchemaBootstrap implements MetricsSchemaBootstrap { + + private static final Logger LOGGER = LoggerFactory.getLogger(JdbcMetricsSchemaBootstrap.class); + + /** Current metrics schema version. */ + private static final int METRICS_SCHEMA_VERSION = 1; + + private final DatasourceOperations datasourceOperations; + + public JdbcMetricsSchemaBootstrap(DatasourceOperations datasourceOperations) { + this.datasourceOperations = datasourceOperations; + } + + @Override + public void bootstrap(String realmId) { + if (isBootstrapped(realmId)) { + LOGGER.debug("Metrics schema already bootstrapped for realm: {}", realmId); + return; + } + + LOGGER.info("Bootstrapping metrics schema v{} for realm: {}", METRICS_SCHEMA_VERSION, realmId); + + try { + datasourceOperations.executeScript( + datasourceOperations.getDatabaseType().openMetricsSchemaResource(METRICS_SCHEMA_VERSION)); + LOGGER.info( + "Successfully bootstrapped metrics schema v{} for realm: {}", + METRICS_SCHEMA_VERSION, + realmId); + } catch (SQLException e) { + throw new RuntimeException( + String.format( + "Failed to bootstrap metrics schema for realm '%s': %s", realmId, e.getMessage()), + e); + } + } + + @Override + public boolean isBootstrapped(String realmId) { + return loadMetricsSchemaVersion() > 0; + } + + /** + * Loads the current metrics schema version from the database. + * + * @return the metrics schema version, or 0 if not bootstrapped + */ + int loadMetricsSchemaVersion() { + QueryGenerator.PreparedQuery query = QueryGenerator.generateMetricsVersionQuery(); + try { + List<MetricsSchemaVersion> versions = + datasourceOperations.executeSelect(query, new MetricsSchemaVersion()); + if (versions == null || versions.isEmpty()) { + return 0; + } + return versions.getFirst().getValue(); Review Comment: Should it be an error if there are more that one version record? ########## runtime/admin/src/test/java/org/apache/polaris/admintool/relational/jdbc/RelationalJdbcBootstrapMetricsCommandTest.java: ########## @@ -0,0 +1,101 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.admintool.relational.jdbc; + +import static org.assertj.core.api.Assertions.assertThat; + +import io.quarkus.test.junit.TestProfile; +import io.quarkus.test.junit.main.LaunchResult; +import io.quarkus.test.junit.main.QuarkusMainLauncher; +import org.apache.polaris.admintool.BootstrapMetricsCommandTestBase; +import org.junit.jupiter.api.Test; + +/** + * JDBC-specific tests for {@link org.apache.polaris.admintool.BootstrapMetricsCommand}. + * + * <p>These tests verify the bootstrap-metrics command works correctly with the JDBC persistence + * backend. + * + * <p><b>Note:</b> Tests that require state persistence across multiple {@code launcher.launch()} + * calls are not possible with the current test framework because each launch gets a fresh + * PostgreSQL database. See the TODO comment in {@link + * RelationalJdbcBootstrapCommandTest#testBootstrapFailsWhenAddingRealmWithDifferentSchemaVersion} + * for details. + */ +@TestProfile(RelationalJdbcAdminProfile.class) +public class RelationalJdbcBootstrapMetricsCommandTest extends BootstrapMetricsCommandTestBase { + + @Test + public void testBootstrapMetricsForSingleRealm(QuarkusMainLauncher launcher) { + // Bootstrap entity schema and metrics schema in one launch using --include-metrics. + // Note: Each launcher.launch() gets a fresh database, so we use --include-metrics + // to bootstrap both entity and metrics schema in a single launch. + LaunchResult bootstrapResult = + launcher.launch( + "bootstrap", + "-r", + "metrics-realm1", + "-c", + "metrics-realm1,root,s3cr3t", + "--include-metrics"); + assertThat(bootstrapResult.exitCode()).isEqualTo(0); + assertThat(bootstrapResult.getOutput()) + .contains("Realm 'metrics-realm1' successfully bootstrapped."); + } + + @Test + public void testBootstrapMetricsMultipleRealmsInSingleLaunch(QuarkusMainLauncher launcher) { + // Bootstrap entity schema and metrics schema for multiple realms in one launch + LaunchResult result = + launcher.launch( + "bootstrap", + "-r", + "metrics-realm3", + "-r", + "metrics-realm4", + "-c", + "metrics-realm3,root,s3cr3t", + "-c", + "metrics-realm4,root,s3cr3t", + "--include-metrics"); + assertThat(result.exitCode()).isEqualTo(0); + assertThat(result.getOutput()) + .contains("Realm 'metrics-realm3' successfully bootstrapped.") + .contains("Realm 'metrics-realm4' successfully bootstrapped.") + .contains("Bootstrap completed successfully."); + } + + // TODO: Enable these tests once we enable postgres container reuse across launches. + // See + // RelationalJdbcBootstrapCommandTest#testBootstrapFailsWhenAddingRealmWithDifferentSchemaVersion + // + // @Test + // public void testBootstrapMetricsIdempotent(QuarkusMainLauncher launcher) { Review Comment: I believe you can run CLI commands multiple times in the same test... Each `launch()` call runs a fresh command from scratch, but `TestResourceEntry`'s are the same within each test method.... IIRC.... Does it not work somehow? ########## persistence/relational-jdbc/build.gradle.kts: ########## @@ -29,10 +29,16 @@ dependencies { compileOnly(platform(libs.jackson.bom)) compileOnly("com.fasterxml.jackson.core:jackson-annotations") + compileOnly("com.fasterxml.jackson.core:jackson-databind") compileOnly(libs.jakarta.annotation.api) compileOnly(libs.jakarta.enterprise.cdi.api) compileOnly(libs.jakarta.inject.api) + // Iceberg API for metrics report conversion + compileOnly(platform(libs.iceberg.bom)) + compileOnly("org.apache.iceberg:iceberg-api") + compileOnly("org.apache.iceberg:iceberg-core") Review Comment: Might as well make it a proper `implementation` dependency, I guess 🤔 ########## persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/RelationalJdbcProductionReadinessChecks.java: ########## @@ -55,4 +58,49 @@ public ProductionReadinessCheck checkRelationalJdbc( } return ProductionReadinessCheck.OK; } + + /** + * Checks if metrics persistence is properly configured for the relational-jdbc backend. + * + * <p>When the metrics persistence type is set to 'relational-jdbc', this check verifies that: + * + * <ol> + * <li>The MetricsPersistence implementation is actually JdbcMetricsPersistence + * <li>The metrics tables have been bootstrapped in the database + * </ol> + * + * <p>This ensures that the system is properly configured before attempting to persist metrics. + */ + @Produces + public ProductionReadinessCheck checkMetricsPersistenceBootstrapped( + @Any Instance<MetricsPersistence> metricsPersistenceImpls, + Instance<DataSource> dataSource, + RelationalJdbcConfiguration relationalJdbcConfiguration) { + + // Check if relational-jdbc metrics persistence implementation is available + Instance<MetricsPersistence> jdbcMetricsInstance = + metricsPersistenceImpls.select(Identifier.Literal.of("relational-jdbc")); Review Comment: I think we should probably inject a pre-resolved `MetricsPersistence` object here and just check its type as in `checkRelationalJdbc`. The idea is to skip this check is some other metrics persistence is configured, but selecting `relational-jdbc` we force CDI to attempt to resolve it, even if the admin user did not intend so. If you switch to using a CDI builder / factory for `MetricsPersistence`, that same factory could be a producer for `ProductionReadinessCheck`, in which case not type casts should be necessary... I hope it'll work this way 😅 -- 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]
