Croway commented on PR #29332: URL: https://github.com/apache/camel/pull/29332#issuecomment-6686980096
@davsclaus thanks for the review. Points 1, 3 and 4 are addressed in this PR; points 2 and 5 are noted as follow-ups, not in this PR. 1. **No request without ack ids** (db5fbbc1aee2): when neither the exchange nor its grouped exchanges have the ack id header, `AcknowledgeSync.ack`/`nack` no longer build a stub or send a request. They log a WARN that names the exchange and says that no message is acked (or nacked), so the messages are delivered again after their ack deadline. They do not throw, because they may run in an on completion callback. Two new unit tests check this with the mocked stub: no request, no stub created, one WARN that names the exchange id. On the previous head (2e494100bdb4) both fail because an empty request is sent; with the change they pass. The doc sentence about a removed header is updated to match. 2. **Large acks**: noted as a follow-up. The Java client library (google-cloud-pubsub 1.158.0, the version Camel uses) splits acks and modacks at 1,000 ids per request (`MAX_PER_REQUEST_CHANGES = 1000` in `StreamingSubscriberConnection`), not 2,500, so a follow-up would use 1,000. 3. **Same subscription** (1e6e82321670): the "Manual Acknowledgement" section now says that the request goes to the subscription of the `GooglePubsubAcknowledge` used, so every grouped exchange must come from that same subscription, that is from the same consumer endpoint. The catalog copy is refreshed. 4. **Test nit** (8f96c54ca4d4): removed the `pending.cancel(true)` before `context.stop()`. With a pull counter added locally, the consumer pulled 9 and 4 times with the cancel (spinning on the cancelled future), and exactly 2 times without it (the first pull and the pending one, which the stop cancels). The test passes and does not hang. 5. **`AcknowledgeAsync` acking the grouped exchanges**: noted as a follow-up too. It changes behaviour for the asynchronous consumer, and that consumer's default flow control (1,000 outstanding messages) limits how large an aggregation can get, so it deserves its own change. Module: `mvn verify` in `camel-google-pubsub` passes (46 unit tests, 18 emulator ITs). _Claude Code on behalf of Federico Mariani_ -- 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]
