NIFI-8346 PutAzureBlobStorage doesn't route to failure despite the exception during upload by adenes · Pull Request #4917 · apache/nifi

adenes · 2021-03-19T14:11:15Z

If uploading to an archived blob an IOException is thrown by the azure storage client library. If this happens the flowfile is not transferred to the failure relationship.

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 main)?
Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not squash or use --force when pushing to allow for clean monitoring of changes.

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?
Have you verified that the full build is successful on JDK 8?
Have you verified that the full build is successful on JDK 11?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
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 GitHub Actions CI for build issues and submit an update to your PR as soon as possible.

…ception during upload

jfrazee · 2021-03-19T15:46:53Z

@adenes Thanks for catching this. Is there an easy way to manually reproduce?

jfrazee

@adenes Hey, in testing this I also noticed this fixes similar incorrect behavior when auth fails, so we should definitely get this in.

Would you be able to add a case for one or the other of these to the ITs? The auth one is probably easier since I think you'd just have to add a case re-setting AzureStorageUtils.ACCOUNT_KEY to something invalid.

adenes · 2021-03-19T20:17:05Z

@jfrazee , you can reproduce it with a simple flow (GFF -> PutAzureBlobStorage), just first you need to upload a blob to an Azure storage container and set its access tier to archive. Then if you try to put to that (existing) blob you'll see the IOException: This operation is not permitted on an archived blob

jfrazee · 2021-03-19T20:19:42Z

@adenes Yeah, that's basically the flow I ended up running.

adenes · 2021-03-19T20:21:47Z

@adenes Hey, in testing this I also noticed this fixes similar incorrect behavior when auth fails, so we should definitely get this in.

Would you be able to add a case for one or the other of these to the ITs? The auth one is probably easier since I think you'd just have to add a case re-setting AzureStorageUtils.ACCOUNT_KEY to something invalid.

Good point, thanks, will add a new test case for it.

…ception during upload Add IT test case for PutAzureBlobStorage to test if in case of invalid credentials the FF is routed to failure

adenes · 2021-03-22T11:56:25Z

@jfrazee , I have added a new test case to test the invalid credentials scenario.

jfrazee

Thanks for the update and IT @adenes. I'm +1 on this pending one small thing I should have caught before -- if an IOE gets thrown now it's double-wrapped before it's re-thrown.

jfrazee · 2021-03-22T22:41:09Z

...e-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureBlobStorage.java

-                } catch (StorageException | URISyntaxException e) {
+                } catch (StorageException | URISyntaxException | IOException e) {
                    storedException.set(e);
                    throw new IOException(e);


I should have caught this before but we probably shouldn't double-wrap this.

Suggested change

throw new IOException(e);

throw (e instanceof IOException) ? (IOException) e : new IOException(e);

Thanks, updated

exceptionfactory · 2021-03-23T03:08:38Z

...e-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureBlobStorage.java

                    attributes.put("azure.timestamp", String.valueOf(properties.getLastModified()));
-                } catch (StorageException | URISyntaxException e) {
+                } catch (StorageException | URISyntaxException | IOException e) {
                    storedException.set(e);


Reviewing the larger context of this block, is there a reason to continue maintaining this reference to the storageException? The checked StorageException and URISyntaxException could be wrapped in the existing throw new IOException(). If there is a particular reason to evaluate or log the StorageException, that could be handled here, or in the outer catch block. Either way, it seems like it would be worth untangling the exception handling to avoid this kind of scenario down the road.

@exceptionfactory, thanks for the comment. I have checked the bigger context, and although I agree that the storedException is not needed here, but when the original exception gets propagated to the outer catch block it's wrapped into a ProcessException by the framework.
This means that to get the original exception something like if (e instanceof ProcessException && e.getCause() instanceof IOException && e.getCause().getCause() != null)... would be needed.
In my opinion the current logic is cleaner and easier to understand.

Thanks for the response @adenes. I was actually wondering whether that check for ProcessException in the outer catch block should be there at all. As it stands, anything that throws a ProcessException will keep FlowFiles in the queue instead of routing to failure. Perhaps that is the desired behavior with this particular Processor, but if it is not necessary, the exception handling could be generalized to avoid the instanceof check so that all exceptions result in routing to failure. I am fine with the changes as implemented now, but wanted to raise this question given the current issue with exception handling.

I looked at the blame to see what the original intent is and it's not 100% clear. I have to assume then that REL_FAILURE is intended to be specifically for Blob Storage or Blob Storage operation failures and nothing else.

I think we should leave as-is for now. We really need to get back to #4430 or similar to get this on more current dependencies. We can probably (re)think through the "right" exception handling there.

Thanks for the background @jfrazee, leaving it as-is for now sounds good.

…ception during upload Remove unnecessary wrapping of IOException

…ions This closes apache#4917 Signed-off-by: Joey Frazee <jfrazee@apache.org>

NIFI-8346 PutAzureBlobStorage doesn't route to failure despite the ex…

c89691a

…ception during upload

jfrazee self-requested a review March 19, 2021 15:44

jfrazee reviewed Mar 19, 2021

View reviewed changes

NIFI-8346 PutAzureBlobStorage doesn't route to failure despite the ex…

7060db6

…ception during upload Add IT test case for PutAzureBlobStorage to test if in case of invalid credentials the FF is routed to failure

jfrazee requested changes Mar 22, 2021

View reviewed changes

exceptionfactory reviewed Mar 23, 2021

View reviewed changes

NIFI-8346 PutAzureBlobStorage doesn't route to failure despite the ex…

c8b7bf9

…ception during upload Remove unnecessary wrapping of IOException

jfrazee approved these changes Mar 23, 2021

View reviewed changes

jfrazee closed this in 22fcb59 Mar 23, 2021

krisztina-zsihovszki pushed a commit to krisztina-zsihovszki/nifi that referenced this pull request Jun 28, 2022

NIFI-8346 Route to failure in PutAzureBlobStorage for auth, IO except…

fb24450

…ions This closes apache#4917 Signed-off-by: Joey Frazee <jfrazee@apache.org>

	throw new IOException(e);
	throw (e instanceof IOException) ? (IOException) e : new IOException(e);

Conversation

adenes commented Mar 19, 2021

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

jfrazee commented Mar 19, 2021

Uh oh!

jfrazee left a comment

Choose a reason for hiding this comment

Uh oh!

adenes commented Mar 19, 2021

Uh oh!

jfrazee commented Mar 19, 2021

Uh oh!

adenes commented Mar 19, 2021

Uh oh!

adenes commented Mar 22, 2021

Uh oh!

jfrazee left a comment

Choose a reason for hiding this comment

Uh oh!

jfrazee Mar 22, 2021

Choose a reason for hiding this comment

Uh oh!

adenes Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

exceptionfactory Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

adenes Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

exceptionfactory Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

jfrazee Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

exceptionfactory Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants