qianye1001 commented on PR #10519: URL: https://github.com/apache/rocketmq/pull/10519#issuecomment-4828618586
@fuyou001 Thanks for the detailed analysis. I considered this one and I do not plan to address it in this batch-change PR. The reason is that this is not a new correctness model introduced by the batch path. The existing single `changeInvisibilityDuration` path already follows the same pattern: write the new CK, try to remove the old CK from the pop buffer, and then delete the old CK from RocksDB when needed. A concurrent cleanup can theoretically race with that path as well, so fixing this properly would be a broader PopConsumerCache ownership/cleanup change, not a batch-change-specific fix. Making the ownership transfer atomic would likely require coordinating renewal/change and cleanup with an additional lock or explicit ownership state transition on the hot path. For the batch path that cost is relatively high, especially because the observed impact is a low-probability duplicate delivery scenario and POP consumption already has at-least-once semantics. I would prefer not to add that synchronization cost in this PR. So for this PR I am keeping the batch behavior aligned with the existing single-change semantics and limiting the changes to reducing broker round trips and store writes/deletes. If we want to harden this race, I think it should be handled separately for both single and batch paths with a dedicated design and benchmark. Comment addressed: https://github.com/apache/rocketmq/pull/10519#issuecomment-4741428792 -- 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]
