jerrypeng edited a comment on pull request #7010: URL: https://github.com/apache/pulsar/pull/7010#issuecomment-632331138
@srkukarni > Some methods are updating multiple counters/gauages. That still has to be in sync block to ensure that we get consistent stats. Given a specific point of time, we cannot guarantee that all stats retrieved will be consistent regardless of whether we synchronize the methods or not. Until less we update all stats at the end of each function invocation in an atomic fashion, there can also be slight deviations. And that is not accounting user stats that can be updated at any time and out of our control. Adding synchronization on all methods may add overhead and contention that is not necessary. I don't know of any system that attempts to update all metrics in an atomic fashion. > The fact that today this class is using an already thread safe data structure is merely detail. If for whatever reason we switch it to some other impl, the methods should still be thread safe. Thus don;t you think its better to declare the methods synchronized explicitly? Why would we consider using another implementation? Are there any deficiencies in the current solution? The stats manager is not even an interface in which the user specify the implementation. Since there is not contract with developers, we can change the implementation of how stats are handle whenever we feel like the implementation is not good enough. At that time we can synchronize methods if necessary. I don't think there is a point to pre-empt something like this given we don't even know what a new stats implementation would even look like. ---------------------------------------------------------------- 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
