Repository navigation
CAMEL-25939: camel-milo - the retry of refused monitored items follows reconnectTimeout in its docs and tests - #28254
Conversation
…s reconnectTimeout in its docs and tests Since CAMEL-25692 and CAMEL-25873, a monitored item that the server refused is created again every reconnectTimeout milliseconds, as the WARN of the refusal already logs. The component page and the 4.24 upgrade notes still said every 10 seconds, and the description of the option named only the connection attempts; they now name reconnectTimeout. MonitoredItemRetryTest and the consumers of CommunicationLossTest that share the connection of the refused node set reconnectTimeout to one second, with waits to match. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
LGTM. Docs and tests are consistent with the existing behavior; no production code changes.
- Documentation updates (Javadoc, component page, upgrade guide, catalog JSON, DSL factories) all correctly describe
reconnectTimeoutas governing both reconnect attempts and the retry interval for server-refused monitored items — consistent with what the code already does. - Test changes set
reconnectTimeout=1000so the retry fires within a 1-second window, and theawait()timeouts are reduced proportionally (15s → 3s, 12s → 3s, 30s → 10s). ThethroughRelay()helper is used consistently for both route registration and endpoint lookup, so the endpoint identity is preserved. ⚠️ Pre-fix test validation: dynamic check skipped (docs/test-only PR, no production path change). Static trace:reconnectTimeout=1000via URI →MiloClientConfiguration.setReconnectTimeout()(validated > 0, stored) → retry loop uses the field. With a 1s retry interval the refused node would be re-created within 1s; the 3sawait().during()window makes the test reliable. The timing improvement reported in the PR description (52s → 13s forMonitoredItemRetryTest) corroborates that the parameter is wired correctly.- Static analysis flagged pre-existing findings (empty-catch blocks and broad-exception in
MiloClientConfiguration.setAllowedSecurityPolicies(), autocloseable sockets in relay infrastructure) — none introduced by this PR.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks etzel-mes, this is the follow-up agreed in #28212 and it matches the code: SubscriptionManager.Connected.scheduleRetry() and the reconnect in connect() both schedule on reconnectTimeout, and retryRefused skips an attempt while the session is not active, so the new wording of the option ("Neither attempt is made while the OPC UA client recovers a lost session on its own") is correct. The description is the same in the generated JSON, the catalog copy and the component and endpoint DSL. No option is renamed, removed or given a new default, so the 4.24 notes are wording fixes only.
The tests keep their meaning with the shorter interval. Every endpoint that shares a connection gets reconnectTimeout=1000: all of MonitoredItemRetryTest goes through MILO_CLIENT_BASE, and all of the relay consumers of CommunicationLossTest now go through throughRelay. That matters, because toCacheId() does not include reconnectTimeout, so an endpoint without it would take the default if it opened the connection first. The quiet periods of 3 s still cover several retry attempts.
- Optional: the note of CAMEL-25873 in the 4.24 upgrade guide ("the reconnectTimeout of the components sets the delay between connection attempts") still names only the connection attempts. Consider one sentence there saying the same option sets how often refused monitored items are created again. A user who lowers
reconnectTimeoutto reconnect faster also gets one CreateMonitoredItems call per interval for every refused node, for example a node id with a typo, for as long as the connection lasts.
LGTM once the build jobs are green.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
… names the retry of refused monitored items A lower reconnectTimeout also repeats the CreateMonitoredItems request for refused nodes more often, which the note of CAMEL-25873 did not say. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks @davsclaus. Added one sentence to the CAMEL-25873 note in 14806b5: the option also sets how often refused monitored items are created again, so a lower value repeats the CreateMonitoredItems request for them, for example for a node ID with a typo, more often while the connection lasts. Docs only. Claude Code on behalf of etzel-mes |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks Serdar,
The earlier optional point is addressed in 14806b5. The CAMEL-25873 note in the 4.24 upgrade guide now says that reconnectTimeout also sets how often refused monitored items are created again, and what a lower value costs. That matches scheduleRetry(). The commit only touches the guide. It still merges cleanly with main and with #28252, which adds its note right after this one. There were no inline threads to resolve.
LGTM, to merge once CI is green.
Claude Code on behalf of @davsclaus.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔁 1 test passed only after a retry on JDK 21 (1 retried attempt) Recovered flaky tests on JDK 21 (1)
🔬 Scalpel shadow comparison — Scalpel: 8 of 707 tested, 23 compile-only — current: 8 all testedMaveniverse Scalpel detected 8 affected modules (current approach: 8). Skip-tests mode would test 8 modules (5 direct + 7 downstream), skip tests for 23 (generated code, meta-modules) Modules Scalpel would test (8)
Modules with tests skipped (23)
All tested modules (35 modules, 5m 48s total)Total reactor time: 5m 48s
Top 20 slowest modules:
|
Description
The follow-up agreed in the review of #28212. Since CAMEL-25692 (#28212) and CAMEL-25873 (#28219), a monitored item that the server refused is created again every
reconnectTimeoutmilliseconds (10000 by default), as the WARN of the refusal already logs. This aligns the texts and the tests with it:reconnectTimeout(10 seconds by default). The 4.24 note on server certificate validation said the client "tries again every 10 seconds"; connection attempts followreconnectTimeouttoo, so it now uses the wording of the component page.reconnectTimeoutoption named only the connection attempts; it now names the retry of refused monitored items as well (generated JSON, catalog and DSL updated).MonitoredItemRetryTestand the two consumers ofCommunicationLossTestthat share the connection of the refused node setreconnectTimeout=1000, with waits to match: quiet periods of 15 and 12 s become 3 s, and upper bounds of 30 s become 10 s.No change in behaviour; the main code changes are javadoc only.
Tests, run locally on JDK 21 with
-Dsurefire.rerunFailingTestsCount=0:MonitoredItemRetryTestCommunicationLossTest#refusedNodeIsCreatedOnceTheSessionIsActiveAgainIn the second test the retry now creates the node within a second after the session is back. The node's value still arrives about 10 s later, because the embedded server first answers the Publish requests it had queued for the dropped channel. That queue, not the retry interval, sets the test's duration.
Module
-Psourcecheck verify: 99 tests, 0 failures, 1 skipped.Fixes CAMEL-25939.
@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