Add an index->step cache to the PolicyStepsRegistry #82316

joegallo · 2022-01-06T15:03:32Z

Related to #77466. Extracted and cleaned up from #79692.

Adds a Index->Step cache (well, really Index->IndexMetadata+Step cache) that saves us from having to recalculate the step each time. It's keyed off the index, but if the IndexMetadata has changed, then the cache is bypassed and updated. Crucially, though, we also need to clear the cache in the event that the policies themselves change (because a policy change can result in the next step being different even if the IndexMetadata itself didn't change).

joegallo · 2022-01-11T20:03:03Z

Before this change (~20% of all samples in ILM, PolicyStepsRegistry.getStep highlighted with red rectangle):

After this change (~5% of all samples in ILM):

original-brownbear · 2022-01-11T20:32:27Z

x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java

@@ -305,7 +332,9 @@ public Step getStep(final IndexMetadata indexMetadata, final Step.StepKey stepKe
                + phaseSteps;

        // Return the step that matches the given stepKey or else null if we couldn't find it
-        return phaseSteps.stream().filter(step -> step.getKey().equals(stepKey)).findFirst().orElse(null);
+        final Step s = phaseSteps.stream().filter(step -> step.getKey().equals(stepKey)).findFirst().orElse(null);


Just a thought from looking at the profiling you posted: This might be happier as a normal loop :) The phaseSteps list is mostly (always?) very short and setting up a stream and doing all the dance around it doesn't even come close to outweighing the slightly faster iteration the stream provides. I'm good with this either way though, just a thought since this line was changed anyway :)

Ehhhhhhh, I don't think it's worth it. This reads pretty clearly as-is, and I don't think it's enough of a bottleneck anymore to warrant unrolling to a normal loop (due to this PR's optimization resulting in the code in question just being called a bajillion times less frequently).

elasticmachine · 2022-01-11T20:44:41Z

Pinging @elastic/es-data-management (Team:Data Management)

original-brownbear

LGTM If we add some JavaDoc :)

original-brownbear · 2022-01-12T15:10:21Z

x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java

@@ -64,6 +66,8 @@
    private final Map<String, Map<Step.StepKey, Step>> stepMap;
    private final NamedXContentRegistry xContentRegistry;

+    private final Map<Index, Tuple<IndexMetadata, Step>> cachedSteps = new ConcurrentHashMap<>();


Lets add a line or two of Javadoc explaining what this is for please. This is far from obvious for anybody not involved in this work.

How does 5f48e35 sparkle with you?

Would it be possible to key the "changed state" key check that is based on IndexMetadata to use a combination of LifecycleExecutionState and the policy name instead (perhaps encapsulated into an actual Key class)? IndexMetadata feels really heavyweight for this, and the step itself can only differ based on either the policy name or the execution state.

It feels strange to me to hold on to things like the mappings inside this cache, when those should have no bearing on the current ILM step.

IndexMetadata feels really heavyweight for this

We only use it for instance equality and clear it out of the cache if the index gets removed => memory wise it's free, instance equality comparison is cheap/free and this seems like the safest way of checking whether or not index metadata has changed.

We didn't just create a compound key here because we need/want the ability to quickly cleanup the cache if an index is removed without having to iterate a cache. Since we only care about the by index state this seemed fastest/easiest.

This has the side effect however, of any inconsequential change to the index metadata for the index (a dynamic field introduced, for example) invalidating the cache, whereas it doesn't actually need to be invalidated in that case?

I think that's ok. This is just supposed to help us with not running the expensive step computation on unchanged indices (which will always be the majority in almost any CS update). No need to super optimize this beyond that I'd say. Updating index metadata results in expensive operations anyway that even go as far as resulting in disk IO, the step calculation is probably trivial relatively speaking in all cases.

Alright, that's fine then, my preference is still to make the keyed part as small as possible, but this is okay for now I think.

dakrone

I left a few comments, I think IndexMetadata is a bit big to hold on to for a cache like this, and I'd prefer to keep the map as small as possible, what do you think?

dakrone · 2022-01-12T17:45:53Z

x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java

@@ -155,6 +166,22 @@ public LifecyclePolicyMetadata read(StreamInput in, String key) {
        }
    }

