Support max_single_primary_size in Resize Action and exposed in ILM #67705

gaobinlong · 2021-01-19T14:51:14Z

Relates to #65714.

The main changes are:

Add a max_single_primary_size parameter in Resize Action, it's used to calculate an appropriate shards number of the target index when executing the Shrink API.
Expose the max_single_primary_size parameter in ILM's Shrink Action.
Support the parameter in high level rest client.
Add some description for the parameter in the documents.
Add some tests for the code change.

… ILM

elasticmachine · 2021-01-19T15:28:39Z

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

dakrone

Thanks for opening this @gaobinlong, I took a look and left some initial thoughts.

I also think we should use max_single_shard_size, because for this particular application, the difference between primaries and replicas is irrelevant (shrink only deals with primary shards anyway), and it makes it a bit shorter and easier to read.

dakrone · 2021-01-19T23:28:31Z

server/src/main/java/org/elasticsearch/action/admin/indices/shrink/TransportResizeAction.java

+                    if (minShardsNum > sourceIndexShardsNum) {
+                        throw new IllegalArgumentException("The target index's shards number[" + minShardsNum +
+                            "] is greater than the source index's shards number[" + sourceIndexShardsNum + "]");
+                    }


I wonder if this should throw an error, or if it would be better to allow the shrink with the same number of shards as are currently available (basically min(sourceIndexShardsNum, minShardsNum))

I am on the fence about this, since it can make the ILM usage almost useless if time-based rollover is used (imagine a period of high ingestion where an index with 2 primary shards has trouble initially rolling over, but finally does at 200gb, and then tries to shrink to 50gb shards and gets stuck in an error forever because it's trying to "shrink" to 4 shards)

That's a problem, I have set the target index's shards number to sourceIndexShardsNum if minShardsNum is greater than sourceIndexShardsNum, I have also added a log in the code and some description in the document about this.

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

dakrone · 2021-01-19T23:40:32Z

server/src/main/java/org/elasticsearch/action/admin/indices/shrink/ResizeRequest.java

    }

    private CreateIndexRequest targetIndexRequest;
    private String sourceIndex;
    private ResizeType type = ResizeType.SHRINK;
    private Boolean copySettings = true;
+    private ByteSizeValue maxSinglePrimaryShardSize;


This class is missing serialization of this value (with version checks) in the StreamInput constructor and writeTo method

