Re: [PR] NIFI-12249 FetchFTP and FetchSFTP processors now write failure reason to 'failure' attribute [nifi]

2023-10-28 Thread via GitHub 


exceptionfactory closed pull request #7929: NIFI-12249 FetchFTP and FetchSFTP 
processors now write failure reason to 'failure' attribute
URL: https://github.com/apache/nifi/pull/7929


-- 
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: issues-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12249 FetchFTP and FetchSFTP processors now write failure reason to 'failure' attribute [nifi]

2023-10-27 Thread via GitHub 


exceptionfactory commented on code in PR #7929:
URL: https://github.com/apache/nifi/pull/7929#discussion_r1374767594


##
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/FetchFileTransfer.java:
##
@@ -296,6 +298,18 @@ public void onTrigger(final ProcessContext context, final 
ProcessSession session
 } else {
 attributes.put(CoreAttributes.FILENAME.key(), filename);
 }
+
+if (failureRelationship != null) {
+attributes.put(FAILURE_REASON_ATTRIBUTE, 
failureRelationship.getName());
+flowFile = session.putAllAttributes(flowFile, attributes);
+session.transfer(session.penalize(flowFile), 
failureRelationship);
+if (provenanceEventOnFailure) {
+session.getProvenanceReporter().route(flowFile, 
failureRelationship);
+}
+cleanupTransfer(transfer, closeConnOnFailure, transferQueue, 
host, port);
+return;
+}

Review Comment:
   Thanks, that makes sense, and sounds good.



-- 
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: issues-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12249 FetchFTP and FetchSFTP processors now write failure reason to 'failure' attribute [nifi]

2023-10-27 Thread via GitHub 


exceptionfactory commented on code in PR #7929:
URL: https://github.com/apache/nifi/pull/7929#discussion_r1374765978


##
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/FetchFileTransfer.java:
##
@@ -254,36 +254,38 @@ public void onTrigger(final ProcessContext context, final 
ProcessSession session
 transfer = transferWrapper.getFileTransfer();
 }
 
+Relationship failureRelationship = null;
+boolean closeConnOnFailure = false;
+boolean provenanceEventOnFailure = false;

Review Comment:
   Reviewing the changes, it looks like this conditional check can be removed. 
Although the previous failure handling block did not call the Provenance 
Reporter on communications failure, this should be changed to always call 
`getProvenanceReporter().route()` for failures.
   ```suggestion
   ```



-- 
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: issues-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12249 FetchFTP and FetchSFTP processors now write failure reason to 'failure' attribute [nifi]

2023-10-27 Thread via GitHub 


exceptionfactory commented on code in PR #7929:
URL: https://github.com/apache/nifi/pull/7929#discussion_r1374765978


##
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/FetchFileTransfer.java:
##
@@ -254,36 +254,38 @@ public void onTrigger(final ProcessContext context, final 
ProcessSession session
 transfer = transferWrapper.getFileTransfer();
 }
 
+Relationship failureRelationship = null;
+boolean closeConnOnFailure = false;
+boolean provenanceEventOnFailure = false;

Review Comment:
   Reviewing the changes, it looks like this conditional check can be removed. 
Although the previous failure handling block did not call the Provenance 
Reporter on communications failure, this should be changed to always call 
`getProvenanceReporter().route()` for failures.



-- 
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: issues-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12249 FetchFTP and FetchSFTP processors now write failure reason to 'failure' attribute [nifi]

2023-10-27 Thread via GitHub 


annanys23 commented on code in PR #7929:
URL: https://github.com/apache/nifi/pull/7929#discussion_r1374757819


##
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/FetchFileTransfer.java:
##
@@ -296,6 +298,18 @@ public void onTrigger(final ProcessContext context, final 
ProcessSession session
 } else {
 attributes.put(CoreAttributes.FILENAME.key(), filename);
 }
+
+if (failureRelationship != null) {
+attributes.put(FAILURE_REASON_ATTRIBUTE, 
failureRelationship.getName());
+flowFile = session.putAllAttributes(flowFile, attributes);
+session.transfer(session.penalize(flowFile), 
failureRelationship);
+if (provenanceEventOnFailure) {
+session.getProvenanceReporter().route(flowFile, 
failureRelationship);
+}
+cleanupTransfer(transfer, closeConnOnFailure, transferQueue, 
host, port);
+return;
+}

Review Comment:
   Yes, this was intentional.  I would think the other attributes are needed 
for a follow on processor to have context (ex: generate email notifications 
with similar content to what is being logged in the catch blocks).  Also, this 
was meant to remove some redundant code.



-- 
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: issues-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12249 FetchFTP and FetchSFTP processors now write failure reason to 'failure' attribute [nifi]

2023-10-27 Thread via GitHub 


