Uploaded image for project: 'Apache NiFi'
  1. Apache NiFi
  2. NIFI-3600

Improve logging and relationship routing for failures in DeleteHDFS

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.2.0
    • Component/s: None
    • Labels:
      None

      Description

      Currently if DeleteHDFS attempts to delete a file and does not have permissions to delete that file the flowfile session is rolled back and penalized. This causes the incoming connection into DeleteHDFS to build up over time and not process at all.

      This enhancement would add a new relationship to DeleteHDFS where files failures to delete a file strictly tied to invalid permissions would be routed given the end user the opportunity to react to those invalid permissions through another approach.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user jdye64 opened a pull request:

          https://github.com/apache/nifi/pull/1595

          NIFI-3600 Improve logging and relationship routing for failures in De…

          …leteHDFS

          Thank you for submitting a contribution to Apache NiFi.

          In order to streamline the review of the contribution we ask you
          to ensure the following steps have been taken:

              1. For all changes:
          • [ ] Is there a JIRA ticket associated with this PR? Is it referenced
            in the commit message?
          • [ ] Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
          • [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?
          • [ ] Is your initial contribution a single, squashed commit?
              1. For code changes:
          • [ ] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
          • [ ] Have you written or updated unit tests to verify your changes?
          • [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
          • [ ] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
          • [ ] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
          • [ ] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?
              1. For documentation related changes:
          • [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
              1. Note:
                Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/jdye64/nifi NIFI-3600

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/nifi/pull/1595.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #1595



          Show
          githubbot ASF GitHub Bot added a comment - GitHub user jdye64 opened a pull request: https://github.com/apache/nifi/pull/1595 NIFI-3600 Improve logging and relationship routing for failures in De… …leteHDFS Thank you for submitting a contribution to Apache NiFi. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: For all changes: [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? [ ] Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character. [ ] Has your PR been rebased against the latest commit within the target branch (typically master)? [ ] Is your initial contribution a single, squashed commit? For code changes: [ ] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder? [ ] Have you written or updated unit tests to verify your changes? [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0] ( http://www.apache.org/legal/resolved.html#category-a)? [ ] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly? [ ] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly? [ ] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties? For documentation related changes: [ ] Have you ensured that format looks appropriate for the output in which it is rendered? Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jdye64/nifi NIFI-3600 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/nifi/pull/1595.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1595
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user trixpan commented on the issue:

          https://github.com/apache/nifi/pull/1595

          @jdye64 can you please solve the conflicts?

          Cheers!

          Show
          githubbot ASF GitHub Bot added a comment - Github user trixpan commented on the issue: https://github.com/apache/nifi/pull/1595 @jdye64 can you please solve the conflicts? Cheers!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jdye64 commented on the issue:

          https://github.com/apache/nifi/pull/1595

          @trixpan I tried talking it out but that didn't seem to solve the conflict?? Will try more forceful procedures when I get off the road and near a computer thanks for taking a look at this!

          Show
          githubbot ASF GitHub Bot added a comment - Github user jdye64 commented on the issue: https://github.com/apache/nifi/pull/1595 @trixpan I tried talking it out but that didn't seem to solve the conflict?? Will try more forceful procedures when I get off the road and near a computer thanks for taking a look at this!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user trixpan commented on the issue:

          https://github.com/apache/nifi/pull/1595

          :rofl:

          Show
          githubbot ASF GitHub Bot added a comment - Github user trixpan commented on the issue: https://github.com/apache/nifi/pull/1595 :rofl:
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jdye64 commented on the issue:

          https://github.com/apache/nifi/pull/1595

          @trixpan refactoring due to NIFI-3204 #1561 which caused some conflicts

          Show
          githubbot ASF GitHub Bot added a comment - Github user jdye64 commented on the issue: https://github.com/apache/nifi/pull/1595 @trixpan refactoring due to NIFI-3204 #1561 which caused some conflicts
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user trixpan commented on the issue:

          https://github.com/apache/nifi/pull/1595

          Glad you managed to get the lines out of their conflict... 😀 will review

          Show
          githubbot ASF GitHub Bot added a comment - Github user trixpan commented on the issue: https://github.com/apache/nifi/pull/1595 Glad you managed to get the lines out of their conflict... 😀 will review
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user trixpan commented on the issue:

          https://github.com/apache/nifi/pull/1595

          From a first look the code seems ok but I have a quick question:

          Do we truly need a permission denied relationship? It seems like a deviation from the general nature of failure relationships (generally they are nearly all encompassing).

          Perhaps we could add the failure reason to an attribute and route to failure?

          Reason I ask is that I like the strong similarity between the *HDFS, the *File and *S3 processors and I suspect this change would introduce change that will move some of them appart?

          Show
          githubbot ASF GitHub Bot added a comment - Github user trixpan commented on the issue: https://github.com/apache/nifi/pull/1595 From a first look the code seems ok but I have a quick question: Do we truly need a permission denied relationship? It seems like a deviation from the general nature of failure relationships (generally they are nearly all encompassing). Perhaps we could add the failure reason to an attribute and route to failure? Reason I ask is that I like the strong similarity between the *HDFS, the *File and *S3 processors and I suspect this change would introduce change that will move some of them appart?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jdye64 commented on the issue:

          https://github.com/apache/nifi/pull/1595

          My reasoning for adding the extra relationship was for end users to understand that certain directories or files had improper permissions and have a visible way to see that with the extra relationship. However your right we could just add something like a "error.code" and "error.message" attributes and route to failure. I would like the "error.code" for sure however since that would be easy to route against while error.message might contain different messages from the exceptions when dependency versions change

          Show
          githubbot ASF GitHub Bot added a comment - Github user jdye64 commented on the issue: https://github.com/apache/nifi/pull/1595 My reasoning for adding the extra relationship was for end users to understand that certain directories or files had improper permissions and have a visible way to see that with the extra relationship. However your right we could just add something like a "error.code" and "error.message" attributes and route to failure. I would like the "error.code" for sure however since that would be easy to route against while error.message might contain different messages from the exceptions when dependency versions change
          Hide
          mattyb149 Matt Burgess added a comment -

          Attached patch with unit test

          Show
          mattyb149 Matt Burgess added a comment - Attached patch with unit test
          Hide
          bende Bryan Bende added a comment -

          Looks this could be included in 1.2.0 now that we are doing an RC2...

          My only comment is that I would suggest we don't put the error code since we don't really know for sure if the IOException was for a permissions problem.

          With the above change I am a +1.

          Show
          bende Bryan Bende added a comment - Looks this could be included in 1.2.0 now that we are doing an RC2... My only comment is that I would suggest we don't put the error code since we don't really know for sure if the IOException was for a permissions problem. With the above change I am a +1.
          Hide
          mattyb149 Matt Burgess added a comment -

          I agree, I will add a unit test and remove that attribute (for now), when/if we can tell the difference between IOExceptions, we can add that attribute back in.

          Show
          mattyb149 Matt Burgess added a comment - I agree, I will add a unit test and remove that attribute (for now), when/if we can tell the difference between IOExceptions, we can add that attribute back in.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user mattyb149 commented on the issue:

          https://github.com/apache/nifi/pull/1595

          Reviewer consensus (see the Jira case for details) is to exclude the hdfs.error.code for now, rather than possibly reporting an incorrect cause of an IOException. Once we can tell the difference, we can add that attribute back in. In that state, @bbende and I are a +1

          Adding a unit test, removing the attribute, and merging to master. Thanks much!

          Show
          githubbot ASF GitHub Bot added a comment - Github user mattyb149 commented on the issue: https://github.com/apache/nifi/pull/1595 Reviewer consensus (see the Jira case for details) is to exclude the hdfs.error.code for now, rather than possibly reporting an incorrect cause of an IOException. Once we can tell the difference, we can add that attribute back in. In that state, @bbende and I are a +1 Adding a unit test, removing the attribute, and merging to master. Thanks much!
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 5f65b2561a25a10289a1a10c5325042ed988b105 in nifi's branch refs/heads/master from Jeremy Dyer
          [ https://git-wip-us.apache.org/repos/asf?p=nifi.git;h=5f65b25 ]

          NIFI-3600 Improve logging and relationship routing for failures in DeleteHDFS

          realized that the session should be cloned here because its inside a for loop and the original flow file would be transferred but not be the latest flow file if an error occurred in the for loop

          @trixpan at a high level what do you think about this approach?

          NIFI-3600: Added unit test

          NIFI-3600: Removed the hdfs.error.code attribute
          Signed-off-by: Matt Burgess <mattyb149@apache.org>

          This closes #1595

          Show
          jira-bot ASF subversion and git services added a comment - Commit 5f65b2561a25a10289a1a10c5325042ed988b105 in nifi's branch refs/heads/master from Jeremy Dyer [ https://git-wip-us.apache.org/repos/asf?p=nifi.git;h=5f65b25 ] NIFI-3600 Improve logging and relationship routing for failures in DeleteHDFS realized that the session should be cloned here because its inside a for loop and the original flow file would be transferred but not be the latest flow file if an error occurred in the for loop @trixpan at a high level what do you think about this approach? NIFI-3600 : Added unit test NIFI-3600 : Removed the hdfs.error.code attribute Signed-off-by: Matt Burgess <mattyb149@apache.org> This closes #1595
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 5f65b2561a25a10289a1a10c5325042ed988b105 in nifi's branch refs/heads/master from Jeremy Dyer
          [ https://git-wip-us.apache.org/repos/asf?p=nifi.git;h=5f65b25 ]

          NIFI-3600 Improve logging and relationship routing for failures in DeleteHDFS

          realized that the session should be cloned here because its inside a for loop and the original flow file would be transferred but not be the latest flow file if an error occurred in the for loop

          @trixpan at a high level what do you think about this approach?

          NIFI-3600: Added unit test

          NIFI-3600: Removed the hdfs.error.code attribute
          Signed-off-by: Matt Burgess <mattyb149@apache.org>

          This closes #1595

          Show
          jira-bot ASF subversion and git services added a comment - Commit 5f65b2561a25a10289a1a10c5325042ed988b105 in nifi's branch refs/heads/master from Jeremy Dyer [ https://git-wip-us.apache.org/repos/asf?p=nifi.git;h=5f65b25 ] NIFI-3600 Improve logging and relationship routing for failures in DeleteHDFS realized that the session should be cloned here because its inside a for loop and the original flow file would be transferred but not be the latest flow file if an error occurred in the for loop @trixpan at a high level what do you think about this approach? NIFI-3600 : Added unit test NIFI-3600 : Removed the hdfs.error.code attribute Signed-off-by: Matt Burgess <mattyb149@apache.org> This closes #1595
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 5f65b2561a25a10289a1a10c5325042ed988b105 in nifi's branch refs/heads/master from Jeremy Dyer
          [ https://git-wip-us.apache.org/repos/asf?p=nifi.git;h=5f65b25 ]

          NIFI-3600 Improve logging and relationship routing for failures in DeleteHDFS

          realized that the session should be cloned here because its inside a for loop and the original flow file would be transferred but not be the latest flow file if an error occurred in the for loop

          @trixpan at a high level what do you think about this approach?

          NIFI-3600: Added unit test

          NIFI-3600: Removed the hdfs.error.code attribute
          Signed-off-by: Matt Burgess <mattyb149@apache.org>

          This closes #1595

          Show
          jira-bot ASF subversion and git services added a comment - Commit 5f65b2561a25a10289a1a10c5325042ed988b105 in nifi's branch refs/heads/master from Jeremy Dyer [ https://git-wip-us.apache.org/repos/asf?p=nifi.git;h=5f65b25 ] NIFI-3600 Improve logging and relationship routing for failures in DeleteHDFS realized that the session should be cloned here because its inside a for loop and the original flow file would be transferred but not be the latest flow file if an error occurred in the for loop @trixpan at a high level what do you think about this approach? NIFI-3600 : Added unit test NIFI-3600 : Removed the hdfs.error.code attribute Signed-off-by: Matt Burgess <mattyb149@apache.org> This closes #1595
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/nifi/pull/1595

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/nifi/pull/1595

            People

            • Assignee:
              jeremy.dyer Jeremy Dyer
              Reporter:
              jeremy.dyer Jeremy Dyer
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development