Followup on comments from #91612 #91700
Merged
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
>non-issue
:Distributed Coordination/Allocation
idegtiarenko
changed the title
|
Pinging @elastic/es-distributed (Team:Distributed) |
fcofdez
reviewed
Nov 18, 2022
|
|
||
| var settings = Settings.EMPTY; | ||
| var settings = Settings.builder() | ||
| .put(ThrottlingAllocationDecider.CLUSTER_ROUTING_ALLOCATION_NODE_CONCURRENT_RECOVERIES_SETTING.getKey(), 10) |
There was a problem hiding this comment.
Could you add a comment explaining why those settings are necessary here?
fcofdez
reviewed
Nov 18, 2022
| var metadataBuilder = Metadata.builder(); | ||
| var routingTableBuilder = RoutingTable.builder(); | ||
| for (var index : indices) { | ||
| var inSyncId = UUIDs.randomBase64UUID(random()); | ||
| var build = index.putInSyncAllocationIds(0, Set.of(inSyncId)).build(); | ||
| var build = index.settings(settings(Version.CURRENT).put("index.routing.allocation.exclude._id", "node-3")) |
There was a problem hiding this comment.
is this setting used intentionally here?
There was a problem hiding this comment.
Yes, It forces the shards to be rebalanced by a followup allocate call, I would add a comment
fcofdez
reviewed
Nov 18, 2022
| @@ -1024,7 +1024,6 @@ public void testDoNotRebalanceToTheNodeThatNoLongerExists() { | |||
| .put(SETTING_NUMBER_OF_REPLICAS, 0) | |||
| .put(SETTING_INDEX_VERSION_CREATED.getKey(), Version.CURRENT) | |||
| ) | |||
| .system(randomBoolean()) | |||
There was a problem hiding this comment.
Is this change intentional?
There was a problem hiding this comment.
Yes, this is unnecessary in that test
fcofdez
approved these changes
Nov 18, 2022
DaveCTurner
reviewed
Nov 21, 2022
test/framework/src/main/java/org/elasticsearch/cluster/ESAllocationTestCase.java
Show resolved
Hide resolved
| @@ -95,93 +88,74 @@ public void testDecideShardAllocation() { | |||
|
|
|||
| public void testBalanceByShardLoad() { | |||
There was a problem hiding this comment.
Naming nit:
Suggested change
| public void testBalanceByShardLoad() { | |
| public void testBalanceByWriteLoad() { |
| } | ||
| } | ||
| assertThat(nodeIngestLoad, lessThanOrEqualTo(8.0 + 1.5)); | ||
| assertThat(nodeIngestLoad, lessThanOrEqualTo(8.0)); |
There was a problem hiding this comment.
Implicitly this asserts that heavy-index is on a different node from light-index-* but could we be more explicit about that? Can we also assert that zero-write-load-index and no-write-load-index end up on the same node as heavy-index because of shard balance?
| var indexMetadata = clusterState.metadata().index(shardRouting.index()); | ||
| nodeDiskUsage += indexMetadata.getForecastedShardSizeInBytes().orElse(0L); | ||
| } | ||
| } | ||
| assertThat(nodeDiskUsage, lessThanOrEqualTo(ByteSizeValue.ofGb(8 + 1).getBytes())); | ||
| assertThat(nodeDiskUsage, lessThanOrEqualTo(ByteSizeValue.ofGb(8).getBytes())); |
DaveCTurner
approved these changes
Nov 21, 2022
| @@ -95,93 +88,74 @@ public void testDecideShardAllocation() { | |||
|
|
|||
| public void testBalanceByShardLoad() { | |||
Comment on lines
349
to
361
| // IndexRoutingTable.builder(build.getIndex()) | ||
| // .addShard( | ||
| // TestShardRouting.newShardRouting( | ||
| // new ShardId(build.getIndex(), 0), | ||
| // // allocate indices on excluded node so that they are rebalanced according to the weights later on in test | ||
| // "node-3", | ||
| // null, | ||
| // true, | ||
| // ShardRoutingState.STARTED, | ||
| // AllocationId.newInitializing(inSyncId) | ||
| // ) | ||
| // ) | ||
| // ); |
There was a problem hiding this comment.
Could we remove this commented-out code?
idegtiarenko
added a commit
to idegtiarenko/elasticsearch
that referenced
this pull request
Nov 21, 2022
elasticsearchmachine
pushed a commit
that referenced
this pull request
Nov 21, 2022
* New balancer unit tests (#91612) (cherry picked from commit 7c39880) * Add tests to BalancedShardsAllocator.Balancer#getIndexDiskUsageInBytes (#91642) This commit includes minor cleanups (cherry picked from commit 6cff05b) * Simulate balancer in realistic cluster (#91404) This commit adds a test suite which creates a somewhat realistic cluster and runs the shards allocator until convergence, then reports on the balance quality of the resulting allocation. This is useful for exploring changes to the balancer (although such experiments may want to increase the size of the cluster somewhat) and by running it in CI we can at least be sure that the balancer doesn't throw anything unexpected and does eventually converge in these situations. * Followup on comments from #91612 (#91700) Co-authored-by: Francisco Fernández Castaño <[email protected]> Co-authored-by: David Turner <[email protected]>
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.
Followup on comments from #91612