AHeise commented on a change in pull request #16971:
URL: https://github.com/apache/flink/pull/16971#discussion_r695551636
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/testutils/InMemoryReporter.java
##########
@@ -159,17 +168,51 @@ void applyRemovals() {
@Override
public void notifyOfAddedMetric(Metric metric, String metricName,
MetricGroup group) {
- MetricGroup metricGroup = unwrap(group);
- LOG.debug("Registered {} @ {}", metricName, metricGroup);
- synchronized (this) {
- metrics.computeIfAbsent(metricGroup, dummy -> new
HashMap<>()).put(metricName, metric);
+ if (captureMode != CaptureMode.NONE) {
+ MetricGroup metricGroup = unwrap(group);
+ LOG.debug("Registered {} @ {}", metricName, metricGroup);
+ synchronized (this) {
+ metrics.computeIfAbsent(metricGroup, dummy -> new HashMap<>())
Review comment:
I'm always having a hard time to reason about nested data structures and
their synchronization behavior. For example, could a metric be lost because we
eagerly deregister the last metric at the same time a new metric is added? Then
`notifyOfRemovedMetric` might fetch the group first and determines it is empty.
At the same time, `notifyOfAddedMetric` has fetched the still-registered but
empty group and added the new metric. But this is now lost since
`notifyOfRemovedMetric` eventually removes it.
So before trying to optimize it, I first would go with the simplest
approach. If we see it becomes a bottle-neck (which I doubt) we can still
optimize it later. Then I might have the mental capacity for thinking all cases
through (or we find someone else).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
