[GitHub] nifi pull request #2971: NIFI-5557: handling expired ticket by rollback and ...
Github user asfgit closed the pull request at: https://github.com/apache/nifi/pull/2971 ---
[GitHub] nifi pull request #2971: NIFI-5557: handling expired ticket by rollback and ...
Github user ekovacs commented on a diff in the pull request: https://github.com/apache/nifi/pull/2971#discussion_r220236670 --- Diff: nifi-nar-bundles/nifi-hadoop-bundle/nifi-hdfs-processors/src/main/java/org/apache/nifi/processors/hadoop/PutHDFS.java --- @@ -389,16 +380,24 @@ public void process(InputStream in) throws IOException { session.transfer(putFlowFile, REL_SUCCESS); } catch (final Throwable t) { -if (tempDotCopyFile != null) { -try { -hdfs.delete(tempDotCopyFile, false); -} catch (Exception e) { -getLogger().error("Unable to remove temporary file {} due to {}", new Object[]{tempDotCopyFile, e}); -} + Optional causeOptional = findCause(t, GSSException.class, gsse -> GSSException.NO_CRED == gsse.getMajor()); +if (causeOptional.isPresent()) { + getLogger().warn(String.format("An error occured while connecting to HDFS. " --- End diff -- indeed. ---
[GitHub] nifi pull request #2971: NIFI-5557: handling expired ticket by rollback and ...
Github user ekovacs commented on a diff in the pull request: https://github.com/apache/nifi/pull/2971#discussion_r220236629 --- Diff: nifi-nar-bundles/nifi-hadoop-bundle/nifi-hdfs-processors/src/main/java/org/apache/nifi/processors/hadoop/PutHDFS.java --- @@ -389,16 +380,24 @@ public void process(InputStream in) throws IOException { session.transfer(putFlowFile, REL_SUCCESS); } catch (final Throwable t) { -if (tempDotCopyFile != null) { -try { -hdfs.delete(tempDotCopyFile, false); -} catch (Exception e) { -getLogger().error("Unable to remove temporary file {} due to {}", new Object[]{tempDotCopyFile, e}); -} + Optional causeOptional = findCause(t, GSSException.class, gsse -> GSSException.NO_CRED == gsse.getMajor()); --- End diff -- yes. it makes sense. ---
[GitHub] nifi pull request #2971: NIFI-5557: handling expired ticket by rollback and ...
Github user jtstorck commented on a diff in the pull request: https://github.com/apache/nifi/pull/2971#discussion_r220204503 --- Diff: nifi-nar-bundles/nifi-hadoop-bundle/nifi-hdfs-processors/src/main/java/org/apache/nifi/processors/hadoop/PutHDFS.java --- @@ -389,16 +380,24 @@ public void process(InputStream in) throws IOException { session.transfer(putFlowFile, REL_SUCCESS); } catch (final Throwable t) { -if (tempDotCopyFile != null) { -try { -hdfs.delete(tempDotCopyFile, false); -} catch (Exception e) { -getLogger().error("Unable to remove temporary file {} due to {}", new Object[]{tempDotCopyFile, e}); -} + Optional causeOptional = findCause(t, GSSException.class, gsse -> GSSException.NO_CRED == gsse.getMajor()); --- End diff -- My previous comment was a bit ambiguous, I apologize. Having this logic in this catch for all Throwables is fine, but you could move this bit into a separate catch(IOException e) block of this try/catch. ---
[GitHub] nifi pull request #2971: NIFI-5557: handling expired ticket by rollback and ...
Github user jtstorck commented on a diff in the pull request: https://github.com/apache/nifi/pull/2971#discussion_r220206853 --- Diff: nifi-nar-bundles/nifi-hadoop-bundle/nifi-hdfs-processors/src/main/java/org/apache/nifi/processors/hadoop/PutHDFS.java --- @@ -389,16 +380,24 @@ public void process(InputStream in) throws IOException { session.transfer(putFlowFile, REL_SUCCESS); } catch (final Throwable t) { -if (tempDotCopyFile != null) { -try { -hdfs.delete(tempDotCopyFile, false); -} catch (Exception e) { -getLogger().error("Unable to remove temporary file {} due to {}", new Object[]{tempDotCopyFile, e}); -} + Optional causeOptional = findCause(t, GSSException.class, gsse -> GSSException.NO_CRED == gsse.getMajor()); +if (causeOptional.isPresent()) { + getLogger().warn(String.format("An error occured while connecting to HDFS. " --- End diff -- This could be changed to: ```java getLogger().warn("An error occured while connecting to HDFS. Rolling back session, and penalizing flow file {}", new Object[] {putFlowFile.getAttribute(CoreAttributes.UUID.key()), causeOptional.get()}); ``` ---
[GitHub] nifi pull request #2971: NIFI-5557: handling expired ticket by rollback and ...
Github user jtstorck commented on a diff in the pull request: https://github.com/apache/nifi/pull/2971#discussion_r219380028 --- Diff: nifi-nar-bundles/nifi-hadoop-bundle/nifi-hdfs-processors/src/main/java/org/apache/nifi/processors/hadoop/PutHDFS.java --- @@ -266,6 +271,16 @@ public Object run() { throw new IOException(configuredRootDirPath.toString() + " could not be created"); } changeOwner(context, hdfs, configuredRootDirPath, flowFile); +} catch (IOException e) { + boolean tgtExpired = hasCause(e, GSSException.class, gsse -> GSSException.NO_CRED == gsse.getMajor()); + if (tgtExpired) { +getLogger().error(String.format("An error occured while connecting to HDFS. Rolling back session, and penalizing flow file %s", --- End diff -- The exception be logged here, in addition to the flowfile UUID. It might be useful to have the stack trace and exception class available in the log, and we shouldn't suppress/omit the actual GSSException from the logging. It might also be a good idea to log this at the "warn" level, so that the user can choose to not have these show as bulletins on the processor in the UI. Since the flowfile is being rolled back, and hadoop-client will implicitly acquire a new ticket, I don't think this should show as an error. @mcgilman, @bbende, do either of you have a preference here? ---
[GitHub] nifi pull request #2971: NIFI-5557: handling expired ticket by rollback and ...
Github user jtstorck commented on a diff in the pull request: https://github.com/apache/nifi/pull/2971#discussion_r219377632 --- Diff: nifi-nar-bundles/nifi-hadoop-bundle/nifi-hdfs-processors/src/main/java/org/apache/nifi/processors/hadoop/PutHDFS.java --- @@ -266,6 +271,16 @@ public Object run() { throw new IOException(configuredRootDirPath.toString() + " could not be created"); } changeOwner(context, hdfs, configuredRootDirPath, flowFile); +} catch (IOException e) { --- End diff -- Thanks for changing this to use GSSException.getMajor(). I haven't tested a ticket expiration occurring during the execution of a call to ugi.doAs (as opposed to the ticket having expired before ugi.doAs is invoked), but I think it would be a good idea to move this catch block to the top level try/catch block of the PrivelegedExceptionAction passed to ugi.doAs(). ---
[GitHub] nifi pull request #2971: NIFI-5557: handling expired ticket by rollback and ...
Github user jtstorck commented on a diff in the pull request: https://github.com/apache/nifi/pull/2971#discussion_r216031690 --- Diff: nifi-nar-bundles/nifi-hadoop-bundle/nifi-hdfs-processors/src/main/java/org/apache/nifi/processors/hadoop/PutHDFS.java --- @@ -269,13 +272,15 @@ public Object run() { } changeOwner(context, hdfs, configuredRootDirPath, flowFile); } catch (IOException e) { -if (!Strings.isNullOrEmpty(e.getMessage()) && e.getMessage().contains(String.format("Couldn't setup connection for %s", ugi.getUserName( { - getLogger().error(String.format("An error occured while connecting to HDFS. Rolling back session, and penalizing flowfile %s", - flowFile.getAttribute(CoreAttributes.UUID.key(; - session.rollback(true); -} else { - throw e; -} + boolean tgtExpired = hasCause(e, GSSException.class, gsse -> "Failed to find any Kerberos tgt".equals(gsse.getMinorString())); + if (tgtExpired) { +getLogger().error(String.format("An error occured while connecting to HDFS. Rolling back session, and penalizing flow file %s", + putFlowFile.getAttribute(CoreAttributes.UUID.key(; +session.rollback(true); + } else { +getLogger().error("Failed to access HDFS due to {}", new Object[]{e}); +session.transfer(session.penalize(putFlowFile), REL_FAILURE); --- End diff -- @ekovacs I don't think we need to penalize on the transfer to failure here. ---
[GitHub] nifi pull request #2971: NIFI-5557: handling expired ticket by rollback and ...
Github user jtstorck commented on a diff in the pull request: https://github.com/apache/nifi/pull/2971#discussion_r216037639 --- Diff: nifi-nar-bundles/nifi-hadoop-bundle/nifi-hdfs-processors/src/main/java/org/apache/nifi/processors/hadoop/PutHDFS.java --- @@ -269,13 +272,15 @@ public Object run() { } changeOwner(context, hdfs, configuredRootDirPath, flowFile); } catch (IOException e) { -if (!Strings.isNullOrEmpty(e.getMessage()) && e.getMessage().contains(String.format("Couldn't setup connection for %s", ugi.getUserName( { - getLogger().error(String.format("An error occured while connecting to HDFS. Rolling back session, and penalizing flowfile %s", - flowFile.getAttribute(CoreAttributes.UUID.key(; - session.rollback(true); -} else { - throw e; -} + boolean tgtExpired = hasCause(e, GSSException.class, gsse -> "Failed to find any Kerberos tgt".equals(gsse.getMinorString())); --- End diff -- @ekovacs After seeing the use of getMinorString here, I looked at GSSException, and it looks like there's some error codes that could be used to detect the actual cause, rather than string matching. Are getMajor and getMinor returning ints when these exceptions happen? ---
[GitHub] nifi pull request #2971: NIFI-5557: handling expired ticket by rollback and ...
Github user ekovacs commented on a diff in the pull request: https://github.com/apache/nifi/pull/2971#discussion_r214677358 --- Diff: nifi-nar-bundles/nifi-hadoop-bundle/nifi-hdfs-processors/src/main/java/org/apache/nifi/processors/hadoop/PutHDFS.java --- @@ -266,6 +268,13 @@ public Object run() { throw new IOException(configuredRootDirPath.toString() + " could not be created"); } changeOwner(context, hdfs, configuredRootDirPath, flowFile); +} catch (IOException e) { +if (!Strings.isNullOrEmpty(e.getMessage()) && e.getMessage().contains(String.format("Couldn't setup connection for %s", ugi.getUserName( { --- End diff -- Thanks @jtstorck you are right. I pushed a commit to handle the way you suggested ---
[GitHub] nifi pull request #2971: NIFI-5557: handling expired ticket by rollback and ...
Github user jtstorck commented on a diff in the pull request: https://github.com/apache/nifi/pull/2971#discussion_r214136451 --- Diff: nifi-nar-bundles/nifi-hadoop-bundle/nifi-hdfs-processors/src/main/java/org/apache/nifi/processors/hadoop/PutHDFS.java --- @@ -266,6 +268,13 @@ public Object run() { throw new IOException(configuredRootDirPath.toString() + " could not be created"); } changeOwner(context, hdfs, configuredRootDirPath, flowFile); +} catch (IOException e) { +if (!Strings.isNullOrEmpty(e.getMessage()) && e.getMessage().contains(String.format("Couldn't setup connection for %s", ugi.getUserName( { --- End diff -- @ekovacs I think we should be more selective in this check. I don't think there's a better way to detect this error scenario than string matching at this point, but the exception stack should be inspected to see if you can find the GSSException as the root cause: `Caused by: org.ietf.jgss.GSSException: No valid credentials provided (Mechanism level: Failed to find any Kerberos tgt) ` If you iterate through the causes when PutHDFS encounters an IOException, and see that GSSException, we can do a penalize with a session rollback. Otherwise, we'd want to pass the flowfile to the failure relationship. ---
[GitHub] nifi pull request #2971: NIFI-5557: handling expired ticket by rollback and ...
GitHub user ekovacs opened a pull request: https://github.com/apache/nifi/pull/2971 NIFI-5557: handling expired ticket by rollback and penalization 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: - [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [x] Does your PR title start with NIFI- where is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character. - [x] Has your PR been rebased against the latest commit within the target branch (typically master)? - [x] 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/ekovacs/nifi NIFI-5557 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/nifi/pull/2971.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 #2971 commit 32c42bdd971937cb6904939452420c8754f64287 Author: Endre Zoltan Kovacs Date: 2018-08-28T08:47:59Z NIFI-5557: handling expired ticket by rollback and penalization ---