szhem commented on a change in pull request #24556: [SPARK-27641][CORE] Fix
MetricsSystem to remove unregistered source correctly
URL: https://github.com/apache/spark/pull/24556#discussion_r282115385
##########
File path: core/src/main/scala/org/apache/spark/metrics/MetricsSystem.scala
##########
@@ -166,9 +166,11 @@ private[spark] class MetricsSystem private (
}
def removeSource(source: Source) {
- sources -= source
- val regName = buildRegistryName(source)
- registry.removeMatching((name: String, _: Metric) =>
name.startsWith(regName))
+ if (sources.contains(source)) {
+ sources -= source
+ val regName = buildRegistryName(source)
+ registry.removeMatching((name: String, _: Metric) =>
name.startsWith(regName))
Review comment:
The original JIRA is more generic than it seems to be.
`AccumulatorSource` there is just for example. Just consider any other
custom `Source` and not just `AccumulatorSource`.
There is a bug in the MetricsSystem that allows to add (supports) multiple
sources with the same name, but does not allow to remove a single source with
its metrics only. Removing a single source leads to removing all the registered
metrics from all the sources of the same name.
Removing metrics from this shared registry by exact match seems to be more
accurate than just by the corresponding prefix.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
