qianye1001 commented on PR #1249: URL: https://github.com/apache/rocketmq-clients/pull/1249#issuecomment-4517244243
## Review Suggestions 1. **Per-group buffering for FIFO mode** — The current batch buffer is global across all message groups. In FIFO mode, a batch may contain messages from different groups, and a failure sends all to DLQ together. Consider whether per-group buffering would provide stricter FIFO guarantees. 2. **`directExecutor()` in `submitBatch()` callback** — `handleResult()` runs on the same thread that completes the future. If ack/nack does blocking I/O, it could delay other callbacks. Consider using the consumption executor instead of `MoreExecutors.directExecutor()`. 3. **Add `FifoBatchConsumeServiceTest`** — `BatchConsumeService`, `BatchConsumeTask`, and `BatchPolicy` all have tests, but `FifoBatchConsumeService` lacks dedicated coverage for FIFO ordering, retry, and DLQ scenarios. 4. **Constructor backward compat** — The old 7-param convenience constructor in `PushConsumerImpl` was removed. Consider keeping it as a deprecated overload, or call this out in release notes. -- 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]
