ILM: Add total_shards_per_node setting to searchable snapshot #112972

samxbr · 2024-09-16T21:20:15Z

Allows setting index total_shards_per_node in the SearchableSnapshot action of ILM to remediate hot spot in shard allocation for searchable snapshot index.

Closes #112261

Closes elastic#112261

elasticsearchmachine · 2024-09-16T22:40:31Z

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

elasticsearchmachine · 2024-09-16T22:40:32Z

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

dakrone

Thanks for working on this Sam! I left some comments, but nothing major, this is pretty close!

docs/reference/ilm/actions/ilm-searchable-snapshot.asciidoc

dakrone · 2024-09-18T22:46:09Z

docs/reference/ilm/actions/ilm-searchable-snapshot.asciidoc

          "searchable_snapshot" : {
-            "snapshot_repository" : "backing_repo"
+            "snapshot_repository" : "backing_repo",
+            "total_shards_per_node" : 2


I think this is a pretty power-user sort of feature, so I think it'd be best not to have it in the example (I worry about unthinking copy-and-paste)

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

dakrone · 2024-09-18T22:49:46Z

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

        this.forceMergeIndex = forceMergeIndex;
+
+        if (totalShardsPerNode != null && totalShardsPerNode < -1) {
+            throw new IllegalArgumentException("[" + TOTAL_SHARDS_PER_NODE.getPreferredName() + "] must be >= -1");


Suggested change

throw new IllegalArgumentException("[" + TOTAL_SHARDS_PER_NODE.getPreferredName() + "] must be >= -1");

throw new IllegalArgumentException("[" + TOTAL_SHARDS_PER_NODE.getPreferredName() + "] must be >= 1");

dakrone · 2024-09-18T22:50:14Z

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

        this.snapshotRepository = snapshotRepository;
        this.forceMergeIndex = forceMergeIndex;
+
+        if (totalShardsPerNode != null && totalShardsPerNode < -1) {


Suggested change

if (totalShardsPerNode != null && totalShardsPerNode < -1) {

if (totalShardsPerNode != null && totalShardsPerNode < 1) {

(since we want to disallow -1 and 0)

dakrone · 2024-09-18T22:54:14Z

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicyTests.java

                    Collections.singletonMap(
                        SearchableSnapshotAction.NAME,
-                        new SearchableSnapshotAction(randomAlphaOfLength(10), randomBoolean())
+                        new SearchableSnapshotAction(randomAlphaOfLength(10), randomBoolean(), randomIntBetween(-1, 100))


0 is not a valid setting (it would be like saying "you can allocate zero copies of the data to the node"), so I think this would be better as something like:

Suggested change

new SearchableSnapshotAction(randomAlphaOfLength(10), randomBoolean(), randomIntBetween(-1, 100))

new SearchableSnapshotAction(randomAlphaOfLength(10), randomBoolean(), (randomBoolean() ? null : randomIntBetween(1, 100)))

dakrone · 2024-09-18T22:55:18Z

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/MountSnapshotStepTests.java

    }
+
+    private Integer randomTotalShardsPerNode() {
+        return randomIntBetween(-1, 100);


I would suggest encapsulating the nullability (and fixing the bounds) with:

Suggested change

return randomIntBetween(-1, 100);

return randomBoolean() ? null : randomIntBetween(1, 100);

Co-authored-by: Lee Hinman <[email protected]>

…ilm/MountSnapshotStep.java Co-authored-by: Lee Hinman <[email protected]>

samxbr · 2024-09-20T19:06:31Z

CI tests failing due to #113296

samxbr · 2024-09-20T19:23:34Z

@elasticmachine update branch

dakrone

LGTM, I left one more comment, thanks Sam!

dakrone · 2024-09-20T20:01:22Z

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

        this.restoredIndexPrefix = restoredIndexPrefix;
        this.storageType = Objects.requireNonNull(storageType, "a storage type must be specified");
+        if (totalShardsPerNode != null && totalShardsPerNode < 1) {
+            throw new IllegalArgumentException("[totalShardsPerNode] must be >= 1");


I think this should use the preferred name (and to match the config flag name), something like:

Suggested change

throw new IllegalArgumentException("[totalShardsPerNode] must be >= 1");

throw new IllegalArgumentException("[" + SearchableSnapshotAction.TOTAL_SHARDS_PER_NODE.getPreferredName() + "] must be >= 1");

javanna · 2024-09-24T17:06:29Z

Double checking: was this change supposed to go to main only? Or should it have been backported to 8.x?

samxbr · 2024-09-24T17:53:08Z

Thanks @javanna for the reminder, I added backport for 8.16.

javanna · 2024-09-24T18:59:58Z

@samxbr I'm afraid you'll have to backport manually, because the PR has already been merged.

…c#112972) Allows setting index total_shards_per_node in the SearchableSnapshot action of ILM to remediate hot spot in shard allocation for searchable snapshot index. Closes elastic#112261

… (#113493) Allows setting index total_shards_per_node in the SearchableSnapshot action of ILM to remediate hot spot in shard allocation for searchable snapshot index. Closes #112261

samxbr added 2 commits September 16, 2024 17:14

ILM: Add total_shards_per_node setting to searchable snapshot

4b035ae

Closes elastic#112261

clean up

febc50d

elasticsearchmachine added the v9.0.0 label Sep 16, 2024

fix spotless check

7c18216

samxbr changed the title ~~Feature/add searchable snapshot setting~~ ILM: Add total_shards_per_node setting to searchable snapshot Sep 16, 2024

samxbr added >feature :Data Management/ILM+SLM Index and Snapshot lifecycle management >enhancement and removed >feature labels Sep 16, 2024

samxbr marked this pull request as ready for review September 16, 2024 22:39

samxbr requested a review from a team September 16, 2024 22:40

elasticsearchmachine added the Team:Data Management Meta label for data/management team label Sep 16, 2024

Update docs/changelog/112972.yaml

2ae4aa1

Merge branch 'main' into feature/add-searchable-snapshot-setting

7ded0db

dakrone requested changes Sep 18, 2024

View reviewed changes

samxbr and others added 6 commits September 19, 2024 20:11

Merge branch 'main' into feature/add-searchable-snapshot-setting

6304b1b

Update docs/reference/ilm/actions/ilm-searchable-snapshot.asciidoc

debe05b

Co-authored-by: Lee Hinman <[email protected]>

Update x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/…

776c8ee

…ilm/MountSnapshotStep.java Co-authored-by: Lee Hinman <[email protected]>

address comment

e458d0c

Merge branch 'main' into feature/add-searchable-snapshot-setting

a8bcc83

fix test

01679a4

Merge branch 'main' into feature/add-searchable-snapshot-setting

15cbad0

dakrone approved these changes Sep 20, 2024

View reviewed changes

samxbr added 2 commits September 23, 2024 12:11

Update error msg

92ba2bb

Merge branch 'main' into feature/add-searchable-snapshot-setting

28e28ce

samxbr merged commit 80dd563 into elastic:main Sep 23, 2024
15 checks passed

samxbr added v8.16.0 auto-backport Automatically create backport pull requests when merged labels Sep 24, 2024

dakrone mentioned this pull request Dec 9, 2024

total_shards_per_node can prevent cold phase searchable snapshots from mounting #115479

Closed

	throw new IllegalArgumentException("[" + TOTAL_SHARDS_PER_NODE.getPreferredName() + "] must be >= -1");
	throw new IllegalArgumentException("[" + TOTAL_SHARDS_PER_NODE.getPreferredName() + "] must be >= 1");

	if (totalShardsPerNode != null && totalShardsPerNode < -1) {
	if (totalShardsPerNode != null && totalShardsPerNode < 1) {

	new SearchableSnapshotAction(randomAlphaOfLength(10), randomBoolean(), randomIntBetween(-1, 100))
	new SearchableSnapshotAction(randomAlphaOfLength(10), randomBoolean(), (randomBoolean() ? null : randomIntBetween(1, 100)))

	return randomIntBetween(-1, 100);
	return randomBoolean() ? null : randomIntBetween(1, 100);

	throw new IllegalArgumentException("[totalShardsPerNode] must be >= 1");
	throw new IllegalArgumentException("[" + SearchableSnapshotAction.TOTAL_SHARDS_PER_NODE.getPreferredName() + "] must be >= 1");

ILM: Add total_shards_per_node setting to searchable snapshot #112972

ILM: Add total_shards_per_node setting to searchable snapshot #112972

Uh oh!

Conversation

samxbr commented Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Sep 16, 2024

Uh oh!

elasticsearchmachine commented Sep 16, 2024

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dakrone Sep 18, 2024

Choose a reason for hiding this comment

Uh oh!

samxbr Sep 20, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dakrone Sep 18, 2024

Choose a reason for hiding this comment

Uh oh!

dakrone Sep 18, 2024

Choose a reason for hiding this comment

Uh oh!

dakrone Sep 18, 2024

Choose a reason for hiding this comment

Uh oh!

dakrone Sep 18, 2024

Choose a reason for hiding this comment

Uh oh!

samxbr commented Sep 20, 2024

Uh oh!

samxbr commented Sep 20, 2024

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

dakrone Sep 20, 2024 • 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!

javanna commented Sep 24, 2024

Uh oh!

samxbr commented Sep 24, 2024

Uh oh!

javanna commented Sep 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

samxbr commented Sep 16, 2024 •

edited

Loading

dakrone Sep 20, 2024 •

edited

Loading