Expose Engine.Searcher provider to ingest plugins. #41010

martijnvg · 2019-04-09T13:20:22Z

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

Needed for #32789 in order for ingest processors to perform local Lucene searches.

elasticmachine · 2019-04-09T13:20:24Z

Pinging @elastic/es-core-features

…arch_to_ingest

martijnvg · 2019-04-15T15:16:57Z

Pinging @elastic/es-core-infra to raise awareness of this change. This PR modifies the ingest plugin extension to provide the ability to an ingest processor to access a locally allocated shard.

jakelandis · 2019-04-17T16:19:16Z

server/src/main/java/org/elasticsearch/ingest/IngestService.java

@@ -96,7 +99,19 @@ public IngestService(ClusterService clusterService, ThreadPool threadPool,
                threadPool.getThreadContext(), threadPool::relativeTimeInMillis,
                (delay, command) -> threadPool.schedule(
                    command, TimeValue.timeValueMillis(delay), ThreadPool.Names.GENERIC
-                ), this
+                ), this, index -> {
+                    IndexService indexService = indicesService.indexService(index);


I assume from the exception message that the IndexService returns null unless it it local to this node ? (it is not very clear from the code if InicesService is per node or per cluster).

Yes, IndicesServices only contains a IndexService instance for locally allocated indices.

jakelandis · 2019-04-17T16:21:35Z

server/src/main/java/org/elasticsearch/ingest/IngestService.java

@@ -96,7 +99,19 @@ public IngestService(ClusterService clusterService, ThreadPool threadPool,
                threadPool.getThreadContext(), threadPool::relativeTimeInMillis,
                (delay, command) -> threadPool.schedule(
                    command, TimeValue.timeValueMillis(delay), ThreadPool.Names.GENERIC
-                ), this
+                ), this, index -> {


I think we will need to do some alias resolution here as it appears that this call requires the concrete index ?

I see now you do it in the test plugin ... but maybe move the logic here to have one place for failures of that kind ?

👍 yeah it makes sense to move the index expression resolution to here.

The Function<Index, Engine.Searcher> would then change into Function<String, Engine.Searcher>.

jakelandis · 2019-04-17T16:29:28Z

server/src/main/java/org/elasticsearch/ingest/IngestService.java

+                    }
+
+                    IndexShard indexShard = indexService.getShard(0);
+                    return indexShard.acquireSearcher("ingest");


for my education, does this create a new searcher or give you back a pre-existing one ? If the former, do we need to do anything with the existing one w/r/t disabling refreshes ? (sorry if this a bogus question, I don't have much experience at this level).

This always creates a new searcher. The code invoking this should be aware of this and make sure to close it after it is done with the searcher. In our case we will need to reuse the searcher from an ingest processor. We don't want to invoke this every time an enrich processor is invoked.

jakelandis · 2019-04-17T16:45:52Z

server/src/main/java/org/elasticsearch/ingest/Processor.java

        public Parameters(Environment env, ScriptService scriptService, AnalysisRegistry analysisRegistry,  ThreadContext threadContext,
                          LongSupplier relativeTimeSupplier, BiFunction<Long, Runnable, Scheduler.ScheduledCancellable> scheduler,
-            IngestService ingestService) {
+                          IngestService ingestService, Function<Index, Engine.Searcher> searcherProvider) {


nit: searcherProvider -> localShardSearcher

jakelandis · 2019-04-17T17:02:55Z

LGTM, just a couple questions and a nitpick

However, I am a bit concerned by exposing this to all ingest plugins and would like another opinion on that point before fully commiting to this approach.

martijnvg · 2019-04-17T17:58:49Z

@elasticmachine run elasticsearch-ci/packaging-sample

martijnvg · 2019-04-17T18:02:45Z

However, I am a bit concerned by exposing this to all ingest plugins and would like another opinion on that point before fully commiting to this approach.

Not sure how else we can expose a local allocated shard to an ingest processor. We either allow it for all plugins or not at all. I think the same applies also to other components (ClusterService, ScriptService etc.) which we already expose to ingest plugins.

jakelandis · 2019-04-17T19:34:13Z

I am not advocating for nor against exposing this as-is. I simply don't fully understand the ramifications (if any) this may brings.

@jpountz or @jasontedor - do you see any potential issues of exposing an IndexSearcher (for local single shard index) directly to any Ingest plugin ?

Not sure how else we can expose a local allocated shard to an ingest processor.

There is likely a way, but would require some refactoring.

…arch_to_ingest

jpountz

@jpountz or @jasontedor - do you see any potential issues of exposing an IndexSearcher (for local single shard index) directly to any Ingest plugin ?

The main issue I can think of is that the searcher might provide access to more information than it should, eg. if a filtered alias is queried, if the index is not allowed for reading, or if there are FLS/DLS rules configured. Searchers are read-only so there is no risk of altering data however.

jpountz · 2019-04-17T14:20:11Z

server/src/main/java/org/elasticsearch/ingest/IngestService.java

+                    IndexService indexService = indicesService.indexService(index);
+                    if (indexService == null) {
+                        throw new ResourceNotFoundException("index [" + index + "] not locally allocated");
+                    }


maybe use indexServiceSafe?

jpountz · 2019-04-18T08:31:46Z

server/src/main/java/org/elasticsearch/ingest/IngestService.java

+                    Index[] resolvedIndices = resolver.concreteIndices(state, IndicesOptions.STRICT_EXPAND_OPEN, indexExpression);
+                    if (resolvedIndices.length != 1) {
+                        throw new IllegalStateException("expression [" + indexExpression + "] can only point to a single concrete index");
+                    }


should we prevent filtered aliases too?

Yes, that makes sense. There is no guarantee that they will be applied in an ingest processor. I will fail with an error if an alias has a filter.

martijnvg · 2019-04-18T08:48:04Z

or if there are FLS/DLS rules configured

I think FLS/DLS rules still apply, because IndexShard#acquireSearcher(...) uses an IndexSearcherWrapper it has been configured and that is the case if dls/fls has been configured.

jpountz · 2019-04-18T08:51:08Z

Let's have a test that the searcher wrapper is applied when acquired via an ingest processor?

martijnvg · 2019-04-19T11:04:38Z

@elasticmachine run elasticsearch-ci/packaging-sample

martijnvg · 2019-04-19T18:12:58Z

@elasticmachine run elasticsearch-ci/packaging-sample

jpountz · 2019-04-24T11:54:54Z

server/src/test/java/org/elasticsearch/ingest/IngestLocalShardSearcherTests.java

+        @Override
+        public IngestDocument execute(IngestDocument ingestDocument) throws Exception {
+            String indexExpression = "reference-index";
+            try (Engine.Searcher engineSearcher = localShardSearcher.apply(indexExpression)) {


Asserting that the wrapper has been called doesn't feel enough to me, I'd like to have a custom directory reader that the wrapper uses, and assert here that we get the expected implementation.

I've adjusted this test: fc939ef

…arch_to_ingest

jpountz

Thanks @martijnvg !

martijnvg · 2019-04-24T17:34:51Z

@elasticmachine run elasticsearch-ci/1

(unrelated test failure, IntervalQueryBuilderTests.testToQuery, java.lang.IllegalArgumentException: Too many disjunctions to expand)

Relates to #32789

martijnvg added 2 commits April 9, 2019 14:35

Expose Engine.Searcher provider to ingest plugins.

d5f8e1d

Relates to elastic#32789

Added test verifying that the extension point works.

5800d23

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

martijnvg added 2 commits April 10, 2019 11:22

fixed checkstyle violation

a867fd6

Merge remote-tracking branch 'es/enrich' into enrich_expose_engine_se…

d9eeae7

…arch_to_ingest

jakelandis self-requested a review April 16, 2019 23:38

jakelandis reviewed Apr 17, 2019

View reviewed changes

martijnvg added 5 commits April 18, 2019 09:13

Merge remote-tracking branch 'es/enrich' into enrich_expose_engine_se…

e0f5384

…arch_to_ingest

renamed searcherProvider -> localShardSearcher

340b0c0

moved index expression logic to localShardSearcher function

7fbbd2f

added more tests

69dcf44

renamed test

15b0606

jpountz reviewed Apr 18, 2019

View reviewed changes

Use indexServiceSafe(...) instead of indexService(...)

6cdd8df

martijnvg added 2 commits April 18, 2019 11:05

fail if alias with filter is used

cafaf5d

added a test that verifies that search wrapper is applied when enabled

3c9afb4

martijnvg requested review from jpountz and jakelandis April 18, 2019 09:21

jakelandis mentioned this pull request Apr 18, 2019

[ingest] Enrich documents prior to indexing #32789

Closed

55 tasks

jpountz reviewed Apr 24, 2019

View reviewed changes

martijnvg added 2 commits April 24, 2019 14:39

Merge remote-tracking branch 'es/enrich' into enrich_expose_engine_se…

43a4725

…arch_to_ingest

adjusted test

fc939ef

martijnvg requested a review from jpountz April 24, 2019 13:29

jpountz approved these changes Apr 24, 2019

View reviewed changes

fixed check style violation

8d88d6f

jakelandis approved these changes Apr 24, 2019

View reviewed changes

martijnvg merged commit 284c508 into elastic:enrich Apr 24, 2019

martijnvg added a commit that referenced this pull request Apr 24, 2019

Expose Engine.Searcher provider to ingest plugins. (#41010)

a61ec11

Relates to #32789

Expose Engine.Searcher provider to ingest plugins. #41010

Expose Engine.Searcher provider to ingest plugins. #41010

Uh oh!

Conversation

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

Uh oh!

Uh oh!

elasticmachine commented Apr 9, 2019

Uh oh!

martijnvg commented Apr 15, 2019

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

jakelandis commented Apr 17, 2019

Uh oh!

martijnvg commented Apr 17, 2019

Uh oh!

martijnvg commented Apr 17, 2019

Uh oh!

jakelandis commented Apr 17, 2019

Uh oh!

jpountz left a comment

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!

Choose a reason for hiding this comment

Uh oh!

martijnvg commented Apr 18, 2019

Uh oh!

jpountz commented Apr 18, 2019

Uh oh!

martijnvg commented Apr 19, 2019

Uh oh!

martijnvg commented Apr 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpountz left a comment

Choose a reason for hiding this comment

Uh oh!

martijnvg commented Apr 24, 2019

Uh oh!

Uh oh!

martijnvg commented Apr 9, 2019 •

edited

Loading

jakelandis Apr 17, 2019 •

edited

Loading