AnonHxy commented on PR #15825: URL: https://github.com/apache/pulsar/pull/15825#issuecomment-1152150385
> 1. Why not initialize replicator like initializeDispatchRateLimiterIfNeeded ? If we do that, when broker updating replicator limiter dynamicly, it need call `PersistentReplicator#initializeDispatchRateLimiterIfNeeded` before line2420 to initialize the limiter. But this method is not `synchronized`. https://github.com/apache/pulsar/blob/3525fe478e123734f04864811089411b4a3fdeb4/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L2412-L2421 However if we `synchronized` this method, updating or initializing replicator limiter will both hold `dispatchRateLimiterLock` and `PersistentReplicatorLock`, see line:380 and line 388. So I think it is better to abstract `Replicator#updateRateLimiter()`, which will benefits for broker-level 、namespace-level and topic-level. https://github.com/apache/pulsar/blob/3525fe478e123734f04864811089411b4a3fdeb4/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java#L379-L391 > 2. Even if we can do like this patch, no need to change the return type of initializeDispatchRateLimiterIfNeeded Make sense, I have update to fix this. Thanks very much @Technoboy- -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
