adutra commented on code in PR #4115:
URL: https://github.com/apache/polaris/pull/4115#discussion_r3476154815
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/CommitMetricsRecord.java:
##########
@@ -25,101 +25,55 @@
/**
* Backend-agnostic representation of an Iceberg commit metrics report.
*
- * <p>This record captures all relevant metrics from an Iceberg {@code
CommitReport} along with
- * contextual information such as catalog identification and table location.
- *
- * <p>Common identification fields are inherited from {@link
MetricsRecordIdentity}.
- *
- * <p>Note: Realm ID is not included in this record. Multi-tenancy realm
context should be obtained
- * from the CDI-injected {@code RealmContext} at persistence time.
- *
* <p><b>Note:</b> This type is part of the experimental Metrics Persistence
SPI and may change in
* future releases.
*/
@Beta
@PolarisImmutable
public interface CommitMetricsRecord extends MetricsRecordIdentity {
- // === Commit Context ===
-
- /** Snapshot ID created by this commit. */
Review Comment:
Same, not sure why the javadocs are gone.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/RbacOperationSemantics.java:
##########
@@ -325,6 +327,7 @@ private static void register(
// Metrics and notifications
register(REPORT_READ_METRICS, TABLE_READ_DATA);
+ register(LIST_TABLE_METRICS, TABLE_READ_METRICS);
Review Comment:
Don't you need to also update `RangerPolarisOperationSemantics`?
##########
runtime/service/build.gradle.kts:
##########
@@ -204,6 +205,10 @@ dependencies {
testFixturesImplementation("com.azure:azure-storage-blob")
testFixturesImplementation("com.azure:azure-storage-file-datalake")
+ // Provides jakarta.ws.rs.ext.RuntimeDelegate needed to build Response
objects in plain unit tests
+ testRuntimeOnly(enforcedPlatform(libs.quarkus.bom))
Review Comment:
nit: we already have
`testImplementation(enforcedPlatform(libs.quarkus.bom))` on line 141. I'd
suggest moving these lines up there to put all quarkus dependencies together.
(I also wonder if resteasy-reactive isn't already included?)
##########
gradle/projects.main.properties:
##########
@@ -49,8 +50,10 @@ polaris-extensions-federation-hive=extensions/federation/hive
polaris-extensions-federation-bigquery=extensions/federation/bigquery
polaris-extensions-auth-opa=extensions/auth/opa/impl
polaris-extensions-auth-opa-tests=extensions/auth/opa/tests
+polaris-extensions-metrics-reports-spi=extensions/metrics-reports/spi
Review Comment:
nit: could we keep the alphabetical ordering inside polaris extensions
projects?
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -198,7 +198,8 @@ public synchronized Map<String, PrincipalSecretsResult>
bootstrapRealms(
PolarisMetaStoreManager metaStoreManager = createNewMetaStoreManager();
JdbcBasePersistenceImpl metaStore =
createSession(realm, bootstrapOptions.rootCredentialsSet(), true);
- PolarisCallContext polarisContext = new
PolarisCallContext(realmContext, metaStore);
+ PolarisCallContext polarisContext =
+ new PolarisCallContext(realmContext, metaStore, new
MetricsPersistence() {});
Review Comment:
nit: shouldn't you call `getOrCreateMetricsPersistence(realmContext)`?
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/ScanMetricsRecord.java:
##########
@@ -26,99 +26,55 @@
/**
* Backend-agnostic representation of an Iceberg scan metrics report.
*
- * <p>This record captures all relevant metrics from an Iceberg {@code
ScanReport} along with
- * contextual information such as catalog identification and table location.
- *
- * <p>Common identification fields are inherited from {@link
MetricsRecordIdentity}.
- *
- * <p>Note: Realm ID is not included in this record. Multi-tenancy realm
context should be obtained
- * from the CDI-injected {@code RealmContext} at persistence time.
- *
* <p><b>Note:</b> This type is part of the experimental Metrics Persistence
SPI and may change in
* future releases.
*/
@Beta
@PolarisImmutable
public interface ScanMetricsRecord extends MetricsRecordIdentity {
- // === Scan Context ===
-
- /** Snapshot ID that was scanned. */
Review Comment:
nit: why remove the javadocs?
--
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]
