[GitHub] [pulsar] jerrypeng commented on pull request #7010: Fix null pointer when getting function instance metrics.

[GitBox <gi...@apache.org>]

jerrypeng commented 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. I don't think it worthwhile to consider "what if" situations at this moment.
   


----------------------------------------------------------------
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:
users@infra.apache.org