Take into account expectedShardSize when initializing shard in simulation #95734
Conversation
This file contains hidden or 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 Indexing/Distributed idegtiarenko added Team:Distributed (Obsolete) 
| Pinging @elastic/es-distributed (Team:Distributed) |
DaveCTurner approved these changes May 2, 2023
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
3 participants
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.