You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "mst (via GitHub)" <gi...@apache.org> on 2025/01/24 15:40:27 UTC
[PR] Spark: don't override `LoggingMetricsReporter` [iceberg]
mst opened a new pull request, #12092:
URL: https://github.com/apache/iceberg/pull/12092
the current metrics reporter is overwritten if it's a `LoggingMetricsReporter`. If it's not `LoggingMetricsReporter`, metrics reporters are combined.
Imho, the only reason to do this is to prevent multiple `LoggingMetricsReporters`. However, `MetricsReporters::combine` takes care of this case already as `LoggingMetricsReporter` is a singleton and the `combine` method uses an IdentityHasMap.
--
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
Re: [PR] Core: don't override `LoggingMetricsReporter` [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on PR #12092:
URL: https://github.com/apache/iceberg/pull/12092#issuecomment-2621101631
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).
--
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
Re: [PR] Core: don't override `LoggingMetricsReporter` [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra closed pull request #12092: Core: don't override `LoggingMetricsReporter`
URL: https://github.com/apache/iceberg/pull/12092
--
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
Re: [PR] Core: don't override `LoggingMetricsReporter` [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
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-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
Re: [PR] Core: don't override `LoggingMetricsReporter` [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #12092:
URL: https://github.com/apache/iceberg/pull/12092#issuecomment-2621884278
> 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.
--
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