oscerd commented on PR #27123: URL: https://github.com/apache/camel/pull/27123#issuecomment-5927175240
All three points were right, and the first two were corrections to my own claims rather than to the code. Addressed in `2ccc9d5`. **1. Guide wording — you are right, and it was wrong rather than just loose.** I checked `BridgeExceptionHandlerToErrorHandler` before rewording: it opens with `isFailedByErrorHandler(exchange)` and falls back to the logging handler for an exchange that already failed while being routed, with the comment *"the bridge is only for errors when the consumer picks up messages"*. And `RedeliveryErrorHandler` does call `logFailedDelivery` for an exhausted exchange, so "no log line at all" was false. Reworded to say what actually changes: the consumer no longer treats a failed exchange as delivered, and logs a warning of its own as `TimerConsumer` does — plus a sentence making clear this is not a change to error handling. The `processExchange` javadoc repeated the same overstatement and is corrected too. **2. Data loss with `delete` — filed as [CAMEL-25221](https://issues.apache.org/jira/browse/CAMEL-25221), and the misleading comment is gone.** You are right that "never handed to the route" papered over it. `removeDocument` runs inside the poll loop, so anything the drain loop releases was already deleted and is lost rather than undelivered. The comment now says exactly that and points at the Jira. I took the follow-up option rather than fixing it here, because moving the delete after processing changes *when* the document disappears and deserves its own upgrade-guide note. Worth recording that `isBatchAllowed()` makes it reachable in a default configuration — stopping a route mid-batch loses the remainder — not only via `maxMessagesPerPoll`, which this component does not even declare as an endpoint option. The Jira covers both routes to it and the failed-route case, and is linked to CAMEL-25024, where camel-mongodb only advances position on success. **3. Stale handles on restart — a real regression from my own fix, now fixed and tested.** Exactly as you describe: the old no-op `core().shutdown()` hid it, and making the disconnect real exposed it. The consumer now resolves `bucket`/`scope`/`collection` in `doStart` rather than `doInit`, and the producer resolves its `collection` again in `doStart`. `createClient()` is package-private for that, with a comment explaining why. Proven by `aRestartedProducerTakesItsCollectionFromTheNewConnection`: two mocked clusters, restart the endpoint and the producer in place, assert the collection comes from the second. Remove the producer's `doStart` and it fails with `Wanted but not invoked`. One deliberate wrinkle: the producer resolves its collection **twice** on first start, once in the constructor and once in `doStart`. I kept the constructor resolution because the producer is currently usable without being started and several existing tests rely on that. The second lookup is cheap — the SDK caches scopes and collections — but say the word and I will drop the constructor path and start the producer in those tests instead. Module suite 42/42. The consumer test now mocks `Cluster.connect`, since the consumer genuinely depends on the endpoint's connection after this change. _Claude Code on behalf of oscerd_ -- 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]
