fapaul commented on a change in pull request #16971:
URL: https://github.com/apache/flink/pull/16971#discussion_r695558290
##########
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 am also not concerned about the performance but the readability and
maintainability. To me having dedicated synchronized blocks already implies
that the coming code section is somewhat critical and not thread-safe but
seeing it only doing a single operation feels strange.
Moreover, I think the current threading model is more complicated because
you must go through the complete class and have a look at all `synchronized`
blocks before you can reason whether it is solid or not. By moving this
responsibility to proper java builtins the contract is immediately clear when
seeing the variable declarations.
--
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]
