XComp commented on a change in pull request #13861:
URL: https://github.com/apache/flink/pull/13861#discussion_r515984885
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/AbstractMetricGroup.java
##########
@@ -116,15 +116,18 @@ public AbstractMetricGroup(MetricRegistry registry,
String[] scope, A parent) {
@Override
public Map<String, String> getAllVariables() {
- return internalGetAllVariables(0, Collections.emptySet());
+ return internalGetAllVariables(-1, Collections.emptySet());
Review comment:
```suggestion
return internalGetAllVariables(0, Collections.emptySet());
```
The PR. version causes an `ArrayIndexOutOfBoundsException` in
[internalGetAllVariables:136](https://github.com/apache/flink/pull/13861/files#diff-cafa0bde2d752076fb6e568c8360a90d598b857c387a7263e4162fa37c06ca72R136).
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/metrics/groups/AbstractMetricGroupTest.java
##########
@@ -93,6 +96,26 @@ public void testGetAllVariablesWithExclusions() {
assertEquals(group.getAllVariables(-1,
Collections.singleton(ScopeFormat.SCOPE_HOST)).size(), 0);
}
+ @Test
+ public void testGetAllVariablesWithExclusionsForReporters() {
+ MetricRegistry registry =
TestingMetricRegistry.builder().setNumberReporters(2).build();
+
+ AbstractMetricGroup<?> group = new GenericMetricGroup(registry,
null, "test") {
+ @Override
+ protected void putVariables(Map<String, String>
variables) {
+ variables.put("k1", "v1");
+ variables.put("k2", "v2");
+ }
+ };
+
+ group.getAllVariables(-1, Collections.emptySet());
+
+ assertThat(group.getAllVariables(0,
Collections.singleton("k1")), not(IsMapContaining.hasKey("k1")));
+ assertThat(group.getAllVariables(0,
Collections.singleton("k1")), (IsMapContaining.hasKey("k2")));
+ assertThat(group.getAllVariables(1,
Collections.singleton("k2")), (IsMapContaining.hasKey("k1")));
Review comment:
That's just a minor thing, but I would remove the brackets around
`IsMapContaining.hasKey(..)` to make the code more readable.
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/metrics/groups/AbstractMetricGroupTest.java
##########
@@ -43,9 +43,12 @@
import java.util.Arrays;
import java.util.Collections;
+import java.util.Map;
import java.util.concurrent.atomic.AtomicReference;
import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.collection.IsMapContaining.hasKey;
Review comment:
The static method import
`org.hamcrest.collection.IsMapContaining.hasKey` is never used. You're
referring to the `IsMapContaining` class in the test code.
----------------------------------------------------------------
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:
[email protected]
