BewareMyPower commented on PR #22279: URL: https://github.com/apache/pulsar/pull/22279#issuecomment-2001921237
@lhotari This PR is merged too quickly without a 2nd review. After revisiting the code, I found the double-checked locking is unnecessary. `scheduleReadOnActiveConsumer()` is always called in `pickAndScheduleActiveConsumer()`, which is always called in `addConsumer()` and `removeConsumer()`. However, both of them are synchronized methods. So `scheduleReadOnActiveConsumer` could never be called concurrently. The original motivation is the IDE warning: <img width="853" alt="image" src="https://github.com/apache/pulsar/assets/18204803/b0db0c41-87ba-4a4c-bc5a-315e6acc924e";> It's true that it's a non-atomic operation on volatile field 'readOnActiveConsumerTask'. It's a commonly seen error to access a volatile field so IDE gives such warnings. However, we don't need to make `scheduleReadOnActiveConsumer()` thread safe. Unfortunately, there is no comment that notes this point. The lack of such comments makes code hard to maintain. Generally, developers assume all methods must be thread safe so the code might become unnecessarily complicated. I opened another PR (https://github.com/apache/pulsar/pull/22285) that improves the code quality and reduces the unnecessary atomicity. That PR also reverts the changes of this PR. PTAL again. -- 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]
