Add enrich policy runner #41088

jbaiera · 2019-04-10T18:19:12Z

Note that this is a PR against the enrich branch and will be backported to enrich-7.x branch.

Adds a skeleton of the execution logic to execute an enrich policy. Validates the source index existence as well as mappings, creates a new enrich index for the policy, reindexes the source index into the new enrich index, and swaps the enrich alias for the policy to the new index.

WIP: Comments are welcome at this point. The code really needs some testing and will most likely be rebased on to some other PR's as they come in (namely #41003 and #40997), but I'd like to get some feedback before then. Good to go for review

elasticmachine · 2019-04-10T18:19:14Z

Pinging @elastic/es-core-features

martijnvg · 2019-04-11T12:27:09Z

The general approach in the PR looks good to me.

…ix things

martijnvg

I left a few nits. This change is going well!

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyResult.java

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java

martijnvg · 2019-04-16T15:26:06Z

x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunnerTests.java

+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.nullValue;
+
+public class EnrichPolicyRunnerTests extends ESSingleNodeTestCase {


would be great to test this without starting a node, but I'm not sure how :)

Yeah, this was one of those cases where it felt like mocking out all of the dependencies needed was a bit much. Would be happy to change the test if there are any tools to make that easier, or if we feel that it's important enough that it doesn't spin up a node.

Yes, I think this is the right trade off. Otherwise the code for mocking becomes unmaintainable.

jbaiera · 2019-04-16T20:20:00Z

Removed WIP label, should be good for a proper review at this point

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java

jakelandis · 2019-04-16T20:48:07Z

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java

+                        "Enrich policy execution for [{}] failed. Could not locate enrich key field [{}] on mapping for index [{}]",
+                        policyName, policy.getEnrichKey(), sourceIndex));
+            }
+            for (String enrichField : policy.getEnrichValues()) {


I think it is OK if the not all of the values are present in the source index. Misses of the values should be handled by the processor.

However, we may want to log a warning if the same value is coming from multiple indexes since only 1 will win and it's not deterministic.

I lean towards failing if a decorate field is missing in the mapping. I think this can lead to unexpected behaviour at enrich time? (why does this document not have all the enriched fields?)

Even if the mapping exists in the source index, the document we are using to enrich may not have that field and will need handle partial decorations in the processor.

If the decision to only allow full decorations, then the check here makes sense... but will need to changed slightly such that all enrich fields exist across the set of the source indexes (as opposed to exist in every source index as it is here).

I am in favor of removing this check and implement ignore_missing (or the like) in the processor to allow partial decorations.

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java

jakelandis · 2019-04-16T21:23:43Z

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java

+        client.execute(ReindexAction.INSTANCE, reindexRequest, new ActionListener<BulkByScrollResponse>() {
+            @Override
+            public void onResponse(BulkByScrollResponse bulkByScrollResponse) {
+                // Do we want to fail the request if there were failures during the reindex process?


I am not familiar with this API... does this get called once after the whole re-index is complete, or per scroll response ?

this gets invoked after the whole reindex has been completed.

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyResult.java

x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunnerTests.java

jakelandis · 2019-04-16T21:45:51Z

x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunnerTests.java

+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.nullValue;
+


The tests here are good for the happy case, but I would like to see more testing around the un-happy cases. Investments now in this test will greatly help future developers to quickly test changes.

martijnvg

I did another review round.

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java

martijnvg · 2019-04-17T08:33:12Z

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java

+        client.execute(ReindexAction.INSTANCE, reindexRequest, new ActionListener<BulkByScrollResponse>() {
+            @Override
+            public void onResponse(BulkByScrollResponse bulkByScrollResponse) {
+                // Do we want to fail the request if there were failures during the reindex process?


this gets invoked after the whole reindex has been completed.

x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunnerTests.java

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java

jakelandis · 2019-04-17T16:00:44Z

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java

+                        "Enrich policy execution for [{}] failed. Could not read mapping for source [{}] included by pattern [{}]",
+                        policyName, sourceIndex, policy.getIndexPattern()));
+            }
+            if (properties.containsKey(policy.getEnrichKey()) == false) {


we should probably allow _id as the enrich key

Agreed, though I think it's probably a good idea to put that into a separate PR. I don't think we should bog this one down with too many initial features.

Add EnrichPolicyExecutor which runs runners within a threadpool executor Update the tests

jbaiera · 2019-04-18T21:00:26Z

I made the runner object a Runnable and added a basic EnrichPolicyExecutor implementation. I figured the management threadpool would be fine to execute these on. I also simplified the enrich mapping to just define the key, and rely on the eventual meta mapping type to be added later.

martijnvg

I did another review round. I like the split between EnrichPolicyExecutor and EnrichPolicyRunner.

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyExecutor.java

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java

martijnvg · 2019-04-19T11:51:12Z

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java

+        SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder();
+        searchSourceBuilder.fetchSource(retainFields.toArray(new String[0]), new String[0]);
+        if (policy.getQuery() != null) {
+            searchSourceBuilder.query(QueryBuilders.wrapperQuery(policy.getQuery().getQuery()));


Why is the policy query wrapped in a wrapper query?

I went with the wrapper query since the EnrichPolicy.QuerySource returns the raw query json as a byte sequence. Is there a more appropriate way to convert a raw query body into a query builder?

I see, actually that does make sense to me :)

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java

martijnvg · 2019-04-19T11:53:06Z

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java

+                listener.onFailure(e);
+            }
+        });
+    }


Should we also delete the old enrich index? Maybe not here and not in the pr, but just wondering. These old indices should be purged at some point in time.

These should definitely be subject to some kind of background cleanup task.

x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunnerTests.java

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java

Adds the foundation of the execution logic to execute an enrich policy. Validates the source index existence as well as mappings, creates a new enrich index for the policy, reindexes the source index into the new enrich index, and swaps the enrich alias for the policy to the new index.

Backports #41088 Adds the foundation of the execution logic to execute an enrich policy. Validates the source index existence as well as mappings, creates a new enrich index for the policy, reindexes the source index into the new enrich index, and swaps the enrich alias for the policy to the new index.

jbaiera added >non-issue WIP :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP labels Apr 10, 2019

jbaiera requested review from martijnvg, hub-cap and jakelandis April 10, 2019 18:19

jbaiera added 3 commits April 15, 2019 14:08

WIP Policy execution code for enrich

6336351

Get the policy runner to pass a basic test, add licenses, generally f…

05769e5

…ix things

Fix up implementation after merging in new enrich model

6828919

jbaiera force-pushed the enrich-policy-runner branch from 90b124a to 6828919 Compare April 15, 2019 19:58

martijnvg reviewed Apr 16, 2019

View reviewed changes

Modify runner to accept index patterns for input

0b6ea3a

jbaiera removed the WIP label Apr 16, 2019

jbaiera added 4 commits April 16, 2019 16:21

Make enrich policy result immutable

41a886c

Make enrich policy runner constructor package private

3c4b24b

Add assert to mapping validation

96899fb

Remove SuppressWarnings annotation in policy runner

3dc1e4d

jbaiera requested a review from martijnvg April 16, 2019 20:29

jakelandis requested changes Apr 16, 2019

View reviewed changes

martijnvg requested changes Apr 17, 2019

View reviewed changes

jakelandis reviewed Apr 17, 2019

View reviewed changes

jbaiera added 6 commits April 18, 2019 13:45

Rename EnrichPolicyResult to PolicyExecutionResult

9c5aa7a

Check for null policy when executing id

47099af

Make ".enrich" a constant

ea22658

Simplify the enrich mapping to just the enrich key, disable _source

f3e7afe

Make EnrichPolicyRunner a Runnable

7546e67

Add EnrichPolicyExecutor which runs runners within a threadpool executor Update the tests

Remove enrich value field validation

1be4639

Use keySet on mapping validation instead of Map

6d10db0

Swap to diamond operators on ActionListener impls

23493bf

martijnvg requested changes Apr 19, 2019

View reviewed changes

martijnvg reviewed Apr 30, 2019

View reviewed changes

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java Outdated Show resolved Hide resolved

martijnvg reviewed Apr 30, 2019

View reviewed changes

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java Show resolved Hide resolved

jbaiera added 10 commits April 30, 2019 11:53

Merge branch 'enrich' into enrich-policy-runner

b751660

Use Generic thread pool when executing policy.

35c4a2a

Auto expand replicas on enrich indices.

31eeb40

Enable source on enrich indices

6c884d3

set enrich mapping dynamic field to false

112a40b

Remove default match all query

4137585

Add additional asserts to test

05f201c

Force content type of enrich index to use SMILE

ac69e7d

Fix precommit

08dc157

Merge branch 'enrich' into enrich-policy-runner

7e5d52c

jbaiera requested review from jakelandis and martijnvg May 1, 2019 18:14

Merge branch 'enrich' into enrich-policy-runner

74df5a7

martijnvg approved these changes May 2, 2019

View reviewed changes

jbaiera added 2 commits May 2, 2019 10:09

Merge branch 'enrich' into enrich-policy-runner

6522151

Merge branch 'enrich' into enrich-policy-runner

064371d

jbaiera merged commit a451292 into elastic:enrich May 2, 2019

jbaiera deleted the enrich-policy-runner branch May 2, 2019 16:38

jbaiera added the backport pending label May 2, 2019

jbaiera removed the backport pending label May 6, 2019

Add enrich policy runner #41088

Add enrich policy runner #41088

Uh oh!

Conversation

jbaiera commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Apr 10, 2019

Uh oh!

martijnvg commented Apr 11, 2019

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbaiera commented Apr 16, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelandis Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbaiera commented Apr 18, 2019

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbaiera commented Apr 10, 2019 •

edited

Loading

jakelandis Apr 17, 2019 •

edited

Loading