Add enrich node cache by martijnvg · Pull Request #76800 · elastic/elasticsearch

martijnvg · 2021-08-23T07:01:01Z

Introduce a LRU cache to avoid searches that occur frequently
from the enrich processor.

Relates to #48988

Introduce a LRU cache to avoid searches that occur frequently from the enrich processor. Relates to elastic#48988

elasticmachine · 2021-08-23T07:01:05Z

Pinging @elastic/es-core-features (Team:Core/Features)

danhermann

LGTM. Minor comments below but nothing blocking.

danhermann · 2021-08-30T13:27:00Z

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

        return String.valueOf(maxConcurrentRequests * maxLookupsPerRequest);
    }, val -> Setting.parseInt(val, 1, Integer.MAX_VALUE, QUEUE_CAPACITY_SETTING_NAME), Setting.Property.NodeScope);

+    public static final Setting<Long> CACHE_SIZE = Setting.longSetting("enrich.cache_size", 1000, 0, Setting.Property.NodeScope);


Were you planning to add documentation for this new setting?

Good point, I will add docs for this.

I pushed: 8c328a4
This is the first enrich setting that we document.
@jrodewig do you think this is the right place?

Maybe other enrich settings should be documented as well along side the cache size setting. I will do this in a follow up and ping James when he is back. I will revert the mentioned commit for now.

Thanks for the ping @martijnvg. I'll be happy to take a look!

danhermann · 2021-08-30T13:30:17Z

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

