Repository navigation
CAMEL-26339: camel-plc4x - log a run of failed polls once and stop polling a lost connection without autoReconnect - #28556
Conversation
… consumer once The triggered consumer logged every failed poll at WARN with the stack trace, so a PLC that cannot be reached logged a stack trace on every poll. Its default exception handler now logs the first failed poll of a run at WARN with the stack trace and the following ones at DEBUG without it, and the consumer logs at INFO when a poll succeeds again. A configured exceptionHandler or bridgeErrorHandler still gets every failure, and the failed polls still count for the health check. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…polling without autoReconnect Without autoReconnect, every call of reconnectIfNeeded() after a lost connection logged the shutdown WARN and stopped the endpoint again, while the triggered consumer kept polling. The endpoint now shuts down once per lost connection, and not when no connection could be set up, so the producer and the polling consumer fail their exchanges without repeating the WARN. The triggered consumer fails the poll that finds the connection lost and stops polling until the route is restarted, so its health check stays down; its default exception handler logs that failure at WARN, even after other failed polls. Unscheduling from the poll interrupts the polling thread, so that interrupt is cleared before the failure is reported. 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.
TL;DR
Comment — the triggered plc4x consumer now logs a run of failed polls once at WARN (then DEBUG, then one INFO when the PLC is back), and without autoReconnect a lost connection shuts the endpoint down once and stops the polling until the route is restarted. Well scoped, tested, and documented; only non-blocking suggestions below.
Blockers
- None
Other findings: 3 suggestions (inline), none blocking.
Thanks @etzel-mes for the very thorough description and the before/after numbers from the live snap7 run — that made this easy to review.
What I checked:
FailedPollExceptionHandleris set in the constructor, beforeDefaultEndpoint.configureConsumer()runs, soexceptionHandler=#...andbridgeErrorHandler=truestill replace it (andbridgeErrorHandlerRoutesEveryFailedPollToTheErrorHandlerOfTheRoutecovers the latter).ScheduledPollConsumer.doRun()calls the handler beforeerrorCounter.incrementAndGet(), sogetErrorCounter() == 0is indeed the first failure of a run, and the counter inpoll()still holds the run length before the success resets it.unscheduleTask()cancels the future withcancel(true)on the polling thread itself, so the interrupt is already set whenThread.interrupted()clears it;isSchedulerStarted()is!futures.isEmpty(), so the stopping poll is logged at WARN. A restarted route reschedules.ScheduledPollConsumerHealthCheckreports DOWN whileerrorCounter > 0, so the consumer stays DOWN after the polling stops, as the description says.- The other users of the shared connection (
Plc4XConsumer,Plc4XPollingConsumer,Plc4XProducer) go throughbuildPlc*Request(), which already throwsPlcConnectionExceptionon anullconnection, so the silent return inreconnectIfNeeded()doesn't introduce an NPE. - Docs, catalog copy and the 4.24 upgrade-guide entry (same unreleased version as CAMEL-25617) are updated.
CI has not run yet: the workflows are waiting for a maintainer to approve them.
This review covers project rules and conventions; it does not replace CI, SonarCloud or other review tools.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of davsclaus
…the poll that stops it The failed poll that stops the polling of the triggered consumer logged the message of the scheduled poll consumer, which announces a next poll. It now says that polling stops until the route is restarted. The consumer asks the endpoint whether it has a connection through hasConnection() instead of reading the connection field. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Thoughtful fix on two fronts — noisy logging and connection-lost handling — for the triggered consumer.
Logging: a FailedPollExceptionHandler logs the first failed poll of a run at WARN with the stack trace and the rest at DEBUG, and a recovered poll at INFO ("succeeded again after N failed polls"), so a PLC that can't be reached no longer WARN-spams every period. The level decision keys off getErrorCounter() == 0 || !isSchedulerStarted() — the first failure of a run (counter still 0, reported before it's counted) and the poll that stops the scheduler get the full WARN, the rest DEBUG — and the "succeeded again" line reads the counter before it's reset. That timing is tightly coupled to ScheduledPollConsumer's internal ordering, but it's clearly commented and the dedicated repeated-failure test locks it in.
Connection-lost: with autoReconnect=false, reconnectIfNeeded() now closes the lost connection under the lock so only one concurrent caller shuts the endpoint down (and returns early when connection == null, nothing left to lose), and read() detects the shut-down connection, unschedules the task so polling stops until the route is restarted, and throws so the poll fails and the health check stays down. Clearing the interrupt (Thread.interrupted()) that unscheduleTask() set, so the failure is reported as a normal poll failure rather than an interrupt, is a subtle but correct touch — and calling stop() outside the lock (per the comment, a concurrent stop() holds the service lock while doStop() waits) avoids the deadlock. The upgrade guide documents the new "lost connection stops polling until restart" behaviour.
No blockers. (CI hasn't run here — fork PR — worth triggering before merge.)
Claude Code on behalf of oscerd
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
The three concerns from the earlier review are all addressed in f26f939. The implementation holds up under scrutiny:
FailedPollExceptionHandler.handleException(String, Throwable)correctly overrides the 2-arg method thatScheduledPollConsumer.doRun()calls. The handler fires beforeerrorCounter.incrementAndGet(), sogetErrorCounter() == 0reliably identifies the first failure of a run. ✅isSchedulerStarted()readsfutures.isEmpty(), andunscheduleTask()cancels futures and clears the list — so the "polling stops" branch in the handler fires correctly on the poll that calledunscheduleTask(). ✅hasConnection()reads thevolatilefield directly; concurrent polls that see a staletruewill proceed to a read that throws, then the next poll shuts down — both outcomes are safe. ✅Thread.interrupted()clears the interrupt set bycancel(true)on the current thread; no race possible since it is the same thread. ✅- Pre-fix test validation:
logsARunOfFailedPollsOnceAtWarnAndItsEndOnceAtInfoasserts the exact level sequence[WARN, DEBUG, DEBUG, INFO, …]; before the fix the default handler logs every failure at WARN, so the assertion would fail. ✅
One low-severity observation: LogCapture sets the Log4j logger level globally (not per-thread). If Maven runs these tests in parallel across the module, two test instances capturing the same logger class simultaneously will interfere. CamelTestSupport creates isolated Camel contexts but shares the JVM logger. This is not blocking — the fix is correct and the tests pass in isolation — but adding @Isolated (JUnit 5) or a note in the Javadoc of LogCapture would prevent intermittent failures if the Maven Surefire fork count is set below the test class count.
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:
|
|
🧪 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 708 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, 6m 3s total)Total reactor time: 6m 3s
Top 20 slowest modules:
|
LogCapture sets the level of a logger for the whole JVM and MockDriver is programmed statically, so the test must not run in parallel with other test classes when JUnit parallel execution is turned on. Follow-up of the review of #28556. Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com> Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Description
Follow-up of CAMEL-25617 (#28387), JIRA: CAMEL-26339. This implements the first two directions of the ticket.
Failed polls. The triggered consumer still fails every poll that cannot read the PLC, so the consumer health check,
bridgeErrorHandlerand a customexceptionHandlersee each failure as before. What changes is the consumer's default exception handler, which the consumer now sets itself, asNettyConsumerdoes withNettyConsumerExceptionHandler:GridFsConsumerdoes since CAMEL-25711;A configured
exceptionHandlerorbridgeErrorHandler=truestill replaces this handler.autoReconnect=false.Plc4XEndpoint.reconnectIfNeeded()logs "Connection lost and auto-reconnect is turned off, shutting down Plc4XEndpoint" and stops the endpoint once per lost connection, instead of on every call, and not at all when no connection could be set up. So the producer and the polling consumer, which share it, fail their exchanges without repeating the warning.Once the endpoint has shut the lost connection down, the triggered consumer fails that poll and stops polling with
getScheduler().unscheduleTask(), asrepeatCountdoes. That failure is logged at WARN even within a run, and restarting the route starts polling again. Unscheduling from the poll interrupts the polling thread, so the consumer clears that interrupt before the failure is reported.What PLC4X still logs. With
autoReconnect=true, every poll during an outage still tries to connect. PLC4X's TCP transport logs each failed attempt at ERROR with the stack trace and then throws the same failure to Camel (TcpTransportInstance, lines 136–141 in 1.0.0, unchanged ondevelop).DriverBasealso logs an INFO line ("Using connection string") for each attempt. In the live run below, a 10 s outage atperiod=100gave 97 of those ERROR lines, each with about 24 stack frames. Camel already reports the failure, so a logger level fororg.apache.plc4x.java.transport.tcp.TcpTransportInstancecan silence them. I did not find a Camel-side way to handle this that I would prefer, so suggestions are welcome.Not included: the backoff options (the third direction). They would also thin out the PLC4X lines above, since there would be fewer connect attempts during an outage, at the price of noticing the PLC's return later. But they need new endpoint options. Making
Plc4XEndpointaScheduledPollEndpointbrings all scheduler options to every plc4x endpoint, includingdelaynext to the existingperiod. Some of those options do not fit a trigger:backoffIdleThresholdcounts every poll without a rising edge as idle, so it thins out the polls and short trigger pulses can be missed;sendEmptyMessageWhenIdlesends an empty exchange on every such poll.Declaring just
backoffErrorThresholdandbackoffMultiplieron the endpoint would fit, but I found no other component that does it that way. So I left them out; if you would rather have those two options now, I can add them to this PR.Verification. Module build only (
mvn install -Psourcecheckincomponents/camel-plc4x; the root build was not run). The module's 43 tests were green in three consecutive runs with-Dsurefire.rerunFailingTestsCount=0. The new route tests fail on the code before the change, except the one that checks thatbridgeErrorHandlerstill gets every failure, which passes before and after. There are no generated changes, and the catalog copy of the component doc is byte-identical.The live check ran against the snap7 1.4.2 S7 server: a triggered consumer with
period=100ran for 30 s, the server was stopped after 10 s and started again 10 s later, with the camel-plc4x jar from before and after this change:period=100autoReconnect=true: "Failed polling endpoint" at WARN with stack traceautoReconnect=true: the same at DEBUG, message onlyautoReconnect=true: INFO when a poll succeeds againautoReconnect=true: PLC4XTcpTransportInstanceERROR with stack traceautoReconnect=false: "Failed polling endpoint" at WARNautoReconnect=false: "Connection lost ... shutting down Plc4XEndpoint"autoReconnect=false: failed polls@davsclaus could you have a look? (I cannot request reviewers.)
Target
mainbranch)Tracking
Apache Camel coding standards and style
I checked that each commit in the pull request has a meaningful subject line and body.
I have run
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.The code, tests and documentation were written with Claude Code (Claude Opus 5.5) under my direction; every commit carries the
Co-Authored-Bytrailer.🤖 Generated with Claude Code
Claude Code on behalf of etzel-mes