+    public void delete(Index deleted) {


Can you add javadoc to this too please, since it isn't as clear when this is intended to be used?

👍, 68161e8

dakrone · 2022-01-12T17:56:04Z

x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java

+        for (Index index : event.indicesDeleted()) {
+            policyRegistry.delete(index);
+        }


Can you wrap this in if (this.isMaster) since we don't care about removing the Index entries if we aren't master since the policyRegistry is cleared when transitioning away from master? We might as well avoid iterating through the deleted indices on every node.

huh come to think of it, can't we just return if isMaster is false, no need to load the ILM metadata below either? :)

That works also, we can return after the cancelJob() and policyRegistry.clear() call above.

👍, 4954725

I didn't end up going with an early return but the logic is essentially the same as if I did.

dakrone · 2022-01-12T18:07:56Z

x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java

@@ -64,6 +66,8 @@
    private final Map<String, Map<Step.StepKey, Step>> stepMap;
    private final NamedXContentRegistry xContentRegistry;

+    private final Map<Index, Tuple<IndexMetadata, Step>> cachedSteps = new ConcurrentHashMap<>();


Would it be possible to key the "changed state" key check that is based on IndexMetadata to use a combination of LifecycleExecutionState and the policy name instead (perhaps encapsulated into an actual Key class)? IndexMetadata feels really heavyweight for this, and the step itself can only differ based on either the policy name or the execution state.

It feels strange to me to hold on to things like the mappings inside this cache, when those should have no bearing on the current ILM step.

dakrone · 2022-01-12T18:15:29Z

x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java

@@ -267,6 +294,11 @@ public LifecyclePolicyMetadata read(StreamInput in, String key) {

    @Nullable
    public Step getStep(final IndexMetadata indexMetadata, final Step.StepKey stepKey) {
+        final Tuple<IndexMetadata, Step> cachedStep = cachedSteps.get(indexMetadata.getIndex());
+        if (cachedStep != null && cachedStep.v1() == indexMetadata && cachedStep.v2().getKey().equals(stepKey)) {


Can you add a comment about why instance equality is used here and okay rather than object equality?

👍, 68161e8

henningandersen

Just a drive-by-comment.

henningandersen · 2022-01-13T14:55:05Z

x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java

            }
        }

+        for (Index index : event.indicesDeleted()) {
+            policyRegistry.delete(index);


I wonder if there is an (benign for sure) risk of a leak here: if a policy adds a step to the cache concurrently with it being deleted here, I think we may leave it there until next clear or master failover. Ideally we would retain only those that remain (thus curing this), but I can see how that is more expensive. A comment could be good though.

Sure thing, 19ecdd3

joegallo · 2022-01-13T20:49:26Z

Dropped the 7.17.0 label based on our conversation this earlier today.

original-brownbear · 2022-01-17T11:21:57Z

@joegallo fyi we got the green-light to backport this to 7.17 after all!

elasticsearchmachine · 2022-01-27T15:29:28Z

Hi @joegallo, I've created a changelog YAML for you.

Promoting the isMaster check around triggerPolicies up to include both these blocks.

dakrone

LGTM

* upstream/master: (100 commits) Avoid duplicate _type fields in v7 compat layer (elastic#83239) Bump bundled JDK to 17.0.2+8 (elastic#83243) [DOCS] Correct header syntax (elastic#83275) Add unit tests for indices.recovery.max_bytes_per_sec default values (elastic#83261) [DOCS] Add note that write indices are not replicated (elastic#82997) Add notes on indexing to kNN search guide (elastic#83188) Fix get-snapshot-api :docs:integTest (elastic#83273) FilterPathBasedFilter support match fieldname with dot (elastic#83178) Fix compilation issues in example-plugins (elastic#83258) fix ClusterStateListener javadoc (elastic#83246) Speed up Building Indices Lookup in Metadata (elastic#83241) Mute whole suite for elastic#82502 (elastic#83252) Make PeerFinder log messages happier (elastic#83222) [Docs] Add supported _terms_enum field types (elastic#83244) Add an aggregator for IPv4 and IPv6 subnets (elastic#82410) [CI] Fix 70_time_series/default sort yaml test failures (elastic#83217) Update test-failure Issue Template to include "needs:triage" label elastic#83226 Add an index->step cache to the PolicyStepsRegistry (elastic#82316) Improve support for joda datetime to java datetime transition in Painless (elastic#83099) Fix joda migration for week based methods in Painless (elastic#83232) ... # Conflicts: # x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/v2/TransportRollupAction.java

joegallo added :Data Management/ILM+SLM Index and Snapshot lifecycle management v8.0.0 v8.1.0 labels Jan 6, 2022

original-brownbear and others added 5 commits January 6, 2022 14:54

Cache for getStep

23d02cc

Add some comments

e6e6118

Move the delete logic from listener to applier

92346ef

Clear out the cache entirely on master->non-master

58ac8a0

Clear out the cache any time the policies change

ed38398

joegallo force-pushed the ilm-policy-registry-steps-cache branch from 3126e40 to ed38398 Compare January 6, 2022 19:55

Merge branch 'master' into ilm-policy-registry-steps-cache

bdf4be8

original-brownbear reviewed Jan 11, 2022

View reviewed changes

joegallo marked this pull request as ready for review January 11, 2022 20:44

elasticmachine added the Team:Data Management Meta label for data/management team label Jan 11, 2022

joegallo added >non-issue v7.17.0 labels Jan 11, 2022

joegallo requested a review from original-brownbear January 12, 2022 15:07

original-brownbear approved these changes Jan 12, 2022

View reviewed changes

joegallo added 4 commits January 12, 2022 10:31

Whitespace

38a1711

Put these first fields in public constructor order

de52716

Add a comment that explains this cache

5f48e35

Merge branch 'master' into ilm-policy-registry-steps-cache

91fcef1

dakrone self-requested a review January 12, 2022 15:36

dakrone requested changes Jan 12, 2022

View reviewed changes

dakrone reviewed Jan 12, 2022

View reviewed changes

joegallo added >enhancement and removed >non-issue labels Jan 12, 2022

Merge branch 'master' into ilm-policy-registry-steps-cache

84a667e

henningandersen reviewed Jan 13, 2022

View reviewed changes

joegallo removed the v7.17.0 label Jan 13, 2022

original-brownbear added the v7.17.0 label Jan 17, 2022

albertzaharovits added v7.17.1 and removed v7.17.0 labels Jan 20, 2022

joegallo and others added 2 commits January 27, 2022 10:24

Merge branch 'master' into ilm-policy-registry-steps-cache

0f37d1e

Update docs/changelog/82316.yaml

25605eb

joegallo added 4 commits January 27, 2022 13:13

Add some explanatory comments

68161e8

Surround this logic in an isMaster check

4954725

Promoting the isMaster check around triggerPolicies up to include both these blocks.

Add a note about a race condition

19ecdd3

Merge branch 'master' into ilm-policy-registry-steps-cache

8ed3286

joegallo requested a review from dakrone January 27, 2022 18:22

dakrone approved these changes Jan 27, 2022

View reviewed changes

joegallo merged commit 72f7947 into elastic:master Jan 27, 2022

joegallo deleted the ilm-policy-registry-steps-cache branch January 27, 2022 21:44

joegallo added a commit that referenced this pull request Jan 27, 2022

Add an index->step cache to the PolicyStepsRegistry (#82316)

34ce3f3

joegallo added a commit that referenced this pull request Jan 27, 2022

Add an index->step cache to the PolicyStepsRegistry (#82316)

c330790

joegallo added v7.17.0 and removed v7.17.1 labels Jan 27, 2022

pugnascotia added v8.0.0-rc2 and removed v8.0.0 labels Feb 1, 2022

joegallo mentioned this pull request Mar 2, 2022

Fix PolicyStepsRegistry's cachedSteps null handling #84588

Merged

joegallo mentioned this pull request Mar 24, 2022

Replace failing PolicyStepsRegistry sanity check assert with a test #85346

Merged

Add an index->step cache to the PolicyStepsRegistry #82316

Add an index->step cache to the PolicyStepsRegistry #82316

Uh oh!

Conversation

joegallo commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joegallo commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jan 11, 2022

Uh oh!

original-brownbear 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!

dakrone Jan 12, 2022 • 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!

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

dakrone Jan 12, 2022 • 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!

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

joegallo commented Jan 13, 2022

Uh oh!

original-brownbear commented Jan 17, 2022

Uh oh!

elasticsearchmachine commented Jan 27, 2022

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joegallo commented Jan 6, 2022 •

edited

Loading

joegallo commented Jan 11, 2022 •

edited

Loading

dakrone Jan 12, 2022 •

edited

Loading

dakrone Jan 12, 2022 •

edited

Loading