davsclaus commented on code in PR #28195:
URL: https://github.com/apache/camel/pull/28195#discussion_r4333294211
##########
docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc:
##########
@@ -2107,3 +2107,18 @@ its array headers (`tags[#2]:` becomes `tags[2]:`).
With `strict=false`, unmarshalling no longer returns `null` or a partial
result for malformed TOON. Only the non-strict
recoveries of the TOON specification apply, such as accepting an array whose
length differs from the declared length;
other malformed TOON fails with `IllegalArgumentException`, as in strict mode.
+
+=== camel-ftp, camel-sftp, camel-ftps, camel-mina-sftp - sendNoop also applies
to the consumer
+
+The `sendNoop` option now also applies to the check a consumer makes before
each poll, not only to the pre-write check
+of a producer, and its label is now `common,advanced` instead of
`producer,advanced`. The default is still `true`, so a
+consumer sends a noop before each poll as before.
+
+A consumer endpoint that already sets `sendNoop=false`, which was ignored
before (for example a URI shared by `from`
+and `to`), now skips that noop and only checks whether the client still
regards itself as connected. For ftp and ftps
+the client does not notice a connection that the server has closed, so such a
consumer finds it only when the next
+operation fails, during the poll or while processing a file. After a failure
caused by an I/O error, the consumer
Review Comment:
(not blocking) A concrete hint would help here. For example: "If the FTP
server closes idle connections and the `delay` between polls is longer than its
idle timeout, every other poll fails and is logged before the consumer
reconnects. Keep `sendNoop=true` for such a consumer." The same sentence could
go in the option description.
##########
components/camel-ftp-common/src/main/java/org/apache/camel/component/file/remote/RemoteFileConsumer.java:
##########
@@ -253,11 +255,47 @@ protected void forceDisconnect() {
}
}
+ /**
+ * Whether to send a noop before each poll to check that the connection is
still valid (the sendNoop option).
+ */
+ protected boolean isSendNoop() {
+ return getEndpoint().getConfiguration().isSendNoop();
+ }
+
+ /**
+ * Without the noop before each poll, a connection that the server has
closed may only be found when an operation on
+ * it fails. When the failure is caused by an I/O error, the consumer
therefore sends a noop before the next poll,
+ * and connects again if the connection is no longer valid.
+ *
+ * @param cause the cause of the failure
+ */
+ protected void sendNoopOnNextPollIfIOFailure(Throwable cause) {
+ if (!isSendNoop() && ObjectHelper.getException(IOException.class,
cause) != null) {
+ LOG.debug("Operation failed with an I/O error, sending a noop
before the next poll to: {}", remoteServer());
+ sendNoopOnNextPoll = true;
+ }
+ }
+
+ @Override
+ protected void handleException(String message, Exchange exchange,
Throwable t) {
+ // a failure while retrieving or processing a file is not thrown out
of the poll, so the poll strategy does not
+ // disconnect
+ sendNoopOnNextPollIfIOFailure(t);
+ super.handleException(message, exchange, t);
+ }
+
protected void connectIfNecessary() {
- // We need to send a noop first to check if the connection is still
open
+ // We need to check if the connection is still open, by sending a noop
first (default), or when sendNoop is
+ // turned off by only checking the connection state of the client,
which sends nothing to the server, unless an
+ // operation failed with an I/O error
boolean isConnected = false;
try {
- isConnected = getOperations().sendNoop();
+ if (isSendNoop() || sendNoopOnNextPoll) {
Review Comment:
(optional) When the listing fails, `FtpConsumer.ignoreCannotRetrieveFile()`
sets `sendNoopOnNextPoll`, and then
`RemoteFilePollingConsumerPollStrategy.rollback()` calls `forceDisconnect()`.
On the next poll this branch still calls `sendNoop()` on a closed client.
commons-net throws `IOException("Connection is not open")`, and the consumer
reconnects through `!loggedIn`. It works, but setting `sendNoopOnNextPoll =
false` in `disconnect()` / `forceDisconnect()` would skip that pointless call.
##########
components/camel-ftp-common/src/main/java/org/apache/camel/component/file/remote/RemoteFileConfiguration.java:
##########
@@ -108,10 +108,16 @@ public enum PathSeparator {
+ "By default when a
directory or file does not exist or insufficient permission, then an exception "
+ "is thrown. Setting
this option to <tt>true</tt> allows to ignore that instead.")
private boolean ignoreFileNotFoundOrPermissionError;
- @UriParam(label = "producer,advanced", defaultValue = "true", description
= "Whether to send a noop command "
-
+ "as a pre-write check before uploading files to the FTP server. <p/> This is
enabled by default as "
-
+ "a validation of the connection is still valid, which allows to silently
re-connect to be able to "
-
+ "upload the file. However if this causes problems, you can turn this option
off.")
+ @UriParam(label = "common,advanced", defaultValue = "true", description =
"Whether to send a noop command to "
Review Comment:
(optional) This description is long for the catalog and the endpoint DSL
Javadoc. Maybe keep the first two sentences, plus one line such as "With
sendNoop=false a lost connection may only be found when the next operation
fails", and leave the per-protocol details to the upgrade guide and the setter
Javadoc.
--
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]
