hachikuji commented on a change in pull request #11131:
URL: https://github.com/apache/kafka/pull/11131#discussion_r678716922
##########
File path:
metadata/src/main/java/org/apache/kafka/controller/ControllerMetrics.java
##########
@@ -42,4 +42,6 @@
void setPreferredReplicaImbalanceCount(int replicaImbalances);
int preferredReplicaImbalanceCount();
+
+ void close();
Review comment:
nit: shall we extend `AutoCloseable`?
##########
File path:
metadata/src/test/java/org/apache/kafka/controller/QuorumControllerMetricsTest.java
##########
@@ -45,26 +45,36 @@ public void testControllerEventManagerMetricNames() {
Set<String> expectedMetricNames = Utils.mkSet(
"EventQueueTimeMs",
"EventQueueProcessingTimeMs");
- assertExpectedMetrics(expectedMetricNames, expectedType);
+ assertExpectedMetricsCreatedAndRemovedUponClose(expectedMetricNames,
expectedType);
}
- private static void assertExpectedMetrics(Set<String> expectedMetricNames,
String expectedType) {
+ private static void
assertExpectedMetricsCreatedAndRemovedUponClose(Set<String>
expectedMetricNames, String expectedType) {
String expectedGroup = "kafka.controller";
MetricsRegistry registry = new MetricsRegistry();
- new QuorumControllerMetrics(registry); // populates the registry
- expectedMetricNames.stream().forEach(expectedMetricName -> assertTrue(
- registry.allMetrics().keySet().stream().anyMatch(metricName -> {
- if (metricName.getGroup().equals(expectedGroup) &&
metricName.getType().equals(expectedType)
- && metricName.getScope() == null &&
metricName.getName().equals(expectedMetricName)) {
- // It has to exist AND the MBean name has to be correct;
- // fail right here if the MBean name doesn't match
- String expectedMBeanPrefix = expectedGroup + ":type=" +
expectedType + ",name=";
- assertEquals(expectedMBeanPrefix + expectedMetricName,
metricName.getMBeanName(),
- "Incorrect MBean name");
- return true; // the expected metric name exists and the
associated MBean name matches
- } else {
- return false; // this one didn't match
- }
- }), "Missing metric: " + expectedMetricName));
+ QuorumControllerMetrics quorumControllerMetrics = new
QuorumControllerMetrics(registry); // populates the registry
+ Arrays.asList(true, false).forEach(checkMetricsExist -> {
Review comment:
This seems overly complicated. An easier structure to follow would be
something like this:
```java
String expectedType = "KafkaController";
Set<String> expectedMetricNames = Utils.mkSet(
"ActiveControllerCount",
"GlobalTopicCount",
"GlobalPartitionCount",
"OfflinePartitionsCount",
"PreferredReplicaImbalanceCount"
);
MetricsRegistry registry = new MetricsRegistry();
try (QuorumControllerMetrics quorumControllerMetrics = new
QuorumControllerMetrics(registry)) {
assertMetricsCreated(registry, expectedMetricNames);
}
assertMetricsRemoved(registry, expectedMetricNames);
```
--
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]
