[GitHub] [iceberg] nastra commented on a diff in pull request #6410: Configurable metrics reporter by catalog properties

[GitBox <gi...@apache.org>]

nastra commented on code in PR #6410:
URL: https://github.com/apache/iceberg/pull/6410#discussion_r1045745386


##########
api/src/main/java/org/apache/iceberg/metrics/MetricsReporter.java:
##########
@@ -18,10 +18,16 @@
  */
 package org.apache.iceberg.metrics;
 
+import java.util.Map;
+
 /** This interface defines the basic API for reporting metrics for operations to a Table. */
 @FunctionalInterface
 public interface MetricsReporter {
 
+  default void init(Map<String, String> properties) {

Review Comment:
   we typically use `initialize` throughout the project as a naming pattern. However, I'm not sure yet if we'd need a `initialize(..)` method on the `MetricsReporter`. Do you have a use case where the custom reporter would require this?



##########
core/src/main/java/org/apache/iceberg/BaseTable.java:
##########
@@ -63,6 +63,10 @@ public String name() {
     return name;
   }
 
+  public MetricsReporter reporter() {

Review Comment:
   I don't think we should expose this. We should rather test that `report()` of the custom reporter is actually being called



##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -198,6 +201,28 @@ public void testCreateTableTxnBuilder() throws Exception {
     }
   }
 
+  @Test
+  public void testLoadTableWithCustomMetricReporter() throws Exception {
+    Schema schema = getTestSchema();
+    TableIdentifier tableIdent = TableIdentifier.of(DB_NAME, "tbl");
+    String location = temp.newFolder("tbl").toString();
+
+    HiveCatalog catalog = new HiveCatalog();
+    catalog.initialize(
+        "hive", ImmutableMap.of("metrics-reporter-impl", TestMetricsReporter.class.getName()));
+    try {
+      Transaction txn =
+          catalog.buildTable(tableIdent, schema).withLocation(location).createTransaction();
+      txn.commitTransaction();
+      BaseTable baseTable = (BaseTable) catalog.loadTable(tableIdent);
+      Assertions.assertInstanceOf(TestMetricsReporter.class, baseTable.reporter());

Review Comment:
   rather than exposing `reporter()` in `BaseTable`, I think it would be better to actually make sure that the custom reporter's `report()` method is being called



##########
core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java:
##########
@@ -51,7 +53,13 @@ public Table loadTable(TableIdentifier identifier) {
         }
 
       } else {
-        result = new BaseTable(ops, fullTableName(name(), identifier));
+        MetricsReporter metricsReporter =
+            properties().containsKey(CatalogProperties.METRICS_REPORTER_IMPL)
+                ? CatalogUtil.loadMetricsReporter(
+                    properties().get(CatalogProperties.METRICS_REPORTER_IMPL))
+                : LoggingMetricsReporter.instance();
+        metricsReporter.init(properties());

Review Comment:
   calling `initialize` should be part of `CatalogUtil.loadMetricsReporter`. You can then just add the properties as a parameter to that method



-- 
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@iceberg.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org