xkrogen commented on a change in pull request #34074:
URL: https://github.com/apache/spark/pull/34074#discussion_r779095395
##########
File path:
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java
##########
@@ -1410,4 +1442,42 @@ long getPos() {
return pos;
}
}
+
+ /**
+ * A class that wraps all the push-based metrics.
+ */
+ static class PushMergeMetrics implements MetricSet {
+ static final String NO_OPPORTUNITY_RESPONSES_METRIC =
"couldNotFindOpportunityResponses";
+ static final String TOO_LATE_RESPONSES_METRIC = "tooLateResponses";
+ static final String PUSHED_BYTES_WRITTEN_METRIC = "pushedBytesWritten";
+ static final String CACHED_BLOCKS_BYTES_METRIC = "cachedBlocksBytes";
+
+ private final Map<String, Metric> allMetrics;
+ private final Meter noOpportunityResponses;
+ private final Meter tooLateResponses;
+ private final Meter pushedBytesWritten;
+ private final Counter cachedBlockBytes;
+
+ private PushMergeMetrics() {
+ allMetrics = new HashMap<>();
+ noOpportunityResponses = new Meter();
+ allMetrics.put(NO_OPPORTUNITY_RESPONSES_METRIC, noOpportunityResponses);
+ tooLateResponses = new Meter();
+ allMetrics.put(TOO_LATE_RESPONSES_METRIC, tooLateResponses);
+ pushedBytesWritten = new Meter();
+ allMetrics.put(PUSHED_BYTES_WRITTEN_METRIC, pushedBytesWritten);
+ cachedBlockBytes = new Counter();
+ allMetrics.put(CACHED_BLOCKS_BYTES_METRIC, cachedBlockBytes);
+ }
+
+ @Override
+ public Map<String, Metric> getMetrics() {
+ return allMetrics;
+ }
+
+ @VisibleForTesting
+ Counter getCachedBlockBytes() {
+ return cachedBlockBytes;
+ }
Review comment:
Looks like this is used within `RemoteBlockPushResolverSuite`, but not
consistently. In `verifyMetrics()`, we just use `(Counter)
getMetrics.get(CACHED_BLOCK_BYTES_METRIC)`. Can we just do the same for the
other places where this metric is accessed, instead of adding this additional
method?
e.g.
```
assertEquals("cached bytes", 6L,
((PushMergeMetrics)pushResolver.getMetrics()).getCachedBlockBytes().getCount());
```
becomes
```
assertEquals("cached bytes", 6L,
((Counter)
pushResolver.getMetrics().get(CACHED_BLOCK_BYTES_METRIC)).getCount());
```
You can alternatively even create a helper method used from
`verifyMetrics()` and the other places:
```
private void assertCachedBlockBytesEquals(Map<String, String> actualMetrics,
long expected) {
assertEquals("cached bytes", expected, ((Counter)
actualMetrics.get(CACHED_BLOCK_BYTES_METRIC)).getCount());
}
```
This might be overkill, not sure.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
