-
Notifications
You must be signed in to change notification settings - Fork 24.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensuring that the ShrinkAction does not hang if total shards per node is too low #76732
Ensuring that the ShrinkAction does not hang if total shards per node is too low #76732
Conversation
Pinging @elastic/es-core-features (Team:Core/Features) |
This is related to this PR: #76134 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM, I left one minor comment, thanks for fixing this!
@@ -95,7 +96,8 @@ public void performAction(IndexMetadata indexMetadata, ClusterState clusterState | |||
|
|||
if (nodeId.isPresent()) { | |||
Settings settings = Settings.builder() | |||
.put(IndexMetadata.INDEX_ROUTING_REQUIRE_GROUP_SETTING.getKey() + "_id", nodeId.get()).build(); | |||
.put(IndexMetadata.INDEX_ROUTING_REQUIRE_GROUP_SETTING.getKey() + "_id", nodeId.get()) | |||
.put(ShardsLimitAllocationDecider.INDEX_TOTAL_SHARDS_PER_NODE_SETTING.getKey(), -1).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think rather than setting this explicitly to -1
, we might be better if we did:
.put(ShardsLimitAllocationDecider.INDEX_TOTAL_SHARDS_PER_NODE_SETTING.getKey(), -1).build(); | |
.putNull(ShardsLimitAllocationDecider.INDEX_TOTAL_SHARDS_PER_NODE_SETTING.getKey()).build(); |
Since the default value is -1 (rather than setting it to that explicitly)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Done.
… is too low (elastic#76732) We added configuration to AllocateAction to set the total shards per node property on the index. This makes it possible that a user could set this to a value lower than the total number of shards in the index that is about to be shrunk, meaning that all of the shards could not be moved to a single node in the ShrinkAction. This commit unsets the total shards per node property so that we fall back to the default value (-1, unlimited) in the ShrinkAction to avoid this. Relates to elastic#44070
… is too low (#76732) (#76780) This is a backport of #76732. We added configuration to AllocateAction to set the total shards per node property on the index. This makes it possible that a user could set this to a value lower than the total number of shards in the index that is about to be shrunk, meaning that all of the shards could not be moved to a single node in the ShrinkAction. This commit unsets the total shards per node property so that we fall back to the default value (-1, unlimited) in the ShrinkAction to avoid this. Relates to #44070
We added configuration to AllocateAction to set the total shards per node property on the index. This makes it possible that a user could set this to a value lower than the total number of shards in the index that is about to be shrunk, meaning that all of the shards could not be moved to a single node in the ShrinkAction. This commit sets the total shards per node property to -1 (unlimited) in the ShrinkAction to avoid this.
Relates to #44070