[GitHub] [kafka] hachikuji commented on a change in pull request #11131: KAFKA-13137: KRaft Controller Metric MBean names incorrectly quoted

[GitBox]

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: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org