obelix74 commented on PR #4115: URL: https://github.com/apache/polaris/pull/4115#issuecomment-4702170099
> Thanks for continuing to push this forward. Before this merges, I want to explicitly call out the latest alignment from the metrics architecture sync notes, because I think it changes how this PR should be scoped: https://docs.google.com/document/d/100h7c4damrUzVuquYbBHM0EvA4LSWuW2IT2dN_7nYVA/edit?pli=1&tab=t.k96s2xyqr5u1#heading=h.uvb454otvxc0 > > My main concern is that this PR defines the SPI at the wrong layer. > > `PolarisMetricsReporter` is currently described as the metrics reporting SPI, but it lives in `runtime/service` and is shaped around CDI/runtime wiring. That makes it hard for it to serve as the stable Polaris-facing contract for metrics ingestion, which should be runtime/framework agnostic. I think this contract should instead live in `polaris-core`, where the rest of Polaris can depend on it without depending on the Quarkus/runtime wiring layer. > > At the same time, this PR expands `MetricsPersistence` under `polaris-core` and wires it through `BasePersistence`. Although #4397 already covers decomposing it from `BasePersistence`, `MetricsPersistence` would still dangle under `polaris-core`. Since durable metrics persistence is now being treated as an extension implementation, I think this durable persistence contract/data model should move under `extensions` and be self-contained there. > > So to me the layering should be: > > 1. `polaris-core`: stable Iceberg metrics reporting SPI contract. > 2. `runtime/service`: REST ingestion, table resolution, authz, and implementation selection. > 3. `extensions/...`: concrete implementations, including the default no-op/log-only battery and the durable JDBC-backed implementation. > 4. durable metrics extension: owns its own persistence contract, data model, schema/bootstrap, retention, and read/query backing. > > Could we split this PR into two scopes? > > **First scope: core-level metrics reporting SPI + default battery** > > Define the Iceberg metrics reporting SPI in `polaris-core`, not in `runtime/service`, and not as `MetricsPersistence` inherited by `BasePersistence`. The contract can stay small: a Polaris-resolved metrics context plus the Iceberg `MetricsReport` payload. > > Runtime/service would still call the selected implementation after resolving and authorizing the table, but it would not define the SPI shape itself. > > The built-in battery can stay simple and safe: no-op/log-only by default (2 cents: preferred no-op), provided as the default extension implementation like `extensions/metrics-reports/no-op` (bundled for release via BOM), out of the box experience. > > **Second scope: durable Iceberg metrics extension implementation** > > Reintroduce durable metrics storage into a self-contained extension implementation. That implementation should own the durable data model, schema/bootstrap concerns, retention/query behavior, and read API backing. Concretely, I think the decomposed `MetricsPersistence` work from #4397 should move under this durable metrics extension path, like `extensions/metrics-reports/persistence/relational-jdbc`, rather than remaining under generic `metrics-reports/impl`. That keeps durable JDBC metrics as one implementation of the reporting SPI, instead of making metrics persistence part of the shared core metastore persistence shape. > > @dimas-b I'm trying to keep the useful work here moving, but with the SPI boundary corrected before we merge a shape that downstream implementations may start depending on. WDYT of my proposed plan? > > Anand, one point from the mailing-list thread that may be worth repeating here: I agree migration matters for durable metrics data. To me, that is another reason to keep the durable data model scoped to the durable metrics implementation, rather than making the current per-type JDBC schema and `BasePersistence` coupling part of the shared SPI shape. @flyingImer I think I have got all your comments. **Scope 1 changes: SPI layering correction** What changed 1. `IcebergMetricsReporter` moved to `polaris-core` — the stable reporting SPI is no longer inside runtime/service (CDI-coupled). `PolarisMetricsReporter`, `DefaultMetricsReporter`, and `PersistingMetricsReporter` are deleted from `runtime/service`. All references updated. 2. `MetricsPersistence` removed from `BasePersistence` — `BasePersistence` no longer extends `MetricsPersistence`; `PolarisMetaStoreManager` no longer extends `PolarisMetricsManager`. The durable metrics write/read path is gone from `JdbcBasePersistenceImpl`. The data models (`ModelScanMetricsReport`, `ModelCommitMetricsReport`, `MetricsReportToken`) are deleted from `persistence/relational-jdbc`. 3. Default battery in `extensions/metrics-reports/impl` — `NoOpMetricsReporter` (`@Identifier("no-op")`, default) and `LoggingMetricsReporter` (`@Identifier("log")`) live here. The default reporting type is now "no-op". 4. REST read path stubbed — `MetricsReportsService.listTableMetrics()` resolves and authorizes the table (full privilege check via `LIST_TABLE_METRICS` / `TABLE_READ_METRICS`), then returns an empty result set. Namespace decoding uses `NamespaceUtils.splitNamespace()`. 5. `CHANGELOG` corrected — removed the inaccurate "exposes persisted metrics" language; clarified that durable storage is a follow-up. **What's deferred (Scope 2)** Durable JDBC metrics persistence in a new self-contained module `extensions/metrics-reports/persistence/relational-jdbc`. It will own the data models, DDL bootstrap, write path (`@Identifier("persisting")` reporter), and the backing for the REST read API — without touching `BasePersistence` or `PolarisMetaStoreManager`. Should I create the Scope 2 PR based on this right now or should I wait? -- 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]
