TakaHiR07 commented on PR #4467: URL: https://github.com/apache/bookkeeper/pull/4467#issuecomment-2244216823
> First of all, this is a great catch and definitely looks like a regression. > > There are couple of things that require additional attention: > > First, the fix seems to be on the the right track but I think it should be done in `BookieClientImpl completeAdd(..)` . > > https://github.com/apache/bookkeeper/blob/4ca020a5f0805a748a1e8d983015e5221a534fd7/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClientImpl.java#L281 > > Second, the fix reverts part of the change #3784 (and follow up fix #3806 ) I'd love to have @merlimat look at this issue - is there a better fix or should we simply revert these perf improvements (that don't have any perf numbers shared on what they improve) for the sake of correctness? > > Third, it would be nice to have a test reproing the problem to prevent future regression. 1. That's right. we should do in `BookieClientImpl completeAdd(..)`, otherwise it would cause serious issue. I have updated this pr. 2. I also think about whether we should revert the whole change of #3784. There is a concern of not revert. If it exist other place not obey the specific thread rule, maybe have other dead lock issue. If we revert the whole change, maybe affect some other pr's change after branch-4.16.0 3. Now I prefer to test this pr in our test cluster. If more views agree with this pr's modification, I would try to add unittest to repoving the problem. -- 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]
