Re: [PR] KAFKA-15870: Move new group coordinator metrics from Yammer to Metrics [kafka]

[via GitHub]

dajac commented on code in PR #14848:
URL: https://github.com/apache/kafka/pull/14848#discussion_r1425608080


##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/metrics/GroupCoordinatorMetrics.java:
##########
@@ -174,75 +151,91 @@ public GroupCoordinatorMetrics(MetricsRegistry registry, 
Metrics metrics) {
         ));
     }
 
-    public Long numOffsets() {
-        return shards.values().stream().mapToLong(shard -> 
shard.localGaugeValue(NUM_OFFSETS)).sum();
+    private Long numOffsets() {
+        return 
shards.values().stream().mapToLong(GroupCoordinatorMetricsShard::numOffsets).sum();
     }
 
-    public Long numGenericGroups() {
-        return shards.values().stream().mapToLong(shard -> 
shard.localGaugeValue(NUM_GENERIC_GROUPS)).sum();
+    private Long numGenericGroups() {
+        return shards.values().stream().mapToLong(shard -> 
shard.numGenericGroups(null)).sum();
     }
 
-    public Long numGenericGroupsPreparingRebalanceCount() {
-        return NUM_GENERIC_GROUPS_PREPARING_REBALANCE_COUNTER.get();
+    private Long numGenericGroupsPreparingRebalance() {
+        return shards.values().stream().mapToLong(shard -> 
shard.numGenericGroups(GenericGroupState.PREPARING_REBALANCE)).sum();
     }
 
-    public Long numGenericGroupsCompletingRebalanceCount() {
-        return NUM_GENERIC_GROUPS_COMPLETING_REBALANCE_COUNTER.get();
+    private Long numGenericGroupsCompletingRebalance() {
+        return shards.values().stream().mapToLong(shard -> 
shard.numGenericGroups(GenericGroupState.COMPLETING_REBALANCE)).sum();
     }
-    public Long numGenericGroupsStableCount() {
-        return NUM_GENERIC_GROUPS_STABLE_COUNTER.get();
+    private Long numGenericGroupsStable() {
+        return shards.values().stream().mapToLong(shard -> 
shard.numGenericGroups(GenericGroupState.STABLE)).sum();
     }
 
-    public Long numGenericGroupsDeadCount() {
-        return NUM_GENERIC_GROUPS_DEAD_COUNTER.get();
+    private Long numGenericGroupsDead() {
+        return shards.values().stream().mapToLong(shard -> 
shard.numGenericGroups(GenericGroupState.DEAD)).sum();
     }
 
-    public Long numGenericGroupsEmptyCount() {
-        return NUM_GENERIC_GROUPS_EMPTY_COUNTER.get();
+    private Long numGenericGroupsEmpty() {
+        return shards.values().stream().mapToLong(shard -> 
shard.numGenericGroups(GenericGroupState.EMPTY)).sum();
     }
 
-    public long numConsumerGroups() {
-        return shards.values().stream().mapToLong(shard -> 
shard.localGaugeValue(NUM_CONSUMER_GROUPS)).sum();
+    private long numConsumerGroups() {
+        return shards.values().stream().mapToLong(shard -> 
shard.numConsumerGroups(null)).sum();
     }
 
-    public long numConsumerGroupsEmpty() {
-        return shards.values().stream().mapToLong(shard -> 
shard.localGaugeValue(NUM_CONSUMER_GROUPS_EMPTY)).sum();
+    private long numConsumerGroupsEmpty() {
+        return shards.values().stream().mapToLong(shard -> 
shard.numConsumerGroups(ConsumerGroup.ConsumerGroupState.EMPTY)).sum();
     }
 
-    public long numConsumerGroupsAssigning() {
-        return shards.values().stream().mapToLong(shard -> 
shard.localGaugeValue(NUM_CONSUMER_GROUPS_ASSIGNING)).sum();
+    private long numConsumerGroupsAssigning() {
+        return shards.values().stream().mapToLong(shard -> 
shard.numConsumerGroups(ConsumerGroup.ConsumerGroupState.ASSIGNING)).sum();
     }
 
-    public long numConsumerGroupsReconciling() {
-        return shards.values().stream().mapToLong(shard -> 
shard.localGaugeValue(NUM_CONSUMER_GROUPS_RECONCILING)).sum();
+    private long numConsumerGroupsReconciling() {
+        return shards.values().stream().mapToLong(shard -> 
shard.numConsumerGroups(ConsumerGroup.ConsumerGroupState.RECONCILING)).sum();
     }
 
-    public long numConsumerGroupsStable() {
-        return shards.values().stream().mapToLong(shard -> 
shard.localGaugeValue(NUM_CONSUMER_GROUPS_STABLE)).sum();
+    private long numConsumerGroupsStable() {
+        return shards.values().stream().mapToLong(shard -> 
shard.numConsumerGroups(ConsumerGroup.ConsumerGroupState.STABLE)).sum();
     }
 
-    public long numConsumerGroupsDead() {
-        return shards.values().stream().mapToLong(shard -> 
shard.localGaugeValue(NUM_CONSUMER_GROUPS_DEAD)).sum();
+    private long numConsumerGroupsDead() {
+        return shards.values().stream().mapToLong(shard -> 
shard.numConsumerGroups(ConsumerGroup.ConsumerGroupState.DEAD)).sum();
     }
 
     @Override
     public void close() {
         Arrays.asList(
             NUM_OFFSETS,
-            NUM_GENERIC_GROUPS,
             NUM_GENERIC_GROUPS_PREPARING_REBALANCE,
             NUM_GENERIC_GROUPS_COMPLETING_REBALANCE,
             NUM_GENERIC_GROUPS_STABLE,
             NUM_GENERIC_GROUPS_DEAD,
-            NUM_GENERIC_GROUPS_EMPTY,
-            NUM_CONSUMER_GROUPS,
-            NUM_CONSUMER_GROUPS_EMPTY,
-            NUM_CONSUMER_GROUPS_ASSIGNING,
-            NUM_CONSUMER_GROUPS_RECONCILING,
-            NUM_CONSUMER_GROUPS_STABLE,
-            NUM_CONSUMER_GROUPS_DEAD
+            NUM_GENERIC_GROUPS_EMPTY
         ).forEach(registry::removeMetric);
 
+        
metrics.removeMetric(metrics.metricName(CONSUMER_GROUPS_COUNT_METRIC_NAME, 
METRICS_GROUP));

Review Comment:
   Have you considered keeping a reference to the metric names instead of 
re-creating names here? This may simplify the logic to close them.



-- 
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