+    EnrichProcessorFactory(Client client, ScriptService scriptService, EnrichCache enrichCache) {
        this.client = client;
        this.scriptService = scriptService;
+        this.enrichCache = enrichCache;


Maybe require non-null here since the cache is used unconditionally in createSearchRunner?

👍 will do that

Fixed: 9deb1b5

jbaiera

LGTM as well, just a little I noticed

jbaiera · 2021-08-30T20:32:18Z

.../plugin/core/src/main/java/org/elasticsearch/xpack/core/enrich/action/EnrichStatsAction.java

            return executingPolicies.equals(response.executingPolicies) &&
-                coordinatorStats.equals(response.coordinatorStats);
+                coordinatorStats.equals(response.coordinatorStats) &&
+                cacheStats.equals(response.cacheStats);


cacheStats could be end up null in mixed clusters

Good spot, I will fix this.

Fixed: 9070d70

jbaiera · 2021-08-30T20:50:07Z

...enrich/src/main/java/org/elasticsearch/xpack/enrich/action/EnrichCoordinatorProxyAction.java

    public static class TransportAction extends HandledTransportAction<SearchRequest, SearchResponse> {

        private final Coordinator coordinator;
+        private final EnrichCache enrichCache;


Are we making use of this anywhere in the proxy action?

No, I initially did caching from here, but then moved it to the processor. The reason that in case something is cached then invoking coordinator action can be skipped as well.

Fixed: 55e6e24

mjmbischoff · 2021-09-02T15:34:56Z

@martijnvg can you have a look at mjmbischoff@08950c3 / pull from https://github.com/mjmbischoff/elasticsearch/tree/enrich_cache

martijnvg · 2021-09-02T17:50:53Z

@mjmbischoff Thanks for noticing this! Using computeIfAbsent(...) is preferable here over put() and get() (there are some other places where I think this can be changed too). I will pull that commit from your branch.

martijnvg · 2021-09-02T18:07:56Z

@mjmbischoff I overlooked something, computeIfAbsent(...) can't be used here, because a potential remote call is made here and this method can't handle asynchronous execution with an ActionListener. Blocking a thread like is done here, should be avoided. Otherwise thread pools may get exhausted (For example if another node is slow or search thread pool is current node is exhausted).

This reverts commit 8c328a4.

mjmbischoff · 2021-09-03T01:19:53Z

@martijnvg Yeah my bad I overlooked that the second call was async.

We could keep the get on the normal thread, and then do a computeIfAbsent on the client threadpool. While this fixes multiple executions, the problem here is that it eats one additional thread. I don't see a way to fire off EnrichCoordinatorProxyAction on the calling thread, which would remove the drawback.

However, we can do one better by not having SearchResponse's in the cache but CompletableFuture<SearchResponse>'s this allows us to:

make the compute fast/ non-blocking by:
- creating a CompletableFuture
- do the async call(client.execute)
- set the result on CompletableFuture in the callback of the async call.
consume the retrieved CompletableFuture async / non-blocking by registering a callback on the CompletableFuture which gets called by the thread setting the value

This does have the side effect that exceptions are now cached, so when we 'consume' the exception I'm currently invalidating the cache entry so subsequent look-ups can try again. -> I guess we could do something fancy here, speeding things up when it's a permanent failure.

This results in the cache interaction being non-blocking and only a single search request is performed when cache look-ups for the same value happen in parallel.

I've updated my branch reflecting these changes

martijnvg · 2021-09-03T07:33:16Z

Thanks @mjmbischoff, I think that should work and is a good improvement. I will merge this PR, can you open a followup pr with these changes?

Backporting elastic#76800 to 7.x branch. Introduce a LRU cache to avoid searches that occur frequently from the enrich processor. Relates to elastic#48988

Backporting #76800 to 7.x branch. Introduce a LRU cache to avoid searches that occur frequently from the enrich processor. Relates to #48988

mjmbischoff · 2021-09-03T14:13:28Z

Thanks @mjmbischoff, I think that should work and is a good improvement. I will merge this PR, can you open a followup pr with these changes?

Will do, opened #77259

Add enrich node cache

6d1a534

Introduce a LRU cache to avoid searches that occur frequently from the enrich processor. Relates to elastic#48988

martijnvg added >enhancement :Distributed/Ingest Node Execution or management of Ingest Pipelines v8.0.0 v7.16.0 labels Aug 23, 2021

elasticmachine added the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label Aug 23, 2021

martijnvg mentioned this pull request Aug 23, 2021

Enrich processor followup work #48988

Open

10 tasks

martijnvg added 6 commits August 23, 2021 09:07

Merge remote-tracking branch 'es/master' into enrich_cache

f82a583

fix hlrc integration

fdd7d24

Merge remote-tracking branch 'es/master' into enrich_cache

e5d71e7

adjust docs

bc66265

fixed npe if there are no stats in a mixed cluster scenario

41161f7

fixed npe if there are no stats in a mixed cluster scenario (2)

3b1fdbc

martijnvg requested review from danhermann and jbaiera August 25, 2021 07:21

danhermann approved these changes Aug 30, 2021

View reviewed changes

jbaiera approved these changes Aug 30, 2021

View reviewed changes

martijnvg added 8 commits August 31, 2021 10:50

Merge remote-tracking branch 'es/master' into enrich_cache

33a7136

Use Objects.equals(...) for equals checking cacheStats

9070d70

Removed unused field

55e6e24

require not null

9deb1b5

check style

b2388af

added docs.

8c328a4

adjust test

42bf683

Merge remote-tracking branch 'es/master' into enrich_cache

56f1139

martijnvg added 2 commits September 2, 2021 19:43

Merge remote-tracking branch 'es/master' into enrich_cache

28b4d1a

fix after merging in master

24b3426

Revert "added docs."

ac72f66

This reverts commit 8c328a4.

martijnvg merged commit 1ae4f3c into elastic:master Sep 3, 2021

martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Sep 3, 2021

Add enrich node cache

587adf4

Backporting elastic#76800 to 7.x branch. Introduce a LRU cache to avoid searches that occur frequently from the enrich processor. Relates to elastic#48988

martijnvg mentioned this pull request Sep 3, 2021

[7.x] Add enrich node cache #77229

Merged

martijnvg added the backport pending label Sep 3, 2021

martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Sep 3, 2021

Disable bwc tests for backporting elastic#76800

250e74d

martijnvg mentioned this pull request Sep 3, 2021

Disable bwc tests for backporting #76800 #77232

Merged

martijnvg added a commit that referenced this pull request Sep 3, 2021

Disable bwc tests for backporting #76800 (#77232)

d743920

martijnvg added a commit that referenced this pull request Sep 3, 2021

Add enrich node cache (#77229)

68fc062

Backporting #76800 to 7.x branch. Introduce a LRU cache to avoid searches that occur frequently from the enrich processor. Relates to #48988

martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Sep 3, 2021

Enable bwc tests after backporting elastic#76800

00dd0dc

martijnvg mentioned this pull request Sep 3, 2021

Enable bwc tests after backporting #76800 #77235

Merged

martijnvg added a commit that referenced this pull request Sep 3, 2021

Enable bwc tests after backporting #76800 (#77235)

8dd4231

martijnvg removed the backport pending label Sep 3, 2021

jakelandis added v8.0.0-alpha2 and removed v8.0.0 labels Sep 15, 2021

mjmbischoff mentioned this pull request Oct 4, 2021

Improving cache lookup to reduce recomputing / searches #77259

Merged

Conversation

martijnvg commented Aug 23, 2021

Uh oh!

elasticmachine commented Aug 23, 2021

Uh oh!

danhermann 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!

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!

jbaiera 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!

martijnvg Aug 31, 2021 • 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!

mjmbischoff commented Sep 2, 2021

Uh oh!

martijnvg commented Sep 2, 2021

Uh oh!

martijnvg commented Sep 2, 2021

Uh oh!

mjmbischoff commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martijnvg commented Sep 3, 2021

Uh oh!

mjmbischoff commented Sep 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

martijnvg Aug 31, 2021 •

edited

Loading

mjmbischoff commented Sep 3, 2021 •

edited

Loading