wang-jiahua commented on PR #10659: URL: https://github.com/apache/rocketmq/pull/10659#issuecomment-5101564078
> **[P1] Preserve failed records in the `PopConsumerCache` call path** @fuyou001 Thanks for the careful review — you are right. Addressed by taking the first option in commit b84fcc0a3: the exception-to-`false` conversion is now attached only at the batch call site. - `revive(PopConsumerRecord)` no longer carries an `.exceptionally` handler, so its exception semantics are exactly what they were before this PR; the `PopConsumerCache` callback path is untouched. - In `revive(AtomicLong, int)` the handler sits right where the `false` result is consumed into the failureList backoff retry, with a comment noting that other callers do not consume the result. - This also removes a side problem: the "will retry" log would have been misleading on the cache path, where a cleared record is never retried. On the pre-existing cache-path behavior, I verified: an asynchronously failed revive was already lost before this PR — the exceptionally-completed future is unobserved, the revive-attempted records are not in `writeConsumerRecords`, and `clearStagedRecords()` clears the staging map unconditionally; the `run()` catch only sees synchronous throws, which abort `cleanupRecords` before the clear and get retried on the next sweep. Since the cache path is intentionally out of scope here, I've opened #10667 to track clearing a staged record only after a successful revive, including the `enablePopBufferMerge=true` regression test you suggested. Both regression tests still pass (`PopConsumerServiceTest` 19/19). -- 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]
