[GitHub] [iceberg] rdblue commented on a diff in pull request #7337: Core: Introduce CompositeMetricsReporter

["rdblue (via GitHub)" <gi...@apache.org>]

rdblue commented on code in PR #7337:
URL: https://github.com/apache/iceberg/pull/7337#discussion_r1170602203


##########
core/src/main/java/org/apache/iceberg/TableScanContext.java:
##########
@@ -353,17 +352,29 @@ TableScanContext planWith(ExecutorService executor) {
         toSnapshotId,
         executor,
         fromSnapshotInclusive,
-        metricsReporters);
+        metricsReporter);
   }
 
-  Collection<MetricsReporter> metricsReporters() {
-    return metricsReporters;
+  MetricsReporter metricsReporter() {
+    return metricsReporter;
   }
 
   TableScanContext reportWith(MetricsReporter reporter) {
-    ImmutableList.Builder<MetricsReporter> builder = ImmutableList.builder();
-    builder.addAll(metricsReporters);
-    builder.add(reporter);
+    CompositeMetricsReporter compositeReporter;

Review Comment:
   Why does this always create a composite reporter? That seems strange.
   
   I would expect the metrics reporter to initially be null. When this is called the first time, a context with a non-null reporter is created. When it is subsequently called, a composite reporter is used. If one is already in the context, then a new one is created with all of the old reporters plus the new reporter.
   
   We also need to think about deduplication. What happens if the logging reporter is added by two separate code paths? How does the composite reporter detect this?



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