You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@polaris.apache.org by "flyingImer (via GitHub)" <gi...@apache.org> on 2026/07/31 04:36:22 UTC

[PR] refactor(metrics): move IcebergMetricsReporter SPI to polaris-core [polaris]

flyingImer opened a new pull request, #5204:
URL: https://github.com/apache/polaris/pull/5204

   IcebergMetricsReporter is meant to be a plugin point: implement it your own way (log it, persist it, ship it to Kafka, whatever), and Polaris picks whichever one you've configured. PR #5068 built exactly that shape, but put the contract itself inside runtime/service, the Quarkus REST module, even though the interface has zero CDI, Quarkus, or JAX-RS in it, only guava, jspecify, and Iceberg's own types. I raised this in review ([github.com/apache/polaris/pull/5068#discussion_r3676236578](http://github.com/apache/polaris/pull/5068#discussion_r3676236578)), and this PR moves the contract to polaris-core, alongside every other extension point, and moves the default logging implementation out to extensions/metrics-reports/base, next to its peers NoOpMetricsReporter and PersistingMetricsReporter. Writing a new reporter, or picking one of the three shipped ones, no longer touches runtime/service at all.
   
   •   Out of the box, Polaris used to log every scan and commit report it received, whether you asked for it or not. Defaulting to the loud reporter was never really a decision, it was just whichever one happened to already live in runtime/service. I flipped the default to the no-op reporter (the config value itself is still "default", it now just resolves to a different reporter), so a fresh install stays quiet unless you explicitly opt into logging (polaris.iceberg-metrics.reporting.type=logging) or persistence (=persisting).
   
   •   Selecting the logging reporter used to take two steps: pick it via config, then separately bump a log category to actually see anything, because that category defaulted to OFF. That override existed to keep brand-new installs quiet back when logging was the automatic default; once picking it is a deliberate choice, the override doesn't protect anyone. I dropped it, so selecting "logging" now logs right away, the same as selecting "persisting" persists right away. The override had also drifted to point at the wrong package, a bug I fixed separately: it targeted org.apache.polaris.service.reporting, a bare config interface with no logger, instead of LoggingMetricsReporter's real logger category, which its own javadoc already named.
   
   
   ## Checklist
   - [x] 🛡️ Don't disclose security issues! (contact security@apache.org)
   - [x] 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
   - [x] 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
   - [ ] 💡 Added comments for complex logic
   - [x] 🧾 Updated `CHANGELOG.md` (if needed)
   - [ ] 📚 Updated documentation in `site/content/in-dev/unreleased` (if needed)
   


-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] refactor(metrics): move IcebergMetricsReporter SPI to polaris-core [polaris]

Posted by "adutra (via GitHub)" <gi...@apache.org>.

adutra commented on code in PR #5204:
URL: https://github.com/apache/polaris/pull/5204#discussion_r3704105971


##########
CHANGELOG.md:
##########
@@ -42,6 +42,11 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
   UPDATE polaris_schema.version SET version_value = 5 WHERE version_key = 'version';
   ```
   See the Relational JDBC metastore documentation for details.
+- The implicit default for `polaris.iceberg-metrics.reporting.type` now selects a no-op reporter
+  instead of a logging one, so Iceberg scan/commit reports received on the metrics endpoint are
+  silently discarded out of the box. `default` remains a valid, stable value for the property; it
+  now resolves to the no-op reporter. Deployments that want the previous logging behavior back

Review Comment:
   nit: slight repetition: "now selects a no-op reporter" + "now resolves to the no-op reporter".



##########
CHANGELOG.md:
##########
@@ -42,6 +42,11 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
   UPDATE polaris_schema.version SET version_value = 5 WHERE version_key = 'version';
   ```
   See the Relational JDBC metastore documentation for details.
+- The implicit default for `polaris.iceberg-metrics.reporting.type` now selects a no-op reporter
+  instead of a logging one, so Iceberg scan/commit reports received on the metrics endpoint are
+  silently discarded out of the box. `default` remains a valid, stable value for the property; it
+  now resolves to the no-op reporter. Deployments that want the previous logging behavior back
+  should set `polaris.iceberg-metrics.reporting.type=logging` explicitly.

Review Comment:
   We should also mention that setting the `org.apache.polaris.service.reporting` logger level is not required anymore.



##########
extensions/metrics-reports/base/build.gradle.kts:
##########
@@ -24,11 +24,12 @@ plugins {
 
 dependencies {
   implementation(project(":polaris-core"))
-  implementation(project(":polaris-runtime-service"))
 
   implementation(platform(libs.iceberg.bom))
   implementation("org.apache.iceberg:iceberg-api")
+  implementation("org.apache.iceberg:iceberg-core")

Review Comment:
   ```suggestion
   ```



-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] refactor(metrics): move IcebergMetricsReporter SPI to polaris-core [polaris]

Posted by "dimas-b (via GitHub)" <gi...@apache.org>.

dimas-b commented on code in PR #5204:
URL: https://github.com/apache/polaris/pull/5204#discussion_r3724940003


##########
polaris-core/src/main/java/org/apache/polaris/core/metrics/IcebergMetricsReporter.java:
##########
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.polaris.service.metrics;
+package org.apache.polaris.core.metrics;

Review Comment:
   I agree with the rationale about why this interface's placement inside `runtime/service` is not ideal, but I do not think `polaris-core` is an ideal home for it either.
   
   My rationale: This interface is not used / called by `polaris-core` code. Therefore it is extraneous to `polaris-core`.
   
   In my view, `polaris-core` is more of a library to be shared by servicing working with the Polaris data model. It is not a place to define SPI classes that effectively work at higher level (REST API in this case).
   
   Could adding a separate (new) module lead to a consensus? Something like `polaris-service-spi`?
   
   



-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] refactor(metrics): move IcebergMetricsReporter SPI to polaris-core [polaris]

Posted by "dimas-b (via GitHub)" <gi...@apache.org>.

dimas-b commented on code in PR #5204:
URL: https://github.com/apache/polaris/pull/5204#discussion_r3771034941


##########
polaris-core/src/main/java/org/apache/polaris/core/metrics/IcebergMetricsReporter.java:
##########
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.polaris.service.metrics;
+package org.apache.polaris.core.metrics;

Review Comment:
   Thanks for the recap, @flyingImer !
   
   Minor correction: discussions during in-person meetings cannot be "decisions" from the ASF process POV. However, since the proposal for `polaris-service-spi` is posted in this thread, we can assume lazy consensus if reviewers do not disagree :slightly_smiling_face: 



-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] refactor(metrics): move IcebergMetricsReporter SPI to polaris-core [polaris]

Posted by "dimas-b (via GitHub)" <gi...@apache.org>.

dimas-b commented on code in PR #5204:
URL: https://github.com/apache/polaris/pull/5204#discussion_r3724940003


##########
polaris-core/src/main/java/org/apache/polaris/core/metrics/IcebergMetricsReporter.java:
##########
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.polaris.service.metrics;
+package org.apache.polaris.core.metrics;

Review Comment:
   I agree with the rationale about why this interface's placement inside `runtime/service` is not ideal, but I do not think `polaris-core` is an ideal home for it either.
   
   My rationale: This interface is not used / called by `polaris-core` code. Therefore it is extraneous to `polaris-core`.
   
   In my view, `polaris-core` is more of a library to be shared by services working with the Polaris data model. It is not a place to define SPI classes that effectively work at higher level (REST API in this case).
   
   Could adding a separate (new) module lead to a consensus? Something like `polaris-service-spi`?
   
   



-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] refactor(metrics): move IcebergMetricsReporter SPI to polaris-core [polaris]

Posted by "flyingImer (via GitHub)" <gi...@apache.org>.

flyingImer commented on PR #5204:
URL: https://github.com/apache/polaris/pull/5204#issuecomment-5139314167

   @obelix74 ^^^


-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] refactor(metrics): move IcebergMetricsReporter SPI to polaris-core [polaris]

Posted by "flyingImer (via GitHub)" <gi...@apache.org>.

flyingImer commented on PR #5204:
URL: https://github.com/apache/polaris/pull/5204#issuecomment-5198407037

   Thanks @adutra incorporated your comments!


-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] refactor(metrics): move IcebergMetricsReporter SPI to polaris-core [polaris]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.

github-actions[bot] commented on PR #5204:
URL: https://github.com/apache/polaris/pull/5204#issuecomment-5657892957

   This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.


-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] refactor(metrics): move IcebergMetricsReporter SPI to polaris-core [polaris]

Posted by "flyingImer (via GitHub)" <gi...@apache.org>.

