nastra commented on PR #12092:
URL: https://github.com/apache/iceberg/pull/12092#issuecomment-2621887691

   > Hi @mst , I was looking at this code myself recently too. Initially I also 
thought that we shouldn't swap LoggingMetricsReporter here and we should 
combine it with the one received as a param. However, in that case there is no 
way to not use LoggingMetricsReporter in case you, for some reason, don't want 
logging for the metrics. So I found that the code as it is now kind of makes 
sense in a way. In case you want to provide a MetricsReporter but also want to 
keep the default LoggingMetricsReporter, you can still provide a combined 
reporter to this function.
   > 
   > [This is what I did in my 
experiment](https://github.com/gaborkaszab/impala/commit/ab165651d38aa50b2797ea4b4dcd9f2e1f748516#diff-2845130519fa452e24b37fe26617ebe4c32e8d0df22533d4e3395a4e5880fc08R639).
   > 
   > I'm curios what @nastra think about this.
   
   Thanks for the summary @gaborkaszab, that is in fact exactly the intention 
here. For example, Trino uses a noop reporter for testing and doesn't want 
things to be logged. So I don't think we should be changing anything here.
   Thanks for looking into this @mst but I'll go ahead and close this PR


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

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


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

Reply via email to