[GitHub] [pulsar] bharanic-dev commented on pull request #10384: [Broker] Fix set-publish-rate when using preciseTopicPublishRateLimiterEnable=true

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

bharanic-dev commented on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-891977137


   > > * at the time of configuration change, if the connection was in a state where the 'read is disabled' (because of a previous rate limit), should the RateLimiter call autoReadResetFunction() in it's 'close' method? Otherwise, would the connection deadlock and forever remain in 'read disable' state?
   > 
   > @bharanic-dev Thanks for the feedback. I have added the call to the close method.
   > 
   > > * for PIP-82, one rate limiter will be shared by multiple topics. So, when a topic is unloaded, the ratelimiter should not be closed unless it was the last topic that was using it. So, instead of making 'PublishRateLimiter' 'Autoclosable', can you please consider adding a 'detach' method that can be called when the topic is getting unloaded?
   > 
   > @bharanic-dev I think it's better to handle this with composition instead of making a single class handle different use cases. With composition it becomes easy. You can have a wrapper that takes a PublishRateLimiter and handles the life cycle with reference counting. One possible solution is to use the org.apache.bookkeeper.mledger.util.AbstractCASReferenceCounted class as a base class for this kind of wrapper.
   
   Thank you for taking care of this. LGTM.


-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org