mattcasters commented on code in PR #9187:
URL: https://github.com/apache/hop/pull/9187#discussion_r4435537020
##########
docs/hop-user-manual/modules/ROOT/pages/workflow/actions/ftp.adoc:
##########
@@ -119,8 +119,8 @@ What this action passes on to the next action. See
xref:workflow/action-results.
* All works fine (default): no file failed. No file matching the wildcard is
also true.
* At least we got x files: at least *Limit files* files were downloaded,
whatever the number of errors.
-* Nr errors less than: the number of errors is at most *Limit files*. The
download stops as soon as the number of errors reaches *Limit files*, and the
action is still true then. Some failures before the download also count as 1
error: see False.
-|False|The success condition is not met. Without *Nr errors less than*, the
download stops at the first file that fails. The action is also false, before
any download, when the server can't be reached or refuses the login, the remote
directory doesn't exist, the wildcard is not a valid regular expression, or
*Move files after retrieval* is checked while the move-to folder is empty, or
doesn't exist and *Create folder* is unchecked. With *Nr errors less than*, an
unreachable server, a refused login and an invalid wildcard count as 1 error,
so the action is true when *Limit files* is 1 or more. A missing remote
directory and the move-to folder checks give false whatever the condition.
+* Nr errors at most: after the action is ready to download files, the number
of errors is at most *Limit files*. The download stops as soon as the number of
errors reaches *Limit files*, and the action is still true then.
+|False|The success condition is not met. Without *Nr errors at most*, the
download stops at the first file that fails. The action is also false, whatever
the success condition, when it fails before any download: the server can't be
reached or refuses the login, the remote directory doesn't exist, the wildcard
is not a valid regular expression, or *Move files after retrieval* is checked
while the move-to folder is empty, or doesn't exist and *Create folder* is
unchecked.
Review Comment:
**[suggestion]** "Without *Nr errors at most*, the download stops at the
first file that fails" is only true for *All works fine*.
*At least we got x files* never sets `successConditionBroken`
(`Download.updateErrors` only breaks on no-errors and on the error limit), so
the download continues and the True bullet above is right that the action can
still succeed "whatever the number of errors". This False cell says the
opposite.
**Suggestion:** Say that only *All works fine* stops at the first failed
file, and that *At least we got x files* walks the whole list and checks the
downloaded count at the end. The FTP delete result table has the same sentence.
##########
docs/hop-user-manual/modules/ROOT/pages/workflow/actions/ftpdelete.adoc:
##########
@@ -108,8 +108,8 @@ What this action passes on to the next action. See
xref:workflow/action-results.
* All works fine (default): no file failed. No file matching the wildcard is
also true, and so is *Copy previous results to args* without previous result
rows.
* At least we delete x files: at least *Limit files* files were deleted.
-* Nr errors less than: the number of errors is at most *Limit files*. A
failure before deleting counts as 1 error, so the cases listed under False give
true with 1 error when *Limit files* is 1 or more.
-|False|The success condition is not met, for example because the server can't
be reached or refuses the login, the remote directory doesn't exist, the
wildcard is not a valid regular expression, or *Wildcard* is empty while *Copy
previous results to args* is unchecked. The number of errors is the number of
files that failed, or 1 when the action fails before deleting. With *Nr errors
less than*, these failures count as 1 error, so the action is true when *Limit
files* is 1 or more.
+* Nr errors at most: after the action is ready to delete files, the number of
errors is at most *Limit files*. The delete stops as soon as the number of
errors reaches *Limit files*, and the action is still true then.
+|False|The success condition is not met. Without *Nr errors at most*, the
delete stops at the first file that fails. The action is also false, whatever
the success condition, when it fails before deleting: the server can't be
reached or refuses the login, the remote directory doesn't exist, the wildcard
is not a valid regular expression, or *Wildcard* is empty while *Copy previous
results to args* is unchecked. The number of errors is the number of files that
failed, or 1 when the action fails before deleting.
Review Comment:
**[suggestion]** "Without *Nr errors at most*, the delete stops at the first
file that fails" is only true for *All works fine*.
*At least we delete x files* does not set `successConditionBroken`.
`Deletion.updateErrors` says so: with that condition the count at the end
decides, and a single error must not stop the run. A refused delete is counted,
later files are still deleted, and the action is true when enough files were
removed.
**Suggestion:** Limit the "stops at the first file" sentence to *All works
fine*, and describe *At least we delete x files* as running to the end of the
list. Same fix as on the Get a file with FTP page.
##########
plugins/tech/ftp/src/main/java/org/apache/hop/workflow/actions/ftpdelete/ActionFtpDelete.java:
##########
@@ -301,9 +301,14 @@ public Result execute(Result result, int nr) {
return result;
}
+ // Connection, listing and wildcard are ready: from here on, success
conditions apply.
+ deletion.transferAttempted = true;
deleteFiles(filenames, selector, ftpClient, sftpClient, deletion);
} catch (Exception e) {
- deletion.updateErrors();
+ // deleteFiles already counted the file error that broke the condition;
do not count again.
+ if (!deletion.successConditionBroken) {
+ deletion.updateErrors();
+ }
logError(BaseMessages.getString(PKG, "ActionFTPDelete.ErrorGetting",
e.getMessage()), e);
Review Comment:
**[suggestion]** Stopping at the limit is now a success, but this catch
still logs that stop as a failure.
`deleteFiles` already counts the file error and logs
`ActionFtp.UnexpectedError`. For *Nr errors at most* it then throws
`ActionFTPDelete.SuccesConditionBroken` as soon as `nrErrors >= limit`. This
guard correctly skips a second `updateErrors()`, so `nrErrors == limit` and
`isSuccess()` stays true. Execution still falls through to `logError`, which
writes `Error getting files from FTP : Success condition was broken!` plus a
stack trace onto an action that follows the success hop. With one bad file and
Limit files = 1 that happens on every success-at-the-limit run, because delete
throws immediately rather than waiting for a later file.
**Suggestion:** Log only when this catch is the first report of the failure
(`if (!deletion.successConditionBroken)`), or `return` from `deleteFiles` once
the condition breaks instead of throwing. Keep the per-file `UnexpectedError`
log. `ActionFtp` already avoids the double count and only hits its outer
`logError` when a later file trips the same stop; the same guard belongs there
if that log is cleaned up.
--
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]