I also notice this class is missing a hashCode and equals implementation, which means that we can never compare them. We should remedy that, but it can be done in subsequent work (doesn't have to be done in this PR)

(this is not caught because the tests need to be enhanced to check for transport serialization, that can be done as the subsequent work also)

I have added some serialization code in the StreamInput constructor and writeTo method yet, and
I'm glad to open another PR to add the missing hashCode and equals method.

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

server/src/main/java/org/elasticsearch/action/admin/indices/shrink/TransportResizeAction.java

joegallo · 2021-01-25T15:21:33Z

@dakrone @gaobinlong should we rename this to max_single_primary_size for consistency with #63026 / #67842 (or alternatively, rename things over there to match here...)?

dakrone · 2021-01-26T16:51:15Z

Yes I think we should rename it to max_single_primary_size so that it matches, consistency is very nice to have. Thanks for mentioning that @joegallo.

gaobinlong · 2021-01-27T02:05:46Z

OK, I will rename the parameter to max_single_primary_size soon.

gaobinlong · 2021-01-27T08:46:27Z

@dakrone ,I've renamed the parameter to max_single_primary_size yet, can you take a look at the change?

dakrone

Thanks for working on this @gaobinlong, this looks much closer, I left some more comments on this! (hopefully my suggestions compile, I did not test them so they may need to be checked)

docs/reference/ilm/actions/ilm-shrink.asciidoc

dakrone · 2021-01-28T22:26:16Z

docs/reference/ilm/actions/ilm-shrink.asciidoc

+The max single primary shard size of the target index, it's used to find an optimum shards number of the target index.
+When this parameter is set properly, each shard's storage of the target index will not be greater than the parameter,
+while the shards number of the target index still be a factor of the source index's shards number, but if the parameter
+is less than the single shard size of the source index, the shards number of the target index will be equal to the source index's shards number.
+For example, when this parameter is set to `50GB`, if the source index has `60` primary shards with `100GB` storage, then the
+target index will have `2` primary shards, each shard's storage is `50GB`; if the source index has `60` primary shards
+with `1000GB` storage, then the target index will have `20` primary shards; if the source index has `60` primary shards
+with `4000GB` storage, then the target index will still have `60` primary shards. This parameter is conflict
+with `number_of_shards` in the `settings` parameter, either of them can be set.


Suggested change

The max single primary shard size of the target index, it's used to find an optimum shards number of the target index.

When this parameter is set properly, each shard's storage of the target index will not be greater than the parameter,

while the shards number of the target index still be a factor of the source index's shards number, but if the parameter

is less than the single shard size of the source index, the shards number of the target index will be equal to the source index's shards number.

For example, when this parameter is set to `50GB`, if the source index has `60` primary shards with `100GB` storage, then the

target index will have `2` primary shards, each shard's storage is `50GB`; if the source index has `60` primary shards

with `1000GB` storage, then the target index will have `20` primary shards; if the source index has `60` primary shards

with `4000GB` storage, then the target index will still have `60` primary shards. This parameter is conflict

with `number_of_shards` in the `settings` parameter, either of them can be set.

The max single primary shard size for the target index. Used to find the optimum number of shards for the target index.

When this parameter is set, each shard's storage in the target index will not be greater than the parameter.

The shards count of the target index will still be a factor of the source index's shards count, but if the parameter

is less than the single shard size in the source index, the shards count for the target index will be equal to the source index's shard count.

For example, when this parameter is set to 50gb, if the source index has 60 primary shards with totaling 100gb, then the

target index will have 2 primary shards, with each shard size of 50gb; if the source index has 60 primary shards

totaling 1000gb, then the target index will have 20 primary shards; if the source index has 60 primary shards

totaling 4000gb, then the target index will still have 60 primary shards. This parameter conflicts

with `number_of_shards` in the `settings`, only one of them may be set.

dakrone · 2021-01-28T22:26:47Z

docs/reference/ilm/actions/ilm-shrink.asciidoc

+      "warm": {
+        "actions": {
+          "shrink" : {
+            "max_single_primary_size": "50GB"


Suggested change

"max_single_primary_size": "50GB"

"max_single_primary_size": "50gb"

dakrone · 2021-01-28T22:27:49Z

docs/reference/indices/shrink-index.asciidoc

+The max single primary shard size of the target index, it's used to find an optimum shards number of the target index.
+When this parameter is set properly, each shard's storage of the target index will not be greater than the parameter,
+while the shards number of the target index still be a factor of the source index's shards number, but if the parameter
+is less than the single shard size of the source index, the shards number of the target index will be equal to the source index's shards number.
+For example, when this parameter is set to `50GB`, if the source index has `60` primary shards with `100GB` storage, then the
+target index will have `2` primary shards, each shard's storage is `50GB`; if the source index has `60` primary shards
+with `1000GB` storage, then the target index will have `20` primary shards; if the source index has `60` primary shards
+with `4000GB` storage, then the target index will still have `60` primary shards. This parameter is conflict
+with `number_of_shards` in the `settings` parameter, either of them can be set.


For this I think you can copy the changes made in the previous doc (rather than duplicate them here)

dakrone · 2021-01-28T22:30:42Z

server/src/main/java/org/elasticsearch/action/admin/indices/shrink/ResizeRequest.java

+            boolean hasMaxSinglePrimarySize = maxSinglePrimarySize != null;
+            out.writeBoolean(hasMaxSinglePrimarySize);
+            if (hasMaxSinglePrimarySize) {
+                maxSinglePrimarySize.writeTo(out);
+            }


This can be simplified to

Suggested change

boolean hasMaxSinglePrimarySize = maxSinglePrimarySize != null;

out.writeBoolean(hasMaxSinglePrimarySize);

if (hasMaxSinglePrimarySize) {

maxSinglePrimarySize.writeTo(out);

}

out.writeOptionalWriteable(maxSinglePrimarySize);

dakrone · 2021-01-28T22:35:20Z

server/src/main/java/org/elasticsearch/action/admin/indices/shrink/TransportResizeAction.java

+                        logger.info("By setting max_single_primary_size to [" + maxSinglePrimarySize.toString() +
+                            "], the target index [" + targetIndexName + "] will contain [" + minShardsNum +
+                            "] shards, which will be greater than [" + sourceIndexShardsNum +
+                            "] shards of the source index [" + sourceMetadata.getIndex().getName() +
+                            "], use [" + sourceIndexShardsNum + "] as the shards number of the target index [" + targetIndexName + "]");


This can be cleaned up using the {} substitution

Suggested change

logger.info("By setting max_single_primary_size to [" + maxSinglePrimarySize.toString() +

"], the target index [" + targetIndexName + "] will contain [" + minShardsNum +

"] shards, which will be greater than [" + sourceIndexShardsNum +

"] shards of the source index [" + sourceMetadata.getIndex().getName() +

"], use [" + sourceIndexShardsNum + "] as the shards number of the target index [" + targetIndexName + "]");

logger.info("By setting max_single_primary_size to [{}], the target index [{}] will contain [{}] shards, which will be greater than [{}] shards in the source index [{}], using [{}] for the shard count of the target index [{}]",

maxSinglePrimarySize.toString(), targetIndexName, minShardsNum, sourceIndexShardsNum, sourceMetadata.getIndex().getName(), sourceIndexShardsNum, targetIndexName);

dakrone · 2021-01-28T22:38:33Z

@elasticmachine test this please

dakrone · 2021-01-29T18:59:23Z

@elasticmachine ok to test

dakrone · 2021-01-29T19:12:46Z

Thanks for iterating on this @gaobinlong! It looks good to me!

dakrone · 2021-01-29T19:29:08Z

@elasticmachine update branch

joegallo · 2021-02-01T17:54:50Z

7.x backport in #68321

…67705) (#68321) Co-authored-by: bellengao <[email protected]>

Adjust ResizeRequest and ShrinkAction serialization now that elastic#68321 has been merged, and re-enable BWC testing. Related to elastic#67705, elastic#68321, and elastic#68329

Adjust ResizeRequest and ShrinkAction serialization now that #68321 has been merged, and re-enable BWC testing. Related to #67705, #68321, and #68329

Support max_single_primary_shard_size in Resize Action and exposed in…

29fc9f9

… ILM

dakrone added :Data Management/ILM+SLM Index and Snapshot lifecycle management >feature v7.12.0 v8.0.0 labels Jan 19, 2021

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

dakrone self-requested a review January 19, 2021 15:28

dakrone requested changes Jan 19, 2021

View reviewed changes

dakrone reviewed Jan 19, 2021

View reviewed changes

server/src/main/java/org/elasticsearch/action/admin/indices/shrink/TransportResizeAction.java Outdated Show resolved Hide resolved

bellengao added 4 commits January 21, 2021 13:15

merge origin/master

8139c5f

modify parameter name

68297ae

do not return error if max_single_shard_size is too small

2c23a98

Merge remote-tracking branch 'origin/master' into resize_enhancement

d1f6737

bellengao added 3 commits January 27, 2021 10:10

Merge remote-tracking branch 'origin/master' into resize_enhancement

454a9a1

Rename parameter

97adec9

Merge remote-tracking branch 'origin/master' into resize_enhancement

6943ddf

dakrone self-requested a review January 28, 2021 22:04

dakrone requested changes Jan 28, 2021

View reviewed changes

bellengao added 2 commits January 29, 2021 22:13

optimize some code and doc

43c55ed

Merge remote-tracking branch 'origin/master' into resize_enhancement

e8036d8

dakrone approved these changes Jan 29, 2021

View reviewed changes

dakrone assigned joegallo Jan 29, 2021

Merge branch 'master' into resize_enhancement

675133e

joegallo changed the title ~~Support max_single_primary_shard_size in Resize Action and exposed in ILM~~ Support max_single_primary_size in Resize Action and exposed in ILM Jan 29, 2021

joegallo merged commit d69c033 into elastic:master Jan 29, 2021

dakrone added the backport pending label Feb 1, 2021

joegallo mentioned this pull request Feb 1, 2021

[7.x] Support max_single_primary_size in Resize Action and exposed in ILM #68321

Merged

joegallo added a commit that referenced this pull request Feb 1, 2021

Support max_single_primary_size in Resize Action and exposed in ILM (#…

4fed1a5

…67705) (#68321) Co-authored-by: bellengao <[email protected]>

joegallo mentioned this pull request Feb 1, 2021

Adjust serialization version and re-enable BWC #68335

Merged

joegallo added a commit that referenced this pull request Feb 1, 2021

Adjust serialization version and re-enable BWC (#68335)

14a6bb4

Adjust ResizeRequest and ShrinkAction serialization now that #68321 has been merged, and re-enable BWC testing. Related to #67705, #68321, and #68329

joegallo removed the backport pending label Feb 1, 2021

gaobinlong mentioned this pull request Feb 4, 2021

Enhance and add more tests for ResizeRequest #68502

Merged

joegallo mentioned this pull request Feb 9, 2021

Add max_single_primary_size to ResizeRequest's toXContent #68793

Merged

joegallo mentioned this pull request Feb 18, 2021

Rename max_single_primary_size to max_primary_shard_size #69239

Merged

stevejgordon mentioned this pull request Feb 22, 2021

7.12.0 Meta Ticket elastic/elasticsearch-net#5337

Closed

34 tasks

joegallo mentioned this pull request May 4, 2021

Enhance shrink action in ILM to specify max single primary shard size #65714

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

joegallo mentioned this pull request May 18, 2022

Fix max_primary_shard_size resize factor math #86897

Merged

	"max_single_primary_size": "50GB"
	"max_single_primary_size": "50gb"

Support max_single_primary_size in Resize Action and exposed in ILM #67705

Support max_single_primary_size in Resize Action and exposed in ILM #67705

Uh oh!

Conversation

gaobinlong commented Jan 19, 2021 • edited by joegallo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jan 19, 2021

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!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

joegallo commented Jan 25, 2021

Uh oh!

dakrone commented Jan 26, 2021

Uh oh!

gaobinlong commented Jan 27, 2021

Uh oh!

gaobinlong commented Jan 27, 2021

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

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 commented Jan 28, 2021

Uh oh!

dakrone commented Jan 29, 2021

Uh oh!

dakrone commented Jan 29, 2021

Uh oh!

dakrone commented Jan 29, 2021

Uh oh!

joegallo commented Feb 1, 2021

Uh oh!

Uh oh!

gaobinlong commented Jan 19, 2021 •

edited by joegallo

Loading