[
https://issues.apache.org/jira/browse/NIFI-1086?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15002972#comment-15002972
]
Aldrin Piri commented on NIFI-1086:
-----------------------------------
-1
Build, contrib, and tests all check out.
Issues (lines are approximate, only realized at completion I had been doing the
review after evaluating with TriggerWhenEmpty and other small changes):
InvokeHTTP
The processor does not address NIFI-1009. Without input, the processor does
not bring any actions in. You will need the @TriggerWhenEmpty annotation to
have it work as intended.
Line 61: Avoid the use of wildcards: import
org.apache.nifi.annotation.behavior.*;
PROP_METHOD: While refactoring would be good to standardize on an Enum or
similar for each of the methods. Not sure what you mean by "arbitrary methods"
in this case as these are well defined as per spec. Additionally there is a
missing space in the description.
Lines 349-350: Since refactoring would be good to have these capitalized and
underscore delimited as they are constants
Line 442: OkHttpClient okHttpClient = new OkHttpClient();
Reading through the the client's docs, they seem to indicate that a single
instance can be used in most applications. Is there a reason why one of these
is being done for each invocation? If not, it would be preferable to handle a
single instance
Line 508: if
("true".equalsIgnoreCase(context.getProperty(PROP_ADD_HEADERS_TO_REQUEST).getValue()))
{
Would prefer to use asBoolean() on the property in lieu of statement as listed
Line 521: responseBody = responseHttp.body().string();
If we get a large response and dump this to a string, it could be a big hit on
the heap. Would prefer to use the stream approach as you use later
Additionally, this seems like it could be an NPE. Comment on 553-554 says a
similar responseHttp.body() could potentially be null.
Lines 555-556: This handling is a bit awkward. It seems we would want to check
the responseHttpBody once regardless of destination and use that result
throughout this payload transference process. Minor nitpick, but it would be
nice if responseBody, the variable was indicating it was the string.
Line 561: new
ByteArrayInputStream(responseBody.getBytes(StandardCharsets.UTF_8))
Prefer the Charset, if one was specified by the response as the #string()
method does in your library
Lines 573-576:
This should be cleaned up. No need to catch/rethrow RuntimeException if it is
just to be caught. Can we get more granular and isolated to where this
behavior happens? Part of this is affected by 802 where a spurious IOException
is listed on the method. Avoid printing out the stacktrace as such. Log it if
you think it is beneficial.
Lines 353 & 666-678:
Not sure if this was leftover from testing you were doing during development,
but it doesn't seem like it should be here
Line 704: Use asBoolean
Line 735: Why the IOException in your signature? This gets rethrown several
times up through onTrigger which is making your logic more complex than needed
> Refactor InvokeHttp
> -------------------
>
> Key: NIFI-1086
> URL: https://issues.apache.org/jira/browse/NIFI-1086
> Project: Apache NiFi
> Issue Type: Improvement
> Reporter: Joseph Percivall
> Assignee: Joseph Percivall
> Fix For: 0.4.0
>
> Attachments: NIFI-1086_rebasing_to_master.patch
>
>
> InvokeHttp currently uses Java's HttpUrlConnection, which is lacking in it's
> features and ease-of-use. In order to support all the current InvokeHttp
> pending tickets it's clear that a new underlying library is needed.
> OkHttp looks to be a promising library that focusing on individual
> transactions (as opposed to Apache's HttpClient that focuses more on
> sessions).
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
