NIFI-5051 Created ElasticSearch lookup service.

[MikeThomsen]

Thank you for submitting a contribution to Apache NiFi.

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 master)?

• Is your initial contribution a single, squashed commit?

For code changes:

• Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
• Have you written or updated unit tests to verify your changes?
• If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
• If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
• If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
• If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

• Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

[zenfenan]

Reviewing..

[zenfenan]

@MikeThomsen Extremely sorry.. Unable to spend the planned time on this one. I'll try to get to it, if no one else does.

[mattyb149]

This is included twice

[mattyb149]

Still twice, I can remove on merge

[pvillard31]

Comment is still valid @MikeThomsen

[MikeThomsen]

Done.

[mattyb149]

Since the lookup is performed on an incoming flow file, is there any reason the Index, Type, etc. properties couldn't support attributes coming from the flow file? If it is this way because the ES Client Service CS can't use them, perhaps we should write up a separate improvement Jira to do something like NIFI-5121.

[MikeThomsen]

I could definitely see some value to that, but since this is a LookupService implementation, we should discuss it in that context. NIFI-5121 only describes one particular interface, and LookupService is more expansive in use.

[mattyb149]

Most record-based processors (usually because they're in the same NAR) extend from SchemaRegistryService and thus offer the same schema-related properties. I realize you can't do that here (currently), but I think we should do one of two things: 1) Move SchemaRegistryService into the API, or 2) Offer the same properties as other record-based processors, including Schema Access Strategy, Schema Text, Schema Version, etc. I think the first one would be easier and more helpful for anyone creating a record-based processor in another bundle, what do you think?

[mattyb149]

I put up #2661 to address option 1 above

[mattyb149]

Now that #2661 has been merged, could you extend SchemaRegistryService instead of AbstractControllerService (which you'll still get as SchemaRegistryService's parent)? That way you'll have immediate access to the same properties and logic as other schema-registry-aware processors to give a consistent UX.

[MikeThomsen]

Ok, I'll give it a shot.

[MikeThomsen]

@mattyb149 I got started on that, but I'm not sure how this is supposed to work. The only entry point that is standard for LookupService is lookup(Map). How did you envision communicating the info for the other lookup strategies? The schema name one makes sense; it can be a property on the service or simply schema.name in the Map passed to lookup. The rest, I'm not sure about.

[mattyb149]

Should this be in a utilities class (if there isn't already such a method in one)? Seems pretty helpful for JSON-to-schema conversions (or any Map-to-Schema) in general.

[MikeThomsen]

I agree. I just wrote up a Jira ticket for this. Let's table it for now because we need to think about things like date strings.

[mattyb149]

It would be nice to have some unit tests as well, since the integration tests do not get run as part of any automated build. I think you could mock the ES Client Service or something? There appears to be something similar in TestFetchElasticsearchHttp (and the other ES processor unit tests).

[MikeThomsen]

Ok, I'll add some.

[mattyb149]

Won't nifi-avro-record-utils bring in the Avro library?

[MikeThomsen]

TBH I think that might have just been IntelliJ acting up.

[MikeThomsen]

@mattyb149 Done.

[MikeThomsen]

@mattyb149 Can you comment on the schema detection strategy issue I raised here?

[MikeThomsen]

@mattyb149 I converted it over to be a subclass of SchemaRegistryService. Let me know if it needs anything else.

[mattyb149]

Reviewing...

[mattyb149]

Still twice, I can remove on merge

[mattyb149]

You don't need this property anymore, as you get one from SchemaRegistryService

[MikeThomsen]

Thought I removed that.

[pvillard31]

Do you want to remove it @MikeThomsen ?

[MikeThomsen]

Done.

[mattyb149]

I kinda thought this LookupService would behave a bit like the Mongo one, where you could give it multiple keys and it would do the query based on that (the fields and the values for each record). This one seems a bit awkward to me, as the user would have to build up their own query field in each record, putting the value they want to match inside a JSON query body.

Is there a different use case here, or could/should we make it more consistent with the other "NoSQL" lookup service(s)? We'd have to generate the query body but that shouldn't be too hard. Also you'd only be able to query top-level fields for lookup, but that seems like it would cover most use cases. If there is a way to specify a nested field for lookup (such as a qualified name with period delimiters), we could do that (although we'd likely have to use a "nested" operator in the generated query), seems like a good (but separate) improvement. Thoughts?

[MikeThomsen]

Yeah, we can do that. I think what it would look like is this:

{
    "bool": {
        "must": [
             {
                 "match": {
                     "username": "john.smith"
                 }
             },
             {
                 "match": {
                     "email": "john.smith@company.com"
                 }
             }
         ]
    }
}

For input:

{
    "username": "john.smith",
     "email": "john.smith@company.com"
}

as lookup keys.

[MikeThomsen]

For nested, it could be trickier. I think this would work:

/user/email => "user.contact.email"

{
    "query": {
         "nested": {
             "path": "user.contact",
              "query": {
                  "match": {
                      "email": "john.smith@company.com"
                   }
              }
         }
     }
}

[MikeThomsen]

@mattyb149 I'm going to work on knocking the basic query builder changes out tonight. Could be a big change, so I apologize in advance :)

[mattyb149]

Did those changes make it into the latest PR?

[MikeThomsen]

TBH, I'm not sure. It's been a while. I'll look at let you know today. Got most of your feedback squared away.

[MikeThomsen]

