[
https://issues.apache.org/jira/browse/NIFI-1086?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15004469#comment-15004469
]
Joseph Percivall commented on NIFI-1086:
----------------------------------------
Thanks for the review! I will address each in order:
Added the annotation
I'm not sure if your IDE made import into a wild card import but I don't have
that locally or in the patch
Arbitrary methods are things that would be unique to users. For example one
user might be trying to something fancy and have their server set up to respond
to a "FETCH" request instead of "GET". Arbitrary Http methods are valid.
Agreed in regards to capitalizing and underscoring 349 and 350 - fixing
I thought of this at first but decided against it due to the SSL context being
created and added only if the url has HTTPS in it (and URL supports expression
language) but I added a unit test where it creates and adds the ssl context and
then send an HTTP request. It appears OkHttp will intelligently use the context
only when it needs it. I changed it so the OkHttp client gets created in the
OnScheduled method (can't make it static because other InvokeHttp processors
could have other configuration that conflict).
Agreed with using asBoolean, also a couple other places this applies. I think I
just forgot that you could do that - fixing
That is a very good point. I changed it up to use TeeInputStream so there will
only be the MaxLength in the attribute and that will get populated as the
stream flows to the response contents.
I changed how it's done with the point above
Agreed, I changed it to get the charset from the media type like OkHttp does
I actually tried this new plug-in called FindBugs to figure out where I could
improve and it had suggested I do it this way. If I don't then the
RunTimeException will be caught in the catch Exception block. That being said I
don't think there is significant gains we can make with going more granular at
this time so I'm just gonna remove that. I agree with the stack trace though.
I think I left it in after testing
Ah I thought I had the media type in there from the original InvokeHttp you're
right. I changed it to match how PostHttp grabs the content type from the
flowfile mime-type
using as Boolean - fixing
The only place that was throwing an exception was in isSuccess() which ended up
doing the same check many times. I changed it so it does the check once right
after getting a response and removed it from isSuccess()
Posting a patch
> 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)
