Repository navigation
CAMEL-25692: camel-milo - create a refused monitored item again instead of dropping it - #28212
Conversation
…ad of dropping it A node whose monitored item the server refuses is still reported once with the status of the refusal. While the connection lasts, Camel now creates the refused items of the connection again every 10 seconds in one request, quietly, until the server accepts them; the first value of a created item is the current value of the node. This covers nodes and namespace URIs that the server gets later; the namespace table is read once per attempt. The attempt is skipped while the session is not active, a failed attempt is made again with the next one and never rebuilds the connection, and a stopped consumer is not retried. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Solid implementation. The concurrency model is sound under the existing lock discipline. Two low-severity observations below.
The putSubscriptions return value is intentionally discarded in retryRefused because retry=true means refuse() never adds PendingValues. A one-line comment at the call site would help future readers. The tests are thorough — three distinct retry scenarios plus integration with the lifecycle and communication-loss suites.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks Serdar, this is a nice improvement and it follows the proposal in the JIRA. The retry is bounded per attempt, runs on the component executor, stops cleanly on consumer stop, dispose and reconnect, and a failed attempt leaves no monitored item behind in Milo's subscription. The tests and docs are in place.
Point A inline (the retry blocking under lock) is the one I would like settled before merge; the others are small.
Two questions, fine as follow-ups:
- A node that is never created, for example a typo in the node id, is retried every 10 s for the life of the connection, with only DEBUG after the first WARN. Would it be worth exposing the refused nodes, for example in a health check or the dev console?
- A retry that keeps getting a bad service result (for example
Bad_TooManyOperationsorBad_ServiceUnsupported) is also retried forever at DEBUG and never escalated. Is that intended?
Claude Code on behalf of davsclaus. This is a rules-and-conventions review, not a replacement for static analysis or specialised review tools. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 8 of 707 tested, 25 compile-only — current: 8 all testedMaveniverse Scalpel detected 8 affected modules (current approach: 8). Skip-tests mode would test 8 modules (3 direct + 7 downstream), skip tests for 25 (generated code, meta-modules) Modules Scalpel would test (8)
Modules with tests skipped (25)
All tested modules (35 modules, 5m 22s total)Total reactor time: 5m 22s
Top 20 slowest modules:
|
…retry interval in the warning Review of the retry of refused monitored items. The next attempt of a connection is cancelled under the lock that guards it wherever the connection is replaced or disposed, also in the dispose of the manager, which disposes the connection itself outside of the lock. The warning for a refused node says that Camel tries again and how often. The javadoc of retryRefused says that the producers wait for an attempt and why that wait is bounded. CommunicationLossTest waits with during() and checks that the refused node is not reported while the server is unreachable, instead of sleeping. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review, Claus. c928243 addresses B, C and D and adds the javadoc for A; the reasoning for A is in its thread.
Claude Code on behalf of etzel-mes |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — both previous findings are fully addressed by this commit.
gnodet-bot finding #1 — retryJob lock safety: ✅ Addressed. cancelRetry() is now called under lock in all four paths (SubscriptionManager.dispose(), handleConnectionFailure(), performAndEvalConnect() on both old-connection and new-connection failure). Connected.dispose() runs after the lock is released and no longer touches retryJob. The race is eliminated.
gnodet-bot finding #2 — stale handle in refused: ✅ Addressed. deactivate() now calls this.refused.remove(clientHandle) before removing from goodSubscriptions, and deactivate is called under lock. Any handle that escapes one cycle is harmlessly filtered by the null-subscription check in retryRefused.
davsclaus finding B: ✅ Same as finding #1 — resolved.davsclaus finding C: ✅ CommunicationLossTest now uses await().during(12s).atMost(15s) instead of a bare sleep.davsclaus finding D: ✅ WARN message now reads "...trying again every {} ms: {}".
davsclaus finding A (lock held during I/O): The Javadoc on retryRefused clearly documents this as intentional, mirroring the established pattern for consumer registration. The wait is bounded by session liveness and request timeout. Accepted.
Previous findings are all addressed. LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks Serdar, the points of my earlier review are addressed:
- A (retry under
lock): answered. The numbers from the node-opcua run and the new javadoc ofretryRefusedexplain why the wait is bounded, and I agree that splittingputSubscriptionsinto prepare/call/apply is not worth the new interleavings. Yes please to the separate JIRA for the producers not takinglock. gnodet's draft #27956 (CAMEL-25694) changes the same four producer methods (call,write,readValues,browse), so please link the two. - B: fixed.
cancelRetry()is called underlockeverywhere, andConnected.dispose()no longer touchesretryJob. This also covers gnodet-bot'svolatilecomment. - C: fixed with
during, which now also checks that the refused node is not reported during the outage. - D: fixed, the WARN says the item is tried again and how often.
- Questions 1 and 2: answered. A dev console that lists the refused nodes per connection sounds right; yes please to the JIRA.
Two notes, not blocking:
- On gnodet-bot's second comment (an orphaned handle in
connected.refused): I do not think it can happen.unregisterItemsremoves the handle fromsubscriptions, anddeactivateremoves it fromrefused, under the same lock. The null check inretryRefusedis only defensive. Because of that check,stoppedConsumerIsNotCreatedAgainwould also pass without therefused.removeindeactivate. That is fine for a test that pins the behaviour. - I checked the Milo 1.1.8 contract.
OpcUaSubscription.createMonitoredItemsdoes not throw on a lost connection or a reset subscription. It returns a bad service result for each item (Bad_InvalidStatewhen the subscription has no server state), so a failed attempt removes its items again and leaves nothing behind. Milo splits aBad_TooManyOperationscall into one item per call, andBad_TooManyMonitoredItemscomes back as an operation result, so it is retried as the JIRA asks.
There is no code overlap with #27956, which only touches the producer methods.
LGTM.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
TL;DR
Thanks Serdar, the retry is nicely contained. It runs on the executors of the connection and is cancelled under lock wherever a connection is replaced or disposed. It never rebuilds the connection, and the tests cover a late node, a late namespace, a stopped consumer and an outage. No blockers from this review; one new race worth a look.
Blockers
- None
Other findings: 2 (1 non-blocking, 1 suggestion)
1. A refusal still being delivered can follow the first value of an item that a retry created in the meantime (non-blocking)
Subscription.refuse() (SubscriptionManager.java:300-301) sets refused only when the PendingValue is delivered. Those deliveries run outside lock and run the route for one node after another (:1111 recreate, :1634 registerItems). A newly refused node joins a retry chain that may already be running (:508), so its first attempt can come well before 10 s. If that attempt creates node N (:451 clears refused) before N's pending refusal is delivered, the route receives N's first Good value and then the stale refusal. refused then stays true, so report() (:313) drops Bad_NoCommunication for N for the rest of the connection. It needs a consumer with several nodes, a slow route and a node that appears right after its refusal, so it is unlikely, but it comes with the same-connection retry. The reconnect path is safe: its deliveries and the retry timer share the single reconnect thread. Could we record the refusal under lock and deliver it only while it still holds? For example:
// Connected.refuse(), first refusal: under lock, a later successful creation clears it (:451)
s.refused = true;
pendingValues.add(new PendingValue(s, this.generation, new DataValue(status)));
// Subscription
synchronized void refuse(final long generation, final DataValue status) {
if (this.refused) { // not created by a retry in the meantime
deliver(generation, status);
}
}2. Test time (suggestion)
The interval is fixed at 10 s (MiloClientConnection passes 10_000), so the new tests wait for one or more full intervals (MonitoredItemRetryTest.java:193 15 s, CommunicationLossTest.java:240 12 s). Once CAMEL-25873 lets reconnectTimeout set the interval, they could use a shorter one.
Housekeeping: B, C, D and the retryJob visibility note look addressed by c928243. The orphan-handle note looks moot, because deactivate() (:553) drops the handle under the same lock that removes it from subscriptions.
Reviewed with Claude Code on behalf of oscerd. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
…ill refused The refusals of a registration or of a recreated subscription are delivered outside the lock, one node after another. When the next attempt, or a new connection, created a node before its refusal was delivered, the route received the first value of the node and then its refusal, and the node stayed marked as refused, so a later loss of communication was no longer reported for it. The node is now marked as refused under the lock once the call that refused it has succeeded, and its refusal is delivered only while that still holds. A call that fails leaves the node unmarked, so that it still receives the loss report of the connection. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review, Andrea.
@davsclaus the two follow-ups are CAMEL-25912 for the producers not taking Claude Code on behalf of etzel-mes |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks Serdar. I traced c68d3a1: the node is marked refused under lock once putSubscriptions succeeds, refused is cleared under the same lock when the item is created, and Subscription.refuse() delivers only while the flag holds. A refusal overtaken by a retry or a new connection is dropped. One already being delivered still comes before the first value, because the value goes through the same monitor. A call that fails leaves the nodes to the loss report. This covers Andrea's finding 1. refusalOvertakenByTheNextAttemptIsNotDelivered pins it, and the ordering it relies on holds because the consumer and registerItems both keep the node order. Thanks also for opening CAMEL-25912 and CAMEL-25913.
LGTM, once CI is green.
This review was generated by an AI agent (Claude Code on behalf of davsclaus) and may contain inaccuracies. Please verify all suggestions before applying.
Description
A
milo-clientconsumer whose monitored item the server refuses (for exampleBad_NodeIdUnknown, or a namespace URI that is not in the namespace table yet) is reported once with the status of the refusal, and the item is not created again until the route restarts or the connection is rebuilt. In automation the node often arrives later: a PLC program or tag configuration deployed after the integration started, a server whose address space is built after it accepts sessions, a tag removed and added again by an engineering change.With this change the refused nodes of a connection are created again every 10 seconds (the existing reconnect interval, no new option) in one
CreateMonitoredItemsrequest, while the connection lasts. A node that the server refuses again is logged at DEBUG and nothing is delivered; the created item delivers the current value of the node as its first value, which also ends the refusal. The attempt is skipped while the session is not active. A request that fails is made again 10 seconds later and never rebuilds the connection, so it cannot interrupt the nodes that are monitored; a lost session or subscription is recovered as before. A stopped consumer is not retried. The namespace table is read once per attempt, so a namespace that the server adds later is found.The issue proposed reusing the
badSubscriptionsresubscribe. CAMEL-25671 removed it because it could not be reached, so the retry is new code on the executors of the connection from CAMEL-25713.Tests:
MonitoredItemRetryTest(node added later, namespace added later, stopped consumer), the extendedMonitorItemLifecycleTest(node removed and added again in the same session) andCommunicationLossTest(node refused across an outage; the skipped attempt holds no lock). The tests of the first three cases fail without the change (rerun=0); the stopped-consumer test pins that a stopped consumer is not created again. The behaviour was also run against a real OPC UA server (node-opcua): a node added later, a node removed and added again, a namespace added later, 20 s and 45 s silent network drops while a node is refused, a route stopped while its node is refused, and two nodes refused for 3 minutes (one request every 10 seconds).Docs: the component page ("Monitoring several nodes") and the 4.24 upgrade note on monitored items.
Fixes CAMEL-25692.
@davsclaus @oscerd
Target
mainbranch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.AI-assisted contributions
Co-authored-bytrailers) and the PR description identifies the AI tool used.Claude Code on behalf of etzel-mes