exceptionfactory commented on code in PR #7929:
URL: https://github.com/apache/nifi/pull/7929#discussion_r1374684426


##
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/FetchFTP.java:
##
@@ -47,7 +47,8 @@
 @WritesAttribute(attribute = "ftp.remote.port", description = "The port 
that was used to communicate with the remote FTP server"),
 @WritesAttribute(attribute = "ftp.remote.filename", description = "The 
name of the remote file that was pulled"),
 @WritesAttribute(attribute = "filename", description = "The filename is 
updated to point to the filename fo the remote file"),
-@WritesAttribute(attribute = "path", description = "If the Remote File 
contains a directory name, that directory name will be added to the FlowFile 
using the 'path' attribute")
+@WritesAttribute(attribute = "path", description = "If the Remote File 
contains a directory name, that directory name will be added to the FlowFile 
using the 'path' attribute"),
+@WritesAttribute(attribute = "failure", description = "If the transfer 
failed, the failure reason is written to this attribute")

Review Comment:
   This should be changed to reflect the updated attribute name:
   ```suggestion
   @WritesAttribute(attribute = "fetch.failure.reason", description = "The 
name of the failure relationship applied when routing to any failure 
relationship")
   ```



##
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/FetchFileTransfer.java:
##
@@ -296,6 +298,18 @@ public void onTrigger(final ProcessContext context, final 
ProcessSession session
 } else {
 attributes.put(CoreAttributes.FILENAME.key(), filename);
 }
+
+if (failureRelationship != null) {
+attributes.put(FAILURE_REASON_ATTRIBUTE, 
failureRelationship.getName());
+flowFile = session.putAllAttributes(flowFile, attributes);
+session.transfer(session.penalize(flowFile), 
failureRelationship);
+if (provenanceEventOnFailure) {
+session.getProvenanceReporter().route(flowFile, 
failureRelationship);
+}
+cleanupTransfer(transfer, closeConnOnFailure, transferQueue, 
host, port);
+return;
+}

Review Comment:
   Moving this logic to a separate block outside of the catch blocks introduces 
some potential logic changes, such as the addition of other attributes to the 
FlowFile, is that intentional? Otherwise it would be less of an impact to apply 
the attribute value in the appropriate catch blocks.



##
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/FetchSFTP.java:
##
@@ -47,7 +47,8 @@
 @WritesAttribute(attribute = "sftp.remote.port", description = "The port 
that was used to communicate with the remote SFTP server"),
 @WritesAttribute(attribute = "sftp.remote.filename", description = "The 
name of the remote file that was pulled"),
 @WritesAttribute(attribute = "filename", description = "The filename is 
updated to point to the filename fo the remote file"),
-@WritesAttribute(attribute = "path", description = "If the Remote File 
contains a directory name, that directory name will be added to the FlowFile 
using the 'path' attribute")
+@WritesAttribute(attribute = "path", description = "If the Remote File 
contains a directory name, that directory name will be added to the FlowFile 
using the 'path' attribute"),
+@WritesAttribute(attribute = "failure", description = "If the transfer 
failed, the failure reason is written to this attribute")

Review Comment:
   This should be changed to reflect the updated attribute name:
   ```suggestion
   @WritesAttribute(attribute = "fetch.failure.reason", description = "The 
name of the failure relationship applied when routing to any failure 
relationship")
   ```



-- 
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: issues-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12249 FetchFTP and FetchSFTP processors now write failure reason to 'failure' attribute [nifi]

2023-10-26 Thread via GitHub 


annanys23 commented on PR #7929:
URL: https://github.com/apache/nifi/pull/7929#issuecomment-1781856166

   That works for me.  Thanks for the discussion.


-- 
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: issues-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12249 FetchFTP and FetchSFTP processors now write failure reason to 'failure' attribute [nifi]

2023-10-26 Thread via GitHub 


exceptionfactory commented on PR #7929:
URL: https://github.com/apache/nifi/pull/7929#issuecomment-1781782831

   Thanks for the reply @mosermw.
   
   Introducing a FlowFile attribute that uses the relationship name seems like 
a good middle way in this scenario. It would support the use case of a single 
output Processor for failures if desired, providing a clear programmatic way to 
handle different types of failures.
   
   What do you think @annanys23?


-- 
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: issues-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12249 FetchFTP and FetchSFTP processors now write failure reason to 'failure' attribute [nifi]

2023-10-26 Thread via GitHub 


mosermw commented on PR #7929:
URL: https://github.com/apache/nifi/pull/7929#issuecomment-1781646495

   You make some very thoughtful points @exceptionfactory and it makes sense to 
provide fine grained error handling if the underlying library provides 
different error codes.  I spot checked some other Fetch processors which only 
have a single failure relationship, and I imagine their libraries don't enable 
finer grained handling as easily.
   
   How about one last idea before I join you with opposing this?  What if we do 
not include the log message in the attribute but only include the relationship 
name in the attribute?  And change the attribute name to 'fetch.failure.reason' 
which aligns with the FetchParquet processor.  This should enable this PR to be 
much simpler and easier to maintain.
   
   fetch.failure.reason=comms.failure or fetch.failure.reason=not.found or 
fetch.failure.reason=permission.denied


-- 
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: issues-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12249 FetchFTP and FetchSFTP processors now write failure reason to 'failure' attribute [nifi]

2023-10-26 Thread via GitHub 


exceptionfactory commented on PR #7929:
URL: https://github.com/apache/nifi/pull/7929#issuecomment-1781310538

   I can appreciate the desire to have fewer UpdateAttribute Processors in the 
case of handling different types of failures. Different Processors take 
different approaches in terms of failure handling, thinking of InvokeHTTP as 
one example of multiple categories of relationships.
   
   The value of specific relationships is a clear differentiation of error 
types. Using FlowFile attributes can be helpful where there are well known 
status properties, such as HTTP response codes.
   
   SFTP has some error codes, but even communication failures are different 
than other issues, like file not found or permission denied. One of the main 
reasons for having different relationships is that some errors could be retried.
   
   From that angle, keeping the separate relationships has value for a number 
of use cases, even though it might complicate notification uses cases.


-- 
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: issues-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12249 FetchFTP and FetchSFTP processors now write failure reason to 'failure' attribute [nifi]

2023-10-26 Thread via GitHub 


mosermw commented on PR #7929:
URL: https://github.com/apache/nifi/pull/7929#issuecomment-1781272173

   I think the main motivation for requests like this are dataflow managers who 
don't want a separate UpdateAttribute processor creating a detailed 
notification message for each of the failure relationships.  "If I have 10 
FetchSFTP processors then I need 30 UpdateAttribute processors, when really I 
only want 1".  At a large scale, little design decisions like this make a 
difference.
   
   I wonder what the community would think of consolidating the FetchSFTP 
"comms.failure", "not.found" and "permission.denied" relationships into 1 
"failure" relationship and moving forward with an attribute that describes the 
failure reason?  It would definitely be for NiFi 2.0 since it's a big breaking 
change. I suppose we would want @markap14 to weigh in on this as the original 
author.


-- 
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: issues-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[PR] NIFI-12249 FetchFTP and FetchSFTP processors now write failure reason to 'failure' attribute [nifi]

2023-10-24 Thread via GitHub 


annanys23 opened a new pull request, #7929:
URL: https://github.com/apache/nifi/pull/7929

   
   
   
   
   
   
   
   
   
   
   
   
   
   # Summary
   
   [NIFI-12249](https://issues.apache.org/jira/browse/NIFI-12249)
   
   # Tracking
   
   Please complete the following tracking steps prior to pull request creation.
   
   ### Issue Tracking
   
   - [x] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue 
created
   
   ### Pull Request Tracking
   
   - [x] Pull Request title starts with Apache NiFi Jira issue number, such as 
`NIFI-0`
   - [x] Pull Request commit message starts with Apache NiFi Jira issue number, 
as such `NIFI-0`
   
   ### Pull Request Formatting
   
   - [x] Pull Request based on current revision of the `main` branch
   - [x] Pull Request refers to a feature branch with one commit containing 
changes
   
   # Verification
   
   Unit tests updated to ensure failure attribute is set on the flow file.  
Some tests were updated to check for specific error conditions cited in the 
failure reasons written to the attribute.
   
   Also, verified by creating the following flow:
   GenerateFlowFile -> success -> FetchFTP
   FetchFTP -> comms.failure, not.found, permission.denied -> LogAttribute
   FetchFTP -> success -> funnel
   LogAttribute -> success -> funnel
   
   Configure FetchFTP with a bogus Hostname, Username, and Remote File
   Ran the flow the saw the attribute 'failure' get logged:
   
   Key: 'failure'
   Value: 'Failed to fetch content for 
StandardFlowFileRecord[uuid=a363e000-3c24-429a-8c0b-4e2d12a37345,claim=,offset=0,name=a363e000-3c24-429a-8c0b-4e2d12a37345,size=0]
 from filename file1 on remote host somehost:21 due to 
org.apache.nifi.processors.standard.socket.ClientConnectException: FTP Client 
connection failed [somehost:21]; routing to comms.failure'
   
   
   ### Build
   
   - [x] Build completed using `mvn clean install -P contrib-check`
 - [x] JDK 21
   
   ### Licensing
   
   - [ ] New dependencies are compatible with the [Apache License 
2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License 
Policy](https://www.apache.org/legal/resolved.html)
   - [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` 
files
   
   ### Documentation
   
   - [x] Documentation formatting appears as expected in rendered files
   


-- 
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: issues-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org