Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/2113
+1
Overall looks good and works as expected. I've built with contrib check and
tested using a vanilla instance. I will squash and merge to master. Thanks
@MikeThomsen.
---
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
@JPercivall I just got done testing it w/ X-Pack for SSL and u/p auth based
on Elastic docs. Everything was in working order there. [Steps if you want to
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/2113
Looks good, was able to do a couple simple queries and aggregations. I
wasn't able to test HTTPS though as I don't have access to the X-Pack. Were you
able to do so @MikeThomsen?
The only
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
@JPercivall I fixed the dependency issue and tried a simple flow.
Everything seemed to work.
---
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/2113
Hey @MikeThomsen, just reviewed the changes. They look good and I think
we're almost there. One thing that is missing is the comment regarding pulling
the results into memory.
Also, I
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
@JPercivall FYI, part of the reason I want to get this done is I'm planning
on doing a whole new set of ES processors that are based on bring the total
CRUD functionality over to the official
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
Thanks.
---
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/2113
Hey @MikeThomsen, I'm planning on reviewing this tomorrow evening
---
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
@JPercivall Any chance we can close this out?
---
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
@JPercivall changes checked in.
---
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
> I hope my review doesn't come off in the wrong way, there is a lot of
great work here and I just want to make sure the usability is top notch.
Not at all. It's all fair and good
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/2113
Hey @MikeThomsen, just finished up another round of reviewing. Again,
mostly focused on the error handling portion.
I hope my review doesn't come off in the wrong way, there is a lot of
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/2113
Taking a look now. WRT:
> Ok, I can do that from now on. Shouldn't they be squashed before a merge
into master?
Yup, they will be squashed prior to merge to master but the
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
Ok. It should all be there now.
---
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
@JPercivall Thanks for the feedback. I'll get to working on these. WRT:
> Lastly, it's preferred if you don't squash your commits every time. If
you don't, that allows the reviewer to
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/2113
Thanks @MikeThomsen, looks like that fixed the NPE. Still needs the License
and Notice in the client service nar.
Also, it's a bit of an anti-pattern to have the logging and error handling
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
@JPercivall Done.
---
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/2113
Apologises, won't be able to finish the review this evening. I reviewed the
restapi nar License and Notice files they look good to go. Just need to do a
bit of functional testing and verify
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/2113
The nifi-elasticsearch-client-service-nar is missing a license and notice.
---
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/2113
Thanks @MikeThomsen I should be able to take another look later tonight
---
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
@JPercivall It's ready for review.
---
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
2/3 builds pass now.
---
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
@JPercivall @mattyb149 Once I changed the code to use `transfer(FlowFile,
Relationship)` and not `transfer(List, Relationship)` the error stopped
happening.
---
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
@JPercivall @mattyb149 That exception only happens for me when I stop the
processor. It happens here:
``
if (hitsFlowFiles.size() > 0) {
session.transfer(hitsFlowFiles,
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/2113
I'm seeing NPE and FlowFile handling exceptions. I didn't do anything
special, just using the example query. I'll attach a template. Errors from the
logs below.
> 2018-03-12
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/2113
Template for FlowFile handling exception.
[FlowFile_Handling_Exception.txt](https://github.com/apache/nifi/files/1805420/FlowFile_Handling_Exception.txt)
---
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/2113
Looks like the master build is failing with some check style issues, hence
why your builds failed. I'll work on fixing them.
---
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/2113
Reviewing
---
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
Rebased and building. @mattyb149 I changed the functionality to match the
GetMongo functionality you reviewed recently. Had to spend a while getting the
build to work again.
---
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2113
I'm not sure I'll have time to close the loop before 1.6.0, so if you'd
like to finish the review/merge after Mike's rebase that would be very cool,
thanks!
---
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/2113
Hey @mattyb149, what's your status for this review? From a cursory look, it
appears like it just needs an updated pom from @MikeThomsen and the final
approval. With talks of 1.6.0 happening it
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
@mattyb149 I think all of the change requests are done.
---
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
@mattyb149 I moved it to a new NAR and rebased against the latest master
last night. So once this is committed, I'll start working on the new processors
and service methods.
---
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2113
Makes sense to me, we had to do the same with Elasticsearch 2.x and 5.x
because of the difference in transport client versions. As folks move
exclusively to ES5+, it would be nice to deprecate the
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
@mattyb149 I've been thinking about what to do with this PR, and I think it
needs to be put into a new NAR. I'm thinking "nifi-elasticsearch-restapi.nar"
or something like that. What I'd ideally
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
@mattyb149 I think I got your changes done now. I renamed a few classes to
remove obvious references to ElasticSearch 5.X because the goal is to make it
an obvious fit with 6.X too now that that
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
@mattyb149 I think everything should be up to date now.
---
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
@mattyb149 I think everything except the proxy issue is now in there.
---
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
Thanks @mattyb149 I'll try to find some time to address these today or
tomorrow. I've been knee deep in some Mongo-related work for a client. I have
two pull requests for Mongo and record
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2113
Reviewing...
---
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
@joewitt alright, I'll see what trouble I can get into.
---
Github user joewitt commented on the issue:
https://github.com/apache/nifi/pull/2113
@MikeThomsen It is awesome that you're contributing code/PRs. As you can
see though we do have quite a large backlog of PRs that need
review/feedback/cycles. Some of them, like integration with
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
@mattyb149 Have you had a chance to take a look at it?
---
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
@mattyb149 Ok. Changes are made. I refactored the commit to be based on a
controller service. For now, that service only handles a single function: basic
search. However, there is now a
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
@mattyb149 I'm going to leave this open, but I decided to refactor the heck
out of it around a client service for ElasticSearch. The service only has one
method for now, but I think it's the way
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
I set up a profile called integration-tests and renamed the file to
JsonQueryElasticsearch_IT. It runs really well from the project folder, but for
some reason running `mvn clean install
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2113
If you're looking to run integration tests against a ES instance (from the
Maven plugin), perhaps you can split out the integration tests into a new file
called ITJsonQueryElasticsearch5, rather
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
I discovered a slick plugin for Maven that lets you cleanly run
ElasticSearch in the background during integration tests. I added that to the
test setup under a profile. So I have some cleanup to
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2113
Thanks. I'll get to these some time today or tomorrow.
FYI Elastic has some official statements on the future direction of the
Java client here:
49 matches
Mail list logo
