f1amingo commented on PR #1384: URL: https://github.com/apache/rocketmq-clients/pull/1384#issuecomment-5790124426
Thanks for adding the Node.js message interceptor framework. I found three issues that should be addressed before merging: 1. `Consumer.receiveMessage()` invokes `RECEIVE.doBefore` only after the RPC completes. This prevents `InflightRequestCountInterceptor` from tracking actual in-flight requests. On failure, it also creates a different context for `doAfter`. Please invoke `doBefore` before the RPC and reuse the same context for `doAfter`, consistent with the Java implementation. 2. PushConsumer settlement bypasses the new hooks. `ProcessQueue` calls `ackMessageViaRpc()` and `changeInvisibleDurationViaRpc()` directly, but these methods do not trigger `ACK` or `CHANGE_INVISIBLE_DURATION`. Therefore, these hooks only work for SimpleConsumer. Please wrap the shared RPC methods so both consumer types have consistent behavior, including retries. 3. `CompositedMessageInterceptor.doBefore()` creates a separate context for each interceptor, so later interceptors cannot see attributes added by earlier ones. Its `doAfter()` attribute merging may also overwrite an interceptor’s own attributes. Please preserve the accumulated context semantics used by the Java implementation. The current tests pass, but they do not cover these paths. Please add tests for RECEIVE timing/context reuse, PushConsumer ACK/change-invisible-duration hooks, and multi-interceptor attribute propagation. -- 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]
