davsclaus commented on code in PR #28556:
URL: https://github.com/apache/camel/pull/28556#discussion_r4386725087
##########
components/camel-plc4x/src/main/java/org/apache/camel/component/plc4x/Plc4XTriggeredConsumer.java:
##########
@@ -111,10 +124,22 @@ protected int poll() throws Exception {
}
/**
- * Reads the trigger and the tags in one request; fails unless every tag
was read and the trigger is a boolean.
+ * Reads the trigger and the tags in one request; fails unless every tag
was read and the trigger is a boolean. A
+ * connection that is lost without autoReconnect also stops the polling.
*/
private PlcReadResponse read() throws Exception {
plc4XEndpoint.reconnectIfNeeded();
+ // a connection that is lost after reconnectIfNeeded() fails the read
instead, and the next poll shuts it down
+ if (!plc4XEndpoint.isAutoReconnect() && plc4XEndpoint.connection ==
null) {
Review Comment:
Nit (non-blocking): this reaches into the endpoint's `protected volatile`
field from the consumer. A small package-private method on `Plc4XEndpoint` (say
`boolean isShutDown()` / `hasConnection()`, next to `isConnected()`) would keep
the meaning of `connection == null` ("the lost connection was shut down, or
none was set up") in the class that sets it.
##########
components/camel-plc4x/src/main/java/org/apache/camel/component/plc4x/Plc4XTriggeredConsumer.java:
##########
@@ -147,4 +172,27 @@ private PlcReadResponse read() throws Exception {
public String toString() {
return "Plc4XTriggeredConsumer[" + plc4XEndpoint + "]";
}
+
+ /**
+ * Logs like the default exception handler of a consumer, but a failed
poll only at WARN with the stack trace when
+ * it starts a run of failed polls or stops the polling, and the following
failed polls of the run at DEBUG without
+ * it, so that a PLC that cannot be reached is not logged on every poll.
+ */
+ private final class FailedPollExceptionHandler extends
LoggingExceptionHandler {
+ FailedPollExceptionHandler(CamelContext camelContext) {
+ super(camelContext, Plc4XTriggeredConsumer.class);
+ }
+
+ @Override
+ public void handleException(String message, Throwable exception) {
+ // a failed poll is reported without an exchange and before it is
counted, so the first one of a run finds an
+ // error counter of 0, and the one that stops the polling finds
its task unscheduled
+ if (getErrorCounter() == 0 || !isSchedulerStarted()) {
Review Comment:
Suggestion (non-blocking): for the poll that stops the polling, the WARN
line comes from `ScheduledPollConsumer` as `Failed polling endpoint: ... . Will
try again at next poll`, while its cause says `polling stops until the route is
restarted`. The two halves of one log line contradict each other. Since this
handler already knows that case (`!isSchedulerStarted()`), it could replace the
trailing sentence of `message`, e.g. log `"Failed polling endpoint: " +
getEndpoint() + ". Polling stops until the route is restarted"` there. Only the
default handler would change; bridged/custom handlers still get the stock
message.
##########
components/camel-plc4x/src/main/java/org/apache/camel/component/plc4x/Plc4XTriggeredConsumer.java:
##########
@@ -86,6 +95,10 @@ protected int poll() throws Exception {
// the consumer stopped while the read was in progress
return 0;
}
+ if (getErrorCounter() > 0) {
Review Comment:
Question (non-blocking): this INFO is logged whatever exception handler is
configured, so with `bridgeErrorHandler=true` or a custom `exceptionHandler`
the log shows "succeeded again after N failed polls" without any earlier Camel
log line for those failures. I think that's actually useful (it's the one line
that says the PLC is back), just checking it is intended.
--
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]
