etzel-mes commented on PR #27780: URL: https://github.com/apache/camel/pull/27780#issuecomment-6176045586
Thanks @allthingssecurity, @davsclaus and @oscerd for the reviews. The new commit 12f42e05e07 addresses the points that belong in this PR; the rest is listed below with what the docs now say. **Backport of CAMEL-25599 (@davsclaus).** No backport is planned, and the CAMEL-25566 comment is corrected accordingly. The reasons: nobody has reported the symptoms of CAMEL-25599 so far; we found it while building the multi-node consumer, where thousands of items make a leftover or an ignored item visible. The fix is also not invisible: a stopped consumer now performs a `DeleteMonitoredItems` call (and waits for it while the server is unreachable), and a failed item produces a Bad `DataValue`. Those are reasonable changes for a minor release with an upgrade note, less so for an LTS patch release without a user asking for them. And the two changes touch the same lines of `putSubscriptions`, so one PR keeps the history readable. Could a committer set fixVersion 4.24.0 on CAMEL-25599 and resolve it together with this PR? If a user needs the fix on 4.22.x or 4.18.x, the result evaluation separates cleanly from the `nodes` feature (about +25/−8 in `SubscriptionManager` plus the test) and I can open a small PR against the LTS branch then. **1. Failed `CreateMonitoredItems` service call (@allthingssecurity, @davsclaus).** Done in this PR. The result loop now tells a rejected service call from a failed item: the items of the rejected partition are removed and deleted and a `UaException` is thrown, so `handleConnectionFailure` rebuilds the connection like for any other failed call (Milo partitions the call, so only the items whose `serviceResult()` is bad are affected). Test `MonitorItemLifecycleTest.failedCreateMonitoredItemsCallRebuildsTheConnection` uses your reproducer (subscription deleted on the server, then the consumer starts; the other consumer keeps working). **2. Nodes that appear after a server restart.** Not in this PR, but planned as the next one. The 10 s retry was removed on purpose: `SubscriptionManager` runs everything, including the retry and the blocking `createMonitoredItems`, on Milo's `Stack.sharedScheduledExecutor()`, which also drives the session FSM reconnect timers, and on one CPU a failed item kept the client from recovering after an outage. The fix for that is not the retry but the executor: in a follow-up I would run `SubscriptionManager` on a Camel-managed scheduled executor and bring the failed-item retry back on the reconnect delay (every 10 s, all failed items of the connection, Bad reported once). I will open the issue once this PR is merged. Until then the "Monitoring several nodes" section says that a node that cannot be monitored is reported once and not retried, and that restarting the route creates the items again. **3. Unknown `nsu=` namespace.** Not in this PR, as it is older than this change; it goes into the same follow-up, where a namespace that cannot be resolved is reported per node as `Bad_NodeIdInvalid` and retried with the other failed items instead of failing the connection. The docs sentence now says "a node that does not exist on the server" instead of "a node that cannot be monitored". **4. A failed node is invisible with the defaults.** Done. `putSubscriptions` logs one WARN per failed node with the node ID and the status code when the result is handled (once at create, not per value). The bad-status exchange itself still depends on `omitNullValues=false`; that the default drops status-only notifications is CAMEL-25626. **De-duplication by node ID.** Done. `nodes` is now de-duplicated by the parsed `ExpandedNodeId`, so `ns=0;i=2258` and `i=2258` create one item and the header carries the first spelling. (`nsu=` and `ns=` spellings of the same node stay distinct, as Milo compares them by index or URI.) **`file:`/`classpath:` node list.** Not planned for now, and the CAMEL-25566 comment is corrected. In practice the natural unit is one consumer per device or line, which keeps the inline list short and the route readable, and a node ID that contains a comma can use `node`. If a measured need for a resource form comes up, it can be a follow-up. **Bad-status exchanges inside the lock (@davsclaus).** Done. `putSubscriptions` collects the failed (consumer, status) pairs and delivers them after the lock is released, so a long list of failed nodes no longer holds `write`/`read`/`call` on the shared connection. **Docs.** Added: all nodes of a list share the consumer's `samplingInterval`, `queueSize`, `omitNullValues` and monitor filter options; tags that need other settings need their own consumer (@davsclaus). The 4.24 upgrade guide now also notes that `CamelMiloNodeId` is set on every consumer exchange, including single-`node` consumers (@oscerd). Module build with `-Psourcecheck`: 55 tests, 0 failures. No option or header description changed, so the generated files are untouched. The branch is rebased on current `main`; the workflows wait for approval. _Claude Code on behalf of etzel-mes_ -- 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]
