NIFI-8120 Added RuntimeException handling on HttpContextMap.complete() #4747
Conversation
exceptionfactory
force-pushed
the
NIFI-8120
branch
2 times, most recently
from
b01291a
to
2e33e6e
Compare
| @@ -204,7 +206,7 @@ public void onTrigger(final ProcessContext context, final ProcessSession session | |||
|
|
|||
| try { | |||
| contextMap.complete(contextIdentifier); | |||
| } catch (final IllegalStateException ise) { | |||
| } catch (final RuntimeException ise) { | |||
There was a problem hiding this comment.
ise seems to be a leftover from the previous version.
Could you please use ce here too like in line 197.
There was a problem hiding this comment.
Renamed to ce as requested.
| @@ -194,7 +192,11 @@ public void onTrigger(final ProcessContext context, final ProcessSession session | |||
| } catch (final ProcessException e) { | |||
| session.transfer(flowFile, REL_FAILURE); | |||
| getLogger().error("Failed to respond to HTTP request for {} due to {}", new Object[]{flowFile, e}); | |||
| contextMap.complete(contextIdentifier); | |||
| try { | |||
There was a problem hiding this comment.
Not a new issue but it could be improved around exception handling:
The typical (by convention) error handling flow is logging first, then sending the FF to FAILURE (in case of an exception in session.transfer()). Like in the last catch block at lines 210-211.
This catch and the others throughout onTrigger() do it vice versa.
I would consider moving the
session.transfer(flowFile, REL_FAILURE);
lines just before the return statements.
There was a problem hiding this comment.
That makes sense, reordered this and other session.transfer() calls to occur after logging.
|
@turcsanyip Thanks for the review and detailed feedback. I pushed an update incorporating your recommendations. |
There was a problem hiding this comment.
+1 LGTM
Merging to main.
@exceptionfactory Thanks for improving the exception handling of this processor and also for the test refactoring / clean-up.
NIFI-8120 Renamed exception variable and reordered log statements This closes apache#4747. Signed-off-by: Peter Turcsanyi <turcsanyi@apache.org>
NIFI-8120 Renamed exception variable and reordered log statements This closes apache#4747. Signed-off-by: Peter Turcsanyi <turcsanyi@apache.org>
Description of PR
NIFI-8120 Adds
RuntimeExceptionhandling for both invocations ofHttpContextMap.complete()inHandleHttpResponseProcessor. PR #1567 for NIFI-3517 introduced callingHttpContextMap.complete()whenHandleHttpResponsefailed to send Flow File contents to the HTTP output stream. In cases where the HTTP client has disconnected, however,HttpContextMap.complete()can throw anIllegalStateException, wrapping ajava.nio.channels.ClosedChannelExceptionwhen attempting to close the server connection. This change includes updated unit tests to confirm that Flow Files will be routed to failure when this situation occurs.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
squashor use--forcewhen pushing to allow for clean monitoring of changes.For code changes:
mvn -Pcontrib-check clean installat the rootnififolder?LICENSEfile, including the mainLICENSEfile undernifi-assembly?NOTICEfile, including the mainNOTICEfile found undernifi-assembly?.displayNamein addition to .name (programmatic access) for each of the new properties?For documentation related changes:
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.