dimas-b commented on code in PR #3468:
URL: https://github.com/apache/polaris/pull/3468#discussion_r2714009795
##########
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:
Per our standing [evolution
guidelines](https://polaris.apache.org/in-dev/unreleased/evolution/) `public`
classes / methods can change in any release. Unlike REST API changes, it is not
considered a "major" change for versioning purposes. Essentially java methods
are not part of the API surface in the [SemVer](https://semver.org/) sense.
We should and do try to make java API changes in a backward compatible
manner when practical.
Regarding this particular PR and java interfaces that are part of an SPI
(defined and called by Polaris, implemented by 3rd party plugins), I do not see
a practical way to evolve then in a backward-compatible matter without causing
excessive maintenance burden in Polaris code.
In this case, Polaris would have to define a _new_ interface for the new
method signature and perform `instanceof` checks at runtime in order to decide
whether to call the old or the new method. I do believe it would be an overkill
at the current stage of the project (still evolving actively). It is not too
hard for downstream implementations to adjust code for the newly added
parameter.
That said, I'd like to propose moving the SPI evolution discussion to the
`dev` ML as it is a big and complex topic.
@singhpk234 : please clarify whether you consider this comment thread a
blocker for merging (or not).
--
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]