flyingImer commented on code in PR #5204:
URL: https://github.com/apache/polaris/pull/5204#discussion_r3777280539


##########
polaris-core/src/main/java/org/apache/polaris/core/metrics/IcebergMetricsReporter.java:
##########
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.polaris.service.metrics;
+package org.apache.polaris.core.metrics;

Review Comment:
   noted! thanks for the correction!



-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] refactor(metrics): move IcebergMetricsReporter SPI to polaris-core [polaris]

Posted by "dimas-b (via GitHub)" <gi...@apache.org>.

dimas-b commented on code in PR #5204:
URL: https://github.com/apache/polaris/pull/5204#discussion_r3771069144


##########
runtime/defaults/src/main/resources/application.properties:
##########
@@ -350,8 +350,6 @@ polaris.credential-manager.type=default
 
 # Configuration for the behaviour of the metrics endpoint
 polaris.iceberg-metrics.reporting.type=default
-# Set to INFO if you want to see iceberg metric reports logged
-quarkus.log.category."org.apache.polaris.service.reporting".level=OFF

Review Comment:
   Unrelated change?



##########
spi/src/main/java/org/apache/polaris/spi/substrate/metrics/IcebergMetricsReporter.java:
##########
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.polaris.service.metrics;
+package org.apache.polaris.spi.substrate.metrics;

Review Comment:
   What is `substrate`?
   
   Why not `org.apache.polaris.metrics.spi`?
   
   I believe the "owner" module ("metrics" in this case) has higher weight than "spi" because an SPI is needed by a particular module. I do not think we can talk about "general SPI" in Polaris. It is always grounded in a _particular_ need in a _particular_ module to access the pluggable service.



##########
gradle/projects.main.properties:
##########
@@ -20,6 +20,7 @@
 
 polaris-bom=bom
 polaris-core=polaris-core
+polaris-spi=spi

Review Comment:
   Similar to my other comment: `polaris-spi` feels too generic.
   
   In this case we're adding a module needed by `runtime/service`, which is why I proposed to name it `polaris-service-spi` (path: `runtime/service-spi`) in the old comment thread. "Service" here meaning REST Catalog services.
   
   Alternative: `polaris-rest-spi` (path: `spi/rest`).
   
   Alternative: `polaris-catalog-spi` (path: `spi/catalog`).
   
   WDYT?



-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] refactor(metrics): move IcebergMetricsReporter SPI to polaris-core [polaris]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.

github-actions[bot] closed pull request #5204: refactor(metrics): move IcebergMetricsReporter SPI to polaris-core
URL: https://github.com/apache/polaris/pull/5204


-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] refactor(metrics): move IcebergMetricsReporter SPI to polaris-core [polaris]

Posted by "flyingImer (via GitHub)" <gi...@apache.org>.

flyingImer commented on code in PR #5204:
URL: https://github.com/apache/polaris/pull/5204#discussion_r3770455067


##########
polaris-core/src/main/java/org/apache/polaris/core/metrics/IcebergMetricsReporter.java:
##########
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.polaris.service.metrics;
+package org.apache.polaris.core.metrics;

Review Comment:
   Just a note from the metrics syncup meeting: we have decided to put spis into the `polaris-service-spi` module, and keep entity data model in `core` module going forward. Currently ones, can leave as is until future needs. This also aligns with the incubating SPI arch vision doc: https://docs.google.com/document/d/1mj3vGpix9zTDTFFJKtWT5ZxI3PTGI1YNb5xFqyWrOkc/edit?tab=t.0 (only two things differs: 1) all the core/ reference in the doc would be replaced with spi/; 2) core/ only holds entity data models)



-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] refactor(metrics): move IcebergMetricsReporter SPI to polaris-core [polaris]

Posted by "dimas-b (via GitHub)" <gi...@apache.org>.

dimas-b commented on code in PR #5204:
URL: https://github.com/apache/polaris/pull/5204#discussion_r3771034941


##########
polaris-core/src/main/java/org/apache/polaris/core/metrics/IcebergMetricsReporter.java:
##########
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.polaris.service.metrics;
+package org.apache.polaris.core.metrics;

Review Comment:
   Thanks for the recap, @flyingImer !
   
   Minor correction: discussions during in-person meetings cannot be "decisions" from the ASF practice POV. However, since the proposal for `polaris-service-spi` is posted in this thread, we can assume lazy consensus if reviewers do not disagree :slightly_smiling_face: 



-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org