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


##########
runtime/service/src/main/java/org/apache/polaris/service/reporting/PolarisMetricsReporter.java:
##########
@@ -18,9 +18,39 @@
  */
 package org.apache.polaris.service.reporting;
 
+import java.time.Instant;
 import org.apache.iceberg.catalog.TableIdentifier;
 import org.apache.iceberg.metrics.MetricsReport;
 
+/**
+ * SPI interface for reporting Iceberg metrics received by Polaris.
+ *
+ * <p>Implementations can be used to send metrics to external systems for 
analysis and monitoring.
+ * Custom implementations can be annotated with appropriate {@code Quarkus} 
scope and {@link
+ * io.smallrye.common.annotation.Identifier @Identifier("my-reporter-type")} 
for CDI discovery.
+ *
+ * <p>The implementation to use is selected via the {@code 
polaris.iceberg-metrics.reporting.type}
+ * configuration property, which defaults to {@code "default"}.
+ *
+ * <p>Implementations can inject other CDI beans for context.
+ *
+ * @see DefaultMetricsReporter
+ * @see MetricsReportingConfiguration
+ */
 public interface PolarisMetricsReporter {
-  public void reportMetric(String catalogName, TableIdentifier table, 
MetricsReport metricsReport);

Review Comment:
   > can we not add the api in the same interface and add the default impl of 
old api call this null clock [...]
   
   I do not see a point in adding a `default` impl for the _old_ method alone. 
Existing implementations will have overrides for it already (javac will not let 
them miss that).
   
   We could add a `default` impl. for the _new_ method and redirect to the old 
one (without the `Instant`). This will allow existing implementations to 
compile without changes. However, this creates uncertainty for the implementor 
regarding which method should do the real work. Javadoc helps, but adds 
cognitive load. We could add `default` to both methods and deprecate the old 
one, but this will add "cruft" to the interface definition, when, I believe, 
adapting to the new interface in downstream projects is very easy.
   
   Please note that providing a custom implementation for this interface 
requires a downstream build. So all upgrades will go through local builds and 
CI (assuming normal software engineering practices), where the need for 
adjustments will be become apparent.



-- 
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]

Reply via email to