Store DataTier Preference directly on IndexMetadata #78668

original-brownbear · 2021-10-05T07:35:05Z

The data tier preference is very expensive to parse out of the setting string
repeatedly for large number of indices when using it in the data tier allocation decider.

=> as done with other index settings relevant to allocation, this commit moves the data tier
preference to a field in IndexMetadata. The required moving the DataTier class itself to
server (as well as moving the setting handling from the allocator into it + moving some additional searchable snapshot
setting related logic to server). In a follow-up we can look into making the setting a list setting to remove the
duplication around turning the string value into a list in various places.

The data tier preference is very expensive to parse out of the setting string repeatedly for large number of indices when using it in the data tier allocation decider. => as done with other index settings relevant to allocation, this commit moves the data tier preference to a field in `IndexMetadata`. The required moving the `DataTier` class itself to `server`. In a follow-up we can look into making the setting a list setting to remove the duplication around turning the string value into a list in various places.

elasticmachine · 2021-10-05T07:35:08Z

Pinging @elastic/es-distributed (Team:Distributed)

DaveCTurner

Looks good, I left just a few small comments.

DaveCTurner · 2021-10-06T09:12:51Z

.../java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDeciderTests.java

@@ -100,12 +99,12 @@ public void testIndexPrefer() {
            d = decider.canAllocate(shard, node, allocation);
            assertThat(node.toString(), d.type(), equalTo(Decision.Type.NO));
            assertThat(node.toString(), d.getExplanation(),
-                containsString("index has a preference for tiers [data_warm,data_cold], " +
+                containsString("index has a preference for tiers [data_warm, data_cold], " +


Seems like a nit, but I think we don't accept extra whitespace in this setting (we just use String#split to parse it) so I don't think we should include it when reporting it back to users either. If we do, someone will copy-paste it as-is and get a message saying invalid tier names found in [data_hot, data_warm] allowed values are [data_hot, data_warm, ... which is going to be pretty confusing.

(we should probably fix that error too, I guess almost nobody actually sets these things by hand or else we'd have had a report about it by now)

We wouldn't get here with an invalid tier name because of the setting validation that would catch it early. The validate logic that you mention in org.elasticsearch.cluster.routing.allocation.DataTier.DataTierSettingValidator#validate(java.lang.String) will throw an exception that has the original input string in the "invalid tier names found ". That's why I figured this was ok?

Heh this is exactly the confusion I mean: we're saying that this index has a tier preference of data_warm, data_cold (with a space) but that's not a valid value for the tier preference setting since we just split at commas, and data_cold (with a leading space) isn't the name of a tier.

Bah Github does overly clever stuff with whitespace so my previous comment makes no sense. Reformatting:

Heh this is exactly the confusion I mean: we're saying that this index has a tier preference of `data_warm, data_cold` (with a space) but that's not a valid value for the tier preference setting since we just split at commas, and ` data_cold` (with a leading space) isn't the name of a tier.

Fair point :) I made it use the old style formatting by concatenating strings again (could have used the setting value as well, but I figured this was less-error-prone/more-consistent on the off-chance that we made the setting buggy at some point) and added some early breakouts for the no-debug case so we don't get slower from that.

DaveCTurner · 2021-10-06T09:18:25Z

server/src/main/java/org/elasticsearch/cluster/routing/allocation/DataTier.java

@@ -115,6 +130,15 @@ public static boolean isFrozenNode(final Set<DiscoveryNodeRole> roles) {
        return roles.contains(DiscoveryNodeRole.DATA_FROZEN_NODE_ROLE) || roles.contains(DiscoveryNodeRole.DATA_ROLE);
    }

+    public static List<String> parseTierList(String tiers) {
+        if (Strings.hasText(tiers) == false) {


Bit weird that we treat all-whitespace as empty but otherwise we're whitespace-sensitive. (Acking that this is how it was before too, no action required)

DaveCTurner · 2021-10-06T09:26:39Z

server/src/main/java/org/elasticsearch/cluster/routing/allocation/DataTier.java

@@ -60,7 +76,6 @@ public static boolean validTierName(String tierName) {
    /**
     * Based on the provided target tier it will return a comma separated list of preferred tiers.
     * ie. if `data_cold` is the target tier, it will return `data_cold,data_warm,data_hot`
-     * This is usually used in conjunction with {@link DataTierAllocationDecider#TIER_PREFERENCE_SETTING}


Think we can keep this, the setting is still in scope here.

++ brought this back

u sure? I don't see it?

Now :) Sorry accidentally put this on the top level class.

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java

DaveCTurner · 2021-10-06T09:35:33Z

.../internalClusterTest/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierIT.java

@@ -47,7 +47,7 @@ public void testDefaultIndexAllocateToContent() {
        client().admin().indices().prepareCreate(index).setWaitForActiveShards(0).get();

        Settings idxSettings = client().admin().indices().prepareGetIndex().addIndices(index).get().getSettings().get(index);
-        assertThat(DataTierAllocationDecider.TIER_PREFERENCE_SETTING.get(idxSettings), equalTo(DataTier.DATA_CONTENT));
+        assertThat(DataTier.TIER_PREFERENCE_SETTING.get(idxSettings), equalTo(DataTier.DATA_CONTENT));


Maybe rename this test suite to DataTierAllocationDeciderIT since DataTier isn't in this package any more.

original-brownbear · 2021-10-06T10:00:32Z

Thanks David, all addressed :)

original-brownbear · 2021-10-06T10:50:13Z

@DaveCTurner alright explain format changes reverted now :) should be good for another look now I think.

henningandersen

Thanks Armin, this looks good overall. Left two comments on how to handle the field.

henningandersen · 2021-10-07T12:21:48Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java

+     * {@link String#hashCode()} works.
+     */
+    @Nullable // since lazy-loaded
+    private List<String> tierPreference;


I think we should this still mark this volatile to avoid unsafe publishing. Neither List.of nor string constructor promises to only have finalized fields and while it sort of looks like that is the case today, it could change in JDK updates. Also, a slight modification to DataTier.parseTierPrefence could cause this without anyone noticing.

The difference to String.hashCode is that it is a primitive with no references out (plus it is in the JDK so could special handle it if needed).

henningandersen · 2021-10-07T12:40:30Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java

@@ -574,6 +575,26 @@ public Settings getSettings() {
        return this.aliases;
    }

+    /**
+     * Lazy loaded cache for tier preference setting. We can't eager load this setting because
+     * {@link IndexMetadataVerifier#convertSharedCacheTierPreference(IndexMetadata)} might not have acted on this index yet and thus the


An alternative to lazy loading would be to store a null here if parsing the field fails when building and then parse the field every time when getTierPreference is called (or parse it to throw the exception and then if it succeeds throw another exception). That seems slightly better to me, since then the field is final.

++ I think I like this option best

Implemented :)

henningandersen

LGTM.

henningandersen · 2021-10-07T13:40:27Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java

+            List<String> tierPreference;
+            try {
+                tierPreference = DataTier.parseTierList(DataTier.TIER_PREFERENCE_SETTING.get(settings));
+            } catch (Exception e) {


I think this has to be an IllegalArgumentException so maybe we can catch that instead?

Technically yes, but we also call some other settings. I don't know if we want to bank on no changes to that?
I opted to be careful here mainly because this is a BwC thing to begin with and we could inadvertently introduce a bug like say some out of bounds exception with some specific old version of the setting and I wanted to avoid that. (we had a few similar BwC cases over the years that are hard to catch in tests up front)

OK, can we then assert that it is an IllegalArgumentException instead? Would like to avoid hiding some other exception from tests.

henningandersen · 2021-10-07T13:49:29Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java

+                // BwC hack: the setting failed validation but it will be fixed in
+                // #IndexMetadataVerifier#convertSharedCacheTierPreference(IndexMetadata)} later so we just store a null
+                // to be able to build a temporary instance
+                tierPreference = null;


Can we add a test that we can build an IndexMetadata object based on a settings object with an illegal _tier_preference?

henningandersen · 2021-10-07T13:52:26Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java

@@ -574,6 +580,15 @@ public Settings getSettings() {
        return this.aliases;
    }

+    public List<String> getTierPreference() {


Can we add a simple test that getTierPreference delivers the right output?

++ added both tests

original-brownbear · 2021-10-07T15:43:03Z

Jenkins run elasticsearch-ci/part-2 (unrelated + known)

original-brownbear · 2021-10-08T12:59:07Z

Thanks David + Henning!

The data tier preference is very expensive to parse out of the setting string repeatedly for large number of indices when using it in the data tier allocation decider. => as done with other index settings relevant to allocation, this commit moves the data tier preference to a field in `IndexMetadata`. The required moving the `DataTier` class itself to `server`. In a follow-up we can look into making the setting a list setting to remove the duplication around turning the string value into a list in various places.

* master: Fix DataTierTests package and add a validation test (elastic#78880) Fix split package org.elasticsearch.common.xcontent (elastic#78831) Store DataTier Preference directly on IndexMetadata (elastic#78668) [DOCS] Fixes typo in calendar API example (elastic#78867) Improve Node Shutdown Observability (elastic#78727) Convert encrypted snapshot license object to LicensedFeature (elastic#78731) Revert "Make nodePaths() singular (elastic#72514)" (elastic#78801) Fix incorrect generic type in PolicyStepsRegistry (elastic#78628) [DOCS] Fixes ML get calendars API (elastic#78808) Implement GET API for System Feature Upgrades (elastic#78642) [TEST] More MetadataStateFormat tests (elastic#78577) Add support for rest compatibility headers to the HLRC (elastic#78490) Un-ignoring tests after backporting fix (elastic#78830) Add node REPLACE shutdown implementation (elastic#76247) Wrap VersionPropertiesLoader in a BuildService to decouple build logic projects (elastic#78704) Adjust /_cat/templates not to request all metadata (elastic#78829) [DOCS] Fixes ML get scheduled events API (elastic#78809) Enable exit on out of memory error (elastic#71542) # Conflicts: # server/src/main/java/org/elasticsearch/cluster/metadata/DataStream.java

@OverRide

* upstream/master: (250 commits) [Transform] HLRC cleanups (elastic#78909) [ML] Make ML indices hidden when the node becomes master (elastic#77416) Introduce a Few Settings Singleton Instances (elastic#78897) Simplify TestCluster extraJar configuration (elastic#78837) Add @OverRide annotations to methods in EnrichPlugin class (elastic#76873) Add v7 restCompat for invalidating API key with the id field (elastic#78664) EQL: Refine repeatable queries (elastic#78895) Fix DataTierTests package and add a validation test (elastic#78880) Fix split package org.elasticsearch.common.xcontent (elastic#78831) Store DataTier Preference directly on IndexMetadata (elastic#78668) [DOCS] Fixes typo in calendar API example (elastic#78867) Improve Node Shutdown Observability (elastic#78727) Convert encrypted snapshot license object to LicensedFeature (elastic#78731) Revert "Make nodePaths() singular (elastic#72514)" (elastic#78801) Fix incorrect generic type in PolicyStepsRegistry (elastic#78628) [DOCS] Fixes ML get calendars API (elastic#78808) Implement GET API for System Feature Upgrades (elastic#78642) [TEST] More MetadataStateFormat tests (elastic#78577) Add support for rest compatibility headers to the HLRC (elastic#78490) Un-ignoring tests after backporting fix (elastic#78830) ... # Conflicts: # server/src/main/java/org/elasticsearch/ingest/IngestService.java # server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java

original-brownbear added >non-issue :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) v8.0.0 v7.16.0 labels Oct 5, 2021

elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Oct 5, 2021

original-brownbear added 3 commits October 5, 2021 21:41

fix test compile

66735e1

fix

cacac54

Merge remote-tracking branch 'elastic/master' into data-tiers-on-index

0c69999

original-brownbear requested review from DaveCTurner and henningandersen October 6, 2021 08:36

DaveCTurner reviewed Oct 6, 2021

View reviewed changes

original-brownbear added 2 commits October 6, 2021 11:45

Merge remote-tracking branch 'elastic/master' into data-tiers-on-index

3d3bcaa

Cr comments

e758bf3

original-brownbear requested a review from DaveCTurner October 6, 2021 10:00

original-brownbear added 3 commits October 6, 2021 12:33

old format

c13e2db

old format

5fd6f4e

old style messages

d9b2363

original-brownbear mentioned this pull request Oct 7, 2021

Fix Large Shard Count Scalability Issues #77466

Open

97 tasks

henningandersen reviewed Oct 7, 2021

View reviewed changes

original-brownbear added 3 commits October 7, 2021 14:51

Merge remote-tracking branch 'elastic/master' into data-tiers-on-index

3f381bb

final

ecb5f1f

Merge remote-tracking branch 'elastic/master' into data-tiers-on-index

59adab9

original-brownbear requested a review from henningandersen October 7, 2021 13:12

henningandersen approved these changes Oct 7, 2021

View reviewed changes

original-brownbear added 2 commits October 7, 2021 17:17

CR: add tests and assertion

9ae2ca7

Merge remote-tracking branch 'elastic/master' into data-tiers-on-index

e4ba16e

original-brownbear merged commit 7cb2d05 into elastic:master Oct 8, 2021

original-brownbear deleted the data-tiers-on-index branch October 8, 2021 12:59

original-brownbear mentioned this pull request Oct 8, 2021

Store DataTier Preference directly on IndexMetadata (#78668) #78874

Merged

joegallo mentioned this pull request Oct 8, 2021

Fix DataTierTests package and add a validation test #78880

Merged

joegallo mentioned this pull request Oct 12, 2021

Move DataTier.TIER_PREFERENCE_SETTING registration out of XPackPlugin #78995

Merged

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

original-brownbear mentioned this pull request Nov 3, 2021

[CI] SearchableSnapshotsRollingUpgradeIT.testMountPartialCopyAndRecoversCorrectly failing #79541

Closed

original-brownbear restored the data-tiers-on-index branch April 18, 2023 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store DataTier Preference directly on IndexMetadata #78668

Store DataTier Preference directly on IndexMetadata #78668

original-brownbear commented Oct 5, 2021

elasticmachine commented Oct 5, 2021

DaveCTurner left a comment

DaveCTurner Oct 6, 2021

original-brownbear Oct 6, 2021

DaveCTurner Oct 6, 2021

DaveCTurner Oct 6, 2021

original-brownbear Oct 6, 2021

DaveCTurner Oct 6, 2021

DaveCTurner Oct 6, 2021

original-brownbear Oct 6, 2021

DaveCTurner Oct 6, 2021

original-brownbear Oct 6, 2021

DaveCTurner Oct 6, 2021

original-brownbear Oct 6, 2021

original-brownbear commented Oct 6, 2021

original-brownbear commented Oct 6, 2021

henningandersen left a comment

henningandersen Oct 7, 2021

henningandersen Oct 7, 2021

original-brownbear Oct 7, 2021

original-brownbear Oct 7, 2021

henningandersen left a comment

henningandersen Oct 7, 2021

original-brownbear Oct 7, 2021

henningandersen Oct 7, 2021

original-brownbear Oct 7, 2021

henningandersen Oct 7, 2021

henningandersen Oct 7, 2021

original-brownbear Oct 7, 2021

original-brownbear commented Oct 7, 2021

original-brownbear commented Oct 8, 2021

Store DataTier Preference directly on IndexMetadata #78668

Store DataTier Preference directly on IndexMetadata #78668

Conversation

original-brownbear commented Oct 5, 2021

elasticmachine commented Oct 5, 2021

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear commented Oct 6, 2021

original-brownbear commented Oct 6, 2021

henningandersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henningandersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear commented Oct 7, 2021

original-brownbear commented Oct 8, 2021