tyskjohan commented on code in PR #28195:
URL: https://github.com/apache/camel/pull/28195#discussion_r4333398125
##########
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).
Review Comment:
Applied in dec1cd227, wrapped to the formatter's line length. The Javadoc
now says that even when `isSendNoop()` returns false, the consumer sends a noop
before the next poll after an operation failed with an I/O error, and links to
`sendNoopOnNextPollIfIOFailure(Throwable)`.
_Claude Code on behalf of tyskjohan_
##########
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:
Added in dec1cd227, close to your wording: "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 connects again. Keep
`sendNoop=true` for such a consumer." I split the entry into a paragraph for
ftp/ftps and one for sftp/mina-sftp, and the same hint is in the `setSendNoop`
Javadoc. The option description stays short (see the thread on
`RemoteFileConfiguration`).
The ftp/ftps paragraph now also covers your side note: whatever `sendNoop`
is, the consumer still sends a noop before it renames or deletes a file on the
server, as it does for each processed file unless `noop=true`. I left
`FtpOperations.deleteFile()` / `renameFile()` as they are, for a follow-up
JIRA. The sftp and mina-sftp operations only call `isConnected()` there.
_Claude Code on behalf of tyskjohan_
##########
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:
Done in dec1cd227. `disconnect()` and `forceDisconnect()` now clear
`sendNoopOnNextPoll` next to `loggedIn = false`. Because `loggedIn` is false,
the next poll connects again either way, so the only change is that the noop on
the closed client is skipped. Two new parameterized tests (ftp/ftps/sftp) check
that no noop is sent after an I/O failure followed by `forceDisconnect()` or
`disconnect()`. With the reset removed, those 6 cases fail.
_Claude Code on behalf of tyskjohan_
##########
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:
Shortened in dec1cd227. The description is now the first two sentences plus
"However if this causes problems, you can turn this option off, and then a lost
connection may only be found when the next operation fails." The per-protocol
details are only in the `setSendNoop` Javadoc and the upgrade guide. The
component JSON, catalog and endpoint DSL are regenerated.
_Claude Code on behalf of tyskjohan_
--
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]
