You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nifi.apache.org by pvillard31 <gi...@git.apache.org> on 2016/04/26 22:04:25 UTC
[GitHub] nifi pull request: NIFI-1814 Extended caught exceptions
GitHub user pvillard31 opened a pull request:
https://github.com/apache/nifi/pull/382
NIFI-1814 Extended caught exceptions
In some cases, when trying to
session.exportTo(flowFile, response.getOutputStream());
It seems that response.getOutputStream() has been somehow modified by
Jetty resulting to a FlowFileAccessException encapsulating the
EofException of Jetty.
Extended the exceptions caught and added a unit test to reproduce this
case.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/pvillard31/nifi NIFI-1814
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/382.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 #382
----
commit 0177d8cf87abbe7cea100900a7a8412b212f3f62
Author: Pierre Villard <pi...@gmail.com>
Date: 2016-04-26T20:03:59Z
NIFI-1814 Extended caught exceptions
In some cases, when trying to
session.exportTo(flowFile, response.getOutputStream());
It seems that response.getOutputStream() has been somehow modified by
Jetty resulting to a FlowFileAccessException encapsulating the
EofException of Jetty.
Extended the exceptions caught and added a unit test to reproduce this
case.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1814 Extended caught exceptions
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/382#discussion_r62359651
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpResponse.java ---
@@ -155,9 +155,9 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
try {
session.exportTo(flowFile, response.getOutputStream());
response.flushBuffer();
- } catch (final IOException ioe) {
+ } catch (final Exception e) {
--- End diff --
Yes I agree with this approach. I though Exception was justified by the fact that if the exception is not caught, the request is not removed from the session and will be re-queued (and yield the processor) which can be blocking.
All things considered, it's cleaner to catch individual exceptions and I'll update the PR as suggested.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1814 Extended caught exceptions
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi/pull/382#discussion_r62359332
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpResponse.java ---
@@ -155,9 +155,9 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
try {
session.exportTo(flowFile, response.getOutputStream());
response.flushBuffer();
- } catch (final IOException ioe) {
+ } catch (final Exception e) {
--- End diff --
Thinking through this a bit more, catching Exception is probably appropriate. Typically you want to not blanket catch when creating framework level code but this needs to be hardened to anything that may happen when responding.
I am good with how it is now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1814 Extended caught exceptions
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/382#discussion_r62359879
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpResponse.java ---
@@ -155,9 +155,9 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
try {
session.exportTo(flowFile, response.getOutputStream());
response.flushBuffer();
- } catch (final IOException ioe) {
+ } catch (final Exception e) {
--- End diff --
OK :) I don't change anything then!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1814 Extended caught exceptions
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/382
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1814 Extended caught exceptions
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on the pull request:
https://github.com/apache/nifi/pull/382#issuecomment-217524668
Once the contrib check is fixed I'm a +1.
Go ahead and squash it into one commit and I'll merge it into 0.x and 1.0 branches
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1814 Extended caught exceptions
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi/pull/382#discussion_r62371933
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpResponse.java ---
@@ -155,9 +155,9 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
try {
session.exportTo(flowFile, response.getOutputStream());
response.flushBuffer();
- } catch (final IOException ioe) {
+ } catch (final Exception e) {
--- End diff --
You removed "IOException" from being used but didn't remove the import causing it to fail contrib check.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1814 Extended caught exceptions
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi/pull/382#discussion_r62336974
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpResponse.java ---
@@ -155,9 +155,9 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
try {
session.exportTo(flowFile, response.getOutputStream());
response.flushBuffer();
- } catch (final IOException ioe) {
+ } catch (final Exception e) {
--- End diff --
Instead of changing this to blanket catching "Exception". Could another catch block that handles "FlowFileAccessException" be added, or just add it as another exception type caught in this block?
Blanket catching Exception is considered bad form so unless there is a specific need for it, I tend to stay away from it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---