You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/03/19 14:11:27 UTC
[GitHub] [nifi] adenes opened a new pull request #4917: NIFI-8346 PutAzureBlobStorage doesn't route to failure despite the exception during upload
adenes opened a new pull request #4917:
URL: https://github.com/apache/nifi/pull/4917
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:
- [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-XXXX** where XXXX 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 `main`)?
- [x] 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:
- [x] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [x] Have you written or updated unit tests to verify your changes?
- [x] Have you verified that the full build is successful on JDK 8?
- [x] 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](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 GitHub Actions CI for build issues and submit an update to your PR as soon as possible.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] adenes commented on pull request #4917: NIFI-8346 PutAzureBlobStorage doesn't route to failure despite the exception during upload
Posted by GitBox <gi...@apache.org>.
adenes commented on pull request #4917:
URL: https://github.com/apache/nifi/pull/4917#issuecomment-803100541
> @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.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] adenes commented on a change in pull request #4917: NIFI-8346 PutAzureBlobStorage doesn't route to failure despite the exception during upload
Posted by GitBox <gi...@apache.org>.
adenes commented on a change in pull request #4917:
URL: https://github.com/apache/nifi/pull/4917#discussion_r599502388
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureBlobStorage.java
##########
@@ -141,14 +142,14 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
}
try {
- blob.upload(in, -1, null, null, operationContext);
+ uploadBlob(blob, operationContext, in);
BlobProperties properties = blob.getProperties();
attributes.put("azure.container", containerName);
attributes.put("azure.primaryUri", blob.getSnapshotQualifiedUri().toString());
attributes.put("azure.etag", properties.getEtag());
attributes.put("azure.length", String.valueOf(length));
attributes.put("azure.timestamp", String.valueOf(properties.getLastModified()));
- } catch (StorageException | URISyntaxException e) {
+ } catch (StorageException | URISyntaxException | IOException e) {
storedException.set(e);
Review comment:
@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](https://github.com/apache/nifi/blob/c8b7bf9b427b1a2026d03c4d3438a5238da8acd2/nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureBlobStorage.java#L167) 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.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] jfrazee commented on a change in pull request #4917: NIFI-8346 PutAzureBlobStorage doesn't route to failure despite the exception during upload
Posted by GitBox <gi...@apache.org>.
jfrazee commented on a change in pull request #4917:
URL: https://github.com/apache/nifi/pull/4917#discussion_r599122961
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureBlobStorage.java
##########
@@ -141,14 +142,14 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
}
try {
- blob.upload(in, -1, null, null, operationContext);
+ uploadBlob(blob, operationContext, in);
BlobProperties properties = blob.getProperties();
attributes.put("azure.container", containerName);
attributes.put("azure.primaryUri", blob.getSnapshotQualifiedUri().toString());
attributes.put("azure.etag", properties.getEtag());
attributes.put("azure.length", String.valueOf(length));
attributes.put("azure.timestamp", String.valueOf(properties.getLastModified()));
- } catch (StorageException | URISyntaxException e) {
+ } catch (StorageException | URISyntaxException | IOException e) {
storedException.set(e);
throw new IOException(e);
Review comment:
I should have caught this before but we probably shouldn't double-wrap this.
```suggestion
throw (e instanceof IOException) ? (IOException) e : new IOException(e);
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] adenes commented on pull request #4917: NIFI-8346 PutAzureBlobStorage doesn't route to failure despite the exception during upload
Posted by GitBox <gi...@apache.org>.
adenes commented on pull request #4917:
URL: https://github.com/apache/nifi/pull/4917#issuecomment-804006166
@jfrazee , I have added a new test case to test the invalid credentials scenario.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] jfrazee commented on pull request #4917: NIFI-8346 PutAzureBlobStorage doesn't route to failure despite the exception during upload
Posted by GitBox <gi...@apache.org>.
jfrazee commented on pull request #4917:
URL: https://github.com/apache/nifi/pull/4917#issuecomment-803098978
@adenes Yeah, that's basically the flow I ended up running.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] jfrazee commented on a change in pull request #4917: NIFI-8346 PutAzureBlobStorage doesn't route to failure despite the exception during upload
Posted by GitBox <gi...@apache.org>.
jfrazee commented on a change in pull request #4917:
URL: https://github.com/apache/nifi/pull/4917#discussion_r599607297
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureBlobStorage.java
##########
@@ -141,14 +142,14 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
}
try {
- blob.upload(in, -1, null, null, operationContext);
+ uploadBlob(blob, operationContext, in);
BlobProperties properties = blob.getProperties();
attributes.put("azure.container", containerName);
attributes.put("azure.primaryUri", blob.getSnapshotQualifiedUri().toString());
attributes.put("azure.etag", properties.getEtag());
attributes.put("azure.length", String.valueOf(length));
attributes.put("azure.timestamp", String.valueOf(properties.getLastModified()));
- } catch (StorageException | URISyntaxException e) {
+ } catch (StorageException | URISyntaxException | IOException e) {
storedException.set(e);
Review comment:
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.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4917: NIFI-8346 PutAzureBlobStorage doesn't route to failure despite the exception during upload
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4917:
URL: https://github.com/apache/nifi/pull/4917#discussion_r599236166
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureBlobStorage.java
##########
@@ -141,14 +142,14 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
}
try {
- blob.upload(in, -1, null, null, operationContext);
+ uploadBlob(blob, operationContext, in);
BlobProperties properties = blob.getProperties();
attributes.put("azure.container", containerName);
attributes.put("azure.primaryUri", blob.getSnapshotQualifiedUri().toString());
attributes.put("azure.etag", properties.getEtag());
attributes.put("azure.length", String.valueOf(length));
attributes.put("azure.timestamp", String.valueOf(properties.getLastModified()));
- } catch (StorageException | URISyntaxException e) {
+ } catch (StorageException | URISyntaxException | IOException e) {
storedException.set(e);
Review comment:
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.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] adenes commented on pull request #4917: NIFI-8346 PutAzureBlobStorage doesn't route to failure despite the exception during upload
Posted by GitBox <gi...@apache.org>.
adenes commented on pull request #4917:
URL: https://github.com/apache/nifi/pull/4917#issuecomment-803097523
@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`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] jfrazee commented on pull request #4917: NIFI-8346 PutAzureBlobStorage doesn't route to failure despite the exception during upload
Posted by GitBox <gi...@apache.org>.
jfrazee commented on pull request #4917:
URL: https://github.com/apache/nifi/pull/4917#issuecomment-802928418
@adenes Thanks for catching this. Is there an easy way to manually reproduce?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] jfrazee closed pull request #4917: NIFI-8346 PutAzureBlobStorage doesn't route to failure despite the exception during upload
Posted by GitBox <gi...@apache.org>.
jfrazee closed pull request #4917:
URL: https://github.com/apache/nifi/pull/4917
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] adenes commented on a change in pull request #4917: NIFI-8346 PutAzureBlobStorage doesn't route to failure despite the exception during upload
Posted by GitBox <gi...@apache.org>.
adenes commented on a change in pull request #4917:
URL: https://github.com/apache/nifi/pull/4917#discussion_r599497562
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureBlobStorage.java
##########
@@ -141,14 +142,14 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
}
try {
- blob.upload(in, -1, null, null, operationContext);
+ uploadBlob(blob, operationContext, in);
BlobProperties properties = blob.getProperties();
attributes.put("azure.container", containerName);
attributes.put("azure.primaryUri", blob.getSnapshotQualifiedUri().toString());
attributes.put("azure.etag", properties.getEtag());
attributes.put("azure.length", String.valueOf(length));
attributes.put("azure.timestamp", String.valueOf(properties.getLastModified()));
- } catch (StorageException | URISyntaxException e) {
+ } catch (StorageException | URISyntaxException | IOException e) {
storedException.set(e);
throw new IOException(e);
Review comment:
Thanks, updated
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4917: NIFI-8346 PutAzureBlobStorage doesn't route to failure despite the exception during upload
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4917:
URL: https://github.com/apache/nifi/pull/4917#discussion_r599619151
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureBlobStorage.java
##########
@@ -141,14 +142,14 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
}
try {
- blob.upload(in, -1, null, null, operationContext);
+ uploadBlob(blob, operationContext, in);
BlobProperties properties = blob.getProperties();
attributes.put("azure.container", containerName);
attributes.put("azure.primaryUri", blob.getSnapshotQualifiedUri().toString());
attributes.put("azure.etag", properties.getEtag());
attributes.put("azure.length", String.valueOf(length));
attributes.put("azure.timestamp", String.valueOf(properties.getLastModified()));
- } catch (StorageException | URISyntaxException e) {
+ } catch (StorageException | URISyntaxException | IOException e) {
storedException.set(e);
Review comment:
Thanks for the background @jfrazee, leaving it as-is for now 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4917: NIFI-8346 PutAzureBlobStorage doesn't route to failure despite the exception during upload
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4917:
URL: https://github.com/apache/nifi/pull/4917#discussion_r599583677
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureBlobStorage.java
##########
@@ -141,14 +142,14 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
}
try {
- blob.upload(in, -1, null, null, operationContext);
+ uploadBlob(blob, operationContext, in);
BlobProperties properties = blob.getProperties();
attributes.put("azure.container", containerName);
attributes.put("azure.primaryUri", blob.getSnapshotQualifiedUri().toString());
attributes.put("azure.etag", properties.getEtag());
attributes.put("azure.length", String.valueOf(length));
attributes.put("azure.timestamp", String.valueOf(properties.getLastModified()));
- } catch (StorageException | URISyntaxException e) {
+ } catch (StorageException | URISyntaxException | IOException e) {
storedException.set(e);
Review comment:
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.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org