fuyou001 commented on PR #10659: URL: https://github.com/apache/rocketmq/pull/10659#issuecomment-5099396425
**[P1] Preserve failed records in the `PopConsumerCache` call path** `revive(PopConsumerRecord)` is not only called by `revive(AtomicLong, int)`. When `enablePopBufferMerge` is enabled, `PopConsumerService` passes `this::revive` to `PopConsumerCache` as a `Consumer<PopConsumerRecord>`. In `cleanupRecords()`, `consumer.accept(record)` discards the returned `CompletableFuture<Boolean>`, and `clearStagedRecords()` then unconditionally removes the staged checkpoints. The new exception-to-`false` conversion works in the RocksDB batch path because `false` is consumed and a backoff record is added to `failureList`. In the cache path, however, that `false` result is never observed. An asynchronously failed revive can therefore be cleared from the cache without being persisted for retry, preventing the message from being revived. The outer `catch` in `PopConsumerCache.run()` only sees synchronous exceptions thrown before a future is returned; it cannot observe an exceptional future completion. Please either attach the exception-to-`false` handling only at the batch call site if `PopConsumerCache` is intentionally out of scope, or change the cache callback to return and handle the future, clearing a staged record only after success and retaining or persisting it with backoff on `false` or exception. Please also add a regression test with `enablePopBufferMerge=true` and an asynchronously failed revive, verifying that the checkpoint remains retryable. -- 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]
