[GitHub] [iceberg] gaborkaszab commented on a diff in pull request #6404: Core: Allow configuring metrics reporter impl via Catalog property

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

gaborkaszab commented on code in PR #6404:
URL: https://github.com/apache/iceberg/pull/6404#discussion_r1045678055


##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -304,7 +306,9 @@ public Table loadTable(SessionContext context, TableIdentifier identifier) {
         new BaseTable(
             ops,
             fullTableName(loadedIdent),
-            report -> reportMetrics(tableIdentifier, report, session::headers));

Review Comment:
   Just distantly related and only for my own information: With the current implementation MetricsReporter could be configured when we start the Catalog and then all the tables within this catalog would use the same MetricsReporter. I'm wondering if it would make sense to make this setting more flexible, like being able to set this even after the Catalog is up and running, and additionally have the ability to use different MetricsReporter implementation for different tables.
   I don't have any specific use-cases in mind, one could be when let's say querying a table becomes pretty slow at some point and we want to switch to another MetricsReporter that could help us analysing the scan metrics to spot the issue, and then switch back to LoggingMetricsReporter once the issue is solved. (Assuming that in the future we gonna have way more scan metrics that could be used for debugging such issues)
   
   This is not meant to be covered in this PR just thinking out loud if it makes sense.



##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -177,7 +177,9 @@ public void initialize(String name, Map<String, String> unresolved) {
     this.io =
         CatalogUtil.loadFileIO(
             ioImpl != null ? ioImpl : ResolvingFileIO.class.getName(), mergedProps, conf);
-    this.reporter = new LoggingMetricsReporter();
+    String metricsReporterImpl = mergedProps.get(CatalogProperties.METRICS_REPORTER_IMPL);

Review Comment:
   Would it make sense to add a test where you configure a custom MetricsReporter through this new catalog property?



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