Yeah, didn't make it in. Good news is that after kicking the tires with Kibana looks like it'll be pretty easy to do if I get some time after work.

[MikeThomsen]

@mattyb149 changed the query model per the discussion above and changed the tests to be Groovy so that the inline JSON, etc. would be a lot cleaner to read.

[MikeThomsen]

@mattyb149 @pvillard31 Changed the query model as requested and it's ready for final review AFAICT.

[pvillard31]

Few minor comments after a quick pass over the code, will try to find time to test it but I probably won't be able to do it before next week :(

[pvillard31]

Comment is still valid @MikeThomsen

[pvillard31]

Do we want to have this kind of configuration in low-level poms? Wondering if it'd be an issue with current modifications to support Java 9/10

[MikeThomsen]

Probably not. Removed.

[pvillard31]

Do you want to remove it @MikeThomsen ?

[pvillard31]

.evaluateExpressionLanguage() ?

[MikeThomsen]

Done.

[pvillard31]

.evaluateExpressionLanguage() ?

[MikeThomsen]

Done.

[MikeThomsen]

@pvillard31 @mattyb149 changes checked in.

[mattyb149]

If we were to add a code coverage plugin to Maven, this is probably something that should be added to the root-level pom (and disabled by default?) What was the impetus for including it in a single bundle?

[MikeThomsen]

I'm not sure how well that would work at root level because there are plenty of integration tests that have to be run to get a full sense of code coverage. So maybe I should back this out or one of you can drop it when rebasing for a merge if you think it makes more sense to add a root level profile for code coverage.

[mattyb149]

This one should be 1.8.0-SNAPSHOT now, sorry it's taken so long to get through

[MikeThomsen]

Done.

[mattyb149]

Probably best to remove the groovy and Jacoco stuff, let's get a discussion going on the dev mailing list about code coverage?

[MikeThomsen]

I'm leaving in the helper plugin for now because for some reason, it won't even detect the groovy test source without it. I'll remove it if you have any suggestions on how to fix that.

[alopresto]

If there is nothing in src/test/java, the Groovy tests won't be detected unless a plugin references them directly. In this case, the build-helper-maven-plugin is accomplishing that. In other locations, the maven-compiler-plugin is set to use groovy-eclipse-compiler to achieve the same result.

[MikeThomsen]

@alopresto thanks for explaining that. I just added a .gitignore file into src/test/java and that did the trick.

[mattyb149]

Just a style nitpick, these can be set up in the constructor or a static block (I think the former is preferred?). Unless they're dynamic the list only needs to be created once, where this method gets called often IIRC.

[MikeThomsen]

Done.

[mattyb149]

This shouldn't pass CheckStyle as we don't allow star imports in Java, we probably just don't have an existing (or complete) CheckStyle rule for Groovy files.

[MikeThomsen]

Manually fixed that.

[MikeThomsen]

@mattyb149 Refactored the query builder.

[MikeThomsen]

Should be all good to go now.

[alopresto]

@MikeThomsen I would maybe put a comment in the src/test/java/.gitignore file explaining why it's there so someone in the future doesn't see it as a superfluous tooling artifact and remove it, and then your test is silently no longer executed and we don't catch regressions. We've had similar occurrences in some of the other modules.

[MikeThomsen]

@mattyb149 can we merge?

[MikeThomsen]

@mattyb149 can we close this out?

[mattyb149]

Reviewing...

[mattyb149]

I think this has to be public for ServiceLoader to find it, I'm getting errors when trying to load it into NiFi.

[MikeThomsen]

facepalm

One fix, coming right up...

[MikeThomsen]

Done. NiFi is able to load it and assign it as the lookupservice for lookuprecord.

[mattyb149]

I've run the unit and integration tests and the code LGTM, but I'd feel better if I could get an example going where I do the lookup on a field that's not at the top level. I have a document containing a "user" field, which contains other fields such as "name", and "name" contains other fields like "first" and "last". I tried using this with a simple CSV input containing an id and a first name, and tried to use the lookup service to match "user.name.first" and return the value of "user.name.last", but got an error saying I was trying to do a nested query on a field that wasn't nested. I didn't add an explicit mapping for the index, just put the complex JSON docs into ES. Am I configuring it wrong, or is this not supported, or could there be a bug?

[MikeThomsen]

I'll look into that. Should be able to get something resolved this weekend.

[MikeThomsen]

but got an error saying I was trying to do a nested query on a field that wasn't nested.

I think you are. ES can be weird about detecting nested documents. I've only had consistent good results when explicitly defining them. I'll try to set up a test example.

[MikeThomsen]

So, I got it working and will share some artifacts tomorrow if I get a chance so you can watch them in action. I'm thinking some of the behavior still needs a second opinion on the flexibility/user-friendliness.

[mattyb149]

I'm still getting the same error (nested object under path [user.name] is not of nested type) on my flow. I tried yours but I don't have any documents/mappings in ES (such as a doc with "subfield.longfield), can you share an example doc I can put in there? I have my own ES so I didn't start up the Docker Compose stuff you attached.

[MikeThomsen]

See my comment below. It has a sample flow, the commands for Kibana and a docker compose file.

[MikeThomsen]

ES_Lookup_Test.xml.txt

5051 Kibana Commands

docker-compose.yml.txt

[mattyb149]

+1 LGTM, ran full build with unit tests, tried the lookup service with a nested record and everything worked fine. Thanks for the improvement! Merging to master