Take into account expectedShardSize when initializing shard in simulation #95734
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
idegtiarenko
added
>enhancement
:Distributed/Distributed
idegtiarenko
added
Team:Distributed
|
Pinging @elastic/es-distributed (Team:Distributed) |
DaveCTurner
approved these changes
May 2, 2023
| // initializing new (empty) primary | ||
| return 0L; | ||
| // initializing new (empty?) primary | ||
| return Math.max(0L, shard.getExpectedShardSize()); |
There was a problem hiding this comment.
nit: I guess there's no need for a max here since the caller ignores negative values.
Suggested change
| return Math.max(0L, shard.getExpectedShardSize()); | |
| return shard.getExpectedShardSize(); |
…sage This change asserts that produced balance does not utilize all disk space.
idegtiarenko
force-pushed
the
assert_balanced_disk_usage
branch
from
eaca950
to
5aeb4d9
Compare
idegtiarenko
commented
May 2, 2023
| if (replicaNodeId != null) { | ||
| dataPath.put(new NodeAndShard(replicaNodeId, shardId), "/data"); | ||
| usedDiskSpace.compute(primaryNodeId, (k, v) -> v + thisShardSize); | ||
| usedDiskSpace.compute(replicaNodeId, (k, v) -> v + thisShardSize); |
There was a problem hiding this comment.
This was causing some of the assertions failing
|
Hi @idegtiarenko, I've created a changelog YAML for you. |
idegtiarenko
commented
May 3, 2023
| var deviation = randomIntBetween(0, 50) - 100L; | ||
| return originalSize * (1000 + deviation) / 1000; | ||
| var deviation = randomIntBetween(0, 10) - 5; | ||
| return originalSize * (100 + deviation) / 100; |
There was a problem hiding this comment.
Previously this was only producing shards that are 5-10% smaller then original size, while this should generate -5..+5%
DaveCTurner
approved these changes
May 3, 2023
| } | ||
|
|
||
| private static long smallShardSizeDeviation(long originalSize) { | ||
| var deviation = randomIntBetween(0, 50) - 100L; | ||
| return originalSize * (1000 + deviation) / 1000; | ||
| var deviation = randomIntBetween(0, 10) - 5; |
There was a problem hiding this comment.
nit/suggestion:
Suggested change
| var deviation = randomIntBetween(0, 10) - 5; | |
| var deviation = randomIntBetween(-5, 5); |
or maybe even drop the 100 + and do this:
Suggested change
| var deviation = randomIntBetween(0, 10) - 5; | |
| var deviation = randomIntBetween(95, 105); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change takes into account expectedShardSize when simulating new shard initialization during desired balance computation.
This should reduce balance movements when restoring a shard from snapshot as it has known size beforehand.