Repository navigation
CAMEL-25873: camel-milo - the reconnectTimeout option sets the delay between connection attempts - #28219
Conversation
…between connection attempts The reconnectTimeout setter of the milo-client and milo-browse components set the request timeout of every service call, and the delay between two connection attempts stayed fixed at 10 seconds. The new reconnectTimeout option of the client configuration sets that delay, as an endpoint option and through the component configuration, and both component setters now set it. Its default of 10000 milliseconds keeps the previous delay. A value of 0 or less is rejected, because the client would try to connect again without pause. Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks Serdar for the careful analysis, and for testing against a real OPC UA server too. The bug is clear: both components' setReconnectTimeout called setRequestTimeout, and the delay between connection attempts was the literal 10_000 passed to SubscriptionManager. The new reconnectTimeout option on MiloClientConfiguration (default 10000, the previous delay) goes through the copy constructor, so endpoints inherit the component default, and MiloClientConnection hands it to SubscriptionManager, whose only use is the delay before it schedules connect. Rejecting values of 0 or less is a sensible guard against a tight retry loop.
Tests: ReconnectTimeoutConfigurationTest covers the default, the endpoint option, both component setters and the rejected values, and the new CommunicationLossTest case fails with the hard-coded delay. Both use JUnit assertions like the rest of the module, and timed MockEndpoint assertions rather than sleeps. The generated configurers, JSON, catalog and DSL files are included, and the behaviour change has a 4.24 upgrade-guide entry.
Non-blocking note:
- The upgrade-guide entry tells users to call
setRequestTimeoutinstead, but neitherMiloClientComponentnorMiloBrowseComponenthas such a method. There is an inline hint with a wording that points to therequestTimeoutoption.
Conventions check: the commit, branch and JIRA linkage (CAMEL-25873) follow the project guidelines. CI was still running when I looked.
This review covers project rules and conventions only. It does not replace CodeRabbit, Sourcery or SonarCloud.
Claude Code on behalf of Claus Ibsen
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
…ut option The components have no setRequestTimeout method; the request timeout is the requestTimeout option of the configuration. Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
a5957af to
e3716c0
Compare
|
🌟 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 Serdar. The upgrade-guide sentence now points to the requestTimeout option, which both components expose, so my earlier point is addressed. Nothing else changed since my last review.
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.
|
🧪 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 20s total)Total reactor time: 5m 20s
Top 20 slowest modules:
|
Description
MiloClientComponent.setReconnectTimeoutandMiloBrowseComponent.setReconnectTimeoutset the request timeout of every service call (since the first version of each component), and the delay between two connection attempts was the literal 10000 ms inMiloClientConnection. Settingcamel.component.milo-client.reconnectTimeout=2000to retry faster therefore kept the 10 s delay and made every request time out after 2 s: a request that takes longer, such as a method call, failed withBad_Timeout, the connection was rebuilt, and every consumer of the connection reportedBad_NoCommunication.This adds a
reconnectTimeoutoption toMiloClientConfiguration(label client, default 10000 ms, the previous delay), on both endpoints and through the component configuration, and both component setters now set it. It is the delay before Camel tries again to connect after an attempt failed, at startup or while Camel rebuilds a failed connection. The Milo client still recovers a lost session on its own, with its own timing. A value of 0 or less is rejected, because the client would then retry without pause while the server refuses the connection.setRequestTimeoutnames its parameterrequestTimeoutnow.Tests:
ReconnectTimeoutConfigurationTest(default, endpoint option, both component setters, the request timeout left unset, a value that is not positive) andCommunicationLossTest#consumerStartedWithoutTheServerConnectsAfterTheConfiguredReconnectTimeout, which fails with the hard-coded delay. Also run against a real OPC UA server (node-opcua) that starts listening 35 s after the client starts, with a method that answers after 5 s:reconnectTimeout=2000Bad_Timeoutafter 2 s, connection rebuiltreconnectTimeout=2000reconnectTimeout=2000Without the new check, the endpoint option
reconnectTimeout=0made 54,272 connection attempts, each logged as a WARN, in the 21.5 s before the server listened; with it, the endpoint is rejected at startup withreconnectTimeout must be > 0, got: 0.When the server was killed for 40 s after the client had connected, the option made no difference: in both runs the Milo client reconnected on its own back-off, about 16 s after the server listened again, and Camel then rebuilt the connection at once, because the restarted server no longer had the subscription.
Docs: the option (generated), the sentence of the component page on retrying after a certificate failure, and the 4.24 upgrade note. #28212 (CAMEL-25692) retries refused monitored items at this interval too; whichever of the two is merged second will also update its "every 10 seconds" sentences and the description of this option.
Fixes CAMEL-25873.
@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