TakaHiR07 commented on PR #24978: URL: https://github.com/apache/pulsar/pull/24978#issuecomment-3527642648
> Fixes #24977 > > ### Motivation > As shown in the issue, fix two problem: 1. cleanCacheAndCloseReader() executed twice cause concurrent error, which result in too many orphan reader remain in SystemTopicBasedTopicPoliciesService 2. double update in policyCacheInitMap cause recursive update error > > ### Modifications > 1. no need to do cleanCacheAndCloseReader() when throw exception, since the exception would be catch in outside code. By the way, in previous pulsar-version 2.9.x, cleanCacheAndCloseReader is also executed only once > 2. avoid double update in policyCacheInitMap. use putIfAbsent instead of computeIfAbsent. It is not appropriate to add so many operation into compute(). > 3. add a test to simulate if reader fail in prepareInitPoliciesCacheAsync. By the way, it seems lack of unittest in SystemTopicBasedTopicPoliciesService. > 4. new method "newReader()" to ensure only one readerCreateCompletableFuture. Actually this method is add for test. The whole process of prepareInitPoliciesCacheAsync() is : put future -> put reader -> throw exception -> remove reader -> remove future. so even without "newReader()", namespace's reader can be ensure only one. > > There is one point should be consider in this pr > > 1. When use putIfAbsent, if too many getTopicPolicy() trigger prepareInitPoliciesCacheAsync, it would generate many empty completableFuture. Further more, we can use double check in the code to avoid this object gc.(the code would be longer). > > Besides, this case still exist: if failed to close reader in cleanCacheAndCloseReader(), this closing reader maybe have chance to reconnect and become orphan reader. This is not this pr's work. > > ### Verifying this change > * [] Make sure that the change passes the CI checks. > > ### Does this pull request potentially affect one of the following parts: > _If the box was checked, please highlight the changes_ > > * [ ] Dependencies (add or upgrade a dependency) > * [ ] The public API > * [ ] The schema > * [ ] The default values of configurations > * [ ] The threading model > * [ ] The binary protocol > * [ ] The REST endpoints > * [ ] The admin CLI options > * [ ] The metrics > * [ ] Anything that affects deployment > > ### Documentation > * [ ] `doc` > * [ ] `doc-required` > * [x] `doc-not-needed` > * [ ] `doc-complete` -- 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]
