cmccabe commented on pull request #10572: URL: https://github.com/apache/kafka/pull/10572#issuecomment-823721422
Hi @dielhennr, thanks for the PR! I think it would be simpler to have `ControllerMetrics#setGlobalTopicsCount` / `ControllerMetrics#globalTopicsCount` rather than inc / dec / get. We can just look at the size of `ReplicationControlManager#topics` to get the relevant number in O(1) time. This also makes it harder for our count to get out of sync somehow. For the partition count, we have no choice but to maintain our own count, since we can't efficiently get that number just by taking the size() of any existing data structure. However, we should do this in `ReplicationControlManager` rather than in the metrics object. There are other reasons why we might want this count besides exposing metrics. For example, one reason is that we might not want to allow more than a certain number of partitions to be created. (There is a KIP proposing such a config, I believe.) So again for consistency we can just have `ControllerMetrics#setGlobalPartitionsCount` / `ControllerMetrics#globalPartitionsCount` Keep in mind that anything accessed by a metrics callback functions needs to be set in a thread-safe way. The metrics system uses a different set of threads than the controller. So you need to use a lock, a volatile, or an atomic variable when passing information from one to the other. A volatile is usually the best way to do things since it has the lowest overhead (certainly it's the best in this particular case, I think.) -- 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: us...@infra.apache.org
