-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Add expectedShardSize
to ShardRouting and use it in path.data allocation
#12947
Conversation
@@ -54,6 +54,11 @@ public Long getShardSize(ShardRouting shardRouting) { | |||
return shardSizes.get(shardIdentifierFromRouting(shardRouting)); | |||
} | |||
|
|||
public long getShardSize(ShardRouting shardRouting, long defaultValue) { |
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.
Instead of passing defaultValue here, can we just return ShardRouting.UNAVAILABLE_EXPECTED_SHARD_SIZE? And maybe move that constant here?
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.
others use thsi as well and they need 0
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.
Can't they check the constant and do their own "use 0 instead"?
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.
Actually, thinking about it more ... I think I like the required defaultValue arg: it avoids sneaky bugs when the caller fails to check for the -1.
I left a couple comments, and I don't understand all the places where you had to punch expectedShardSize through, but big +1 to sending expectedShardSize to the node so it can make a more informed decision when it has multiple path.data. |
@s1monw what happens in this scenario: A node has three data paths and one shard, located on the data path with least capacity. The shard grows to fill 99% of its data path... |
it blows up - this is out of scope in this PR that's a problem of the disk-allocation decider and needs to be fixed in a different PR |
if nobody objects I will push this in the next 72 hours |
…ation Today we only guess how big the shard will be that we are allocating on a node. Yet, we have this information on the master but it's not available on the data nodes when we pick a data path for the shard. We use some rather simple heuristic based on existing shard sizes on this node which might be complete bogus. This change adds the expected shard size to the ShardRouting for RELOCATING and INITIALIZING shards to be used on the actual node to find the best data path for the shard. Closes elastic#11271
173f0c8
to
3dd6c4a
Compare
Add `expectedShardSize` to ShardRouting and use it in path.data allocation
@s1monw I can only find this commit in master, not in the 2.0 branch, is it expected? |
Add `expectedShardSize` to ShardRouting and use it in path.data allocation
@jpountz I ported it just now... wanted to give it some CI time first |
Today we only guess how big the shard will be that we are allocating on a node.
Yet, we have this information on the master but it's not available on the data nodes
when we pick a data path for the shard. We use some rather simple heuristic based on
existing shard sizes on this node which might be complete bogus. This change adds
the expected shard size to the ShardRouting for RELOCATING and INITIALIZING shards
to be used on the actual node to find the best data path for the shard.
Closes #11271