hudeqi commented on code in PR #13471:
URL: https://github.com/apache/kafka/pull/13471#discussion_r1152485189
##########
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala:
##########
@@ -4485,6 +4485,25 @@ class ReplicaManagerTest {
assertTrue(response.usableBytes >= 0)
}
}
+
+ @Test
+ def checkRemoveMetricsCountMatchRegisterCount(): Unit = {
Review Comment:
> I think that we can avoid changing the `KafkaMetricsGroup` completely here
and that is a preferred approach in my opinion because:
>
> 1. Methods in `KafkaMetricsGroup` may not be called from same thread? In
that case, then we will have to use AtomicInteger for storing `numAddCount`.
This will synchronize multiple threads on same lock hence, adding another point
of contention.
> 2. Do we anticipate counters inside `KafkaMetricsGroup` to be used for any
other purpose? If not, then we are adding counters only for writing unit test
which is avoidable.
> 3. We are adding a potential source of bugs where if a new method is added
is to `KafkaMetricsGroup`, the author will have to remember to correctly modify
the counter.
I see, thanks!!!
--
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
