Faster access to INITIALIZING/RELOCATING shards #47817

kkewwei · 2019-10-09T17:21:53Z

Today a couple of allocation deciders iterate through all the shards on a node
to find the INITIALIZING or RELOCATING ones, and this can slow down cluster
state updates in clusters with very high-density nodes holding many thousands
of shards even if those shards belong to closed or frozen indices. This commit
pre-computes the sets of INITIALIZING and RELOCATING shards to speed up
this search.

Closes #46941
Relates #48579

Co-authored-by: "hongju.xhj" hongju.xhj@alibaba-inc.com

…e cost time of updating ClusterState

elasticmachine · 2019-10-15T19:21:00Z

Pinging @elastic/es-distributed (:Distributed/Allocation)

DaveCTurner

Thanks @kkewwei, I left a few minor requests inline. I also think we should add a method bool invariant() and call assert invariant() at the end of the constructor and at the start and end of the add(), remove() and update() methods. The invariant() method should assert that various invariants hold, e.g. that initializingShards contains all the initialising shards in shards in the correct order, and similarly for relocatingShards. See org.elasticsearch.index.seqno.ReplicationTracker for a good example of this pattern.

We should also have some unit tests that exercise these methods to verify that the invariant holds.

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNode.java

kkewwei · 2019-10-16T15:51:05Z

Your suggestion is very helpful, I will modify it according to your suggestion. I have a little doubt, if there is need to call assert invariant() at the end of the constructor, because initializingShards and relocatingShards are created here.

DaveCTurner · 2019-10-16T16:14:46Z

Yes, it's good practice to verify invariants at the end of the constructor. It may be obvious today that all assertions pass on the newly-constructed object, but the point of the exercise is to catch mistakes that may be made in future too.

DaveCTurner · 2019-10-24T19:49:34Z

Hi @kkewwei please don't force-push to PR branches, it loses history and makes it harder to keep track of older reviews. I see you've made a couple of changes. Let us know when it's ready for another review.

kkewwei · 2019-10-25T04:25:24Z

Ok, I will not change previous submissions anymore. It's ready to review. should I open another PR fot this, or continue to review here?

DaveCTurner · 2019-10-25T07:45:55Z

Here is good, thanks. You can push more changes onto a PR branch, just don't force-push since that loses older changes. I might not get to this for the next few days, but it's on my list.

DaveCTurner

Thanks @kkewwei, I left a handful more comments.

Another contributor opened a similar PR, but I'd prefer to keep going with this one. They did however make a related change to ThrottlingAllocationDecider that I would like to do here too:

https://github.com/elastic/elasticsearch/pull/48579/files#diff-74018dd650ea4d9dc396af7800ef377eR126

This doesn't need a specific test, since it's already covered adequately by ThrottlingAllocationTests.

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNode.java

DaveCTurner · 2019-10-28T14:42:48Z

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNode.java

+
+        // shards must contains all the shards in initializingShards and relocatingShards
+        assert  initializingShards.stream().allMatch(shardRouting -> shards.containsValue(shardRouting));
+        assert  relocatingShards.stream().allMatch(shardRouting -> shards.containsValue(shardRouting));


I think we need a stronger property, namely that the order of the shards in relocatingShards is consistent with that in shards:

Suggested change

assert relocatingShards.stream().allMatch(shardRouting -> shards.containsValue(shardRouting));

assert new ArrayList<>(relocatingShards)

.equals(shards.values().stream().filter(ShardRouting::relocating).collect(Collectors.toList()));

(this means the implementation needs some adjustment to get the tests to pass again)

DaveCTurner · 2019-10-28T14:48:12Z

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNode.java

+
+    private boolean invariant() {
+        // initializingShards only contains the initializing shards
+        assert  initializingShards.stream().allMatch(shardRouting -> shardRouting.initializing());


This would be unnecessary with the suggested changes to check equality below.

DaveCTurner · 2019-10-28T14:48:23Z

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNode.java

+        assert  initializingShards.stream().allMatch(shardRouting -> shardRouting.initializing());
+
+        // relocatingShards only contains the relocating shards
+        assert  relocatingShards.stream().allMatch(shardRouting -> shardRouting.relocating());


This would be unnecessary with the suggested changes to check equality below.

DaveCTurner · 2019-10-28T14:56:39Z

server/src/test/java/org/elasticsearch/cluster/routing/RoutingNodeTests.java

+        assertThat(routingNode.shardsWithState(ShardRoutingState.INITIALIZING).size(), equalTo(1));
+    }
+
+    public void testshardsWithState() {


Suggested change

public void testshardsWithState() {

public void testShardsWithStateInIndex() {

kkewwei · 2019-10-29T13:56:13Z

TBR

DaveCTurner

The invariant is still not checking that the cached collections have matching iteration orders.

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNode.java

kkewwei · 2019-10-29T16:08:47Z

@DaveCTurner , I realize what you means, looking forward to your review

DaveCTurner · 2019-10-29T17:06:47Z

@elasticmachine test this please

All comments addressed

DaveCTurner · 2019-10-29T19:00:53Z

The failure (at least, the one of elasticsearch-ci/1) looks relevant. I think we might have to go back to a LinkedHashMap<ShardId, ShardRouting> to preserve the iteration order without hitting that exception.

kkewwei · 2019-10-30T07:21:56Z

I set the type of initializingShards and relocatingShards to be LinkedHashMap, but it doesn't work. It seems that the iteration order of shards can't be changed when we test testCloseWhileRelocatingShards. In RoutingNode.update(), if we don't change the iteration order of shards, the iteration order of relocatingShards will not be consistent with that in shards.

in RoutingNode.update(), i directly overwrite instead of deleting and writing, It work well. if we should not keep the stronger property? I am very happy to follow your suggestion.

DaveCTurner · 2019-10-30T08:23:37Z

I see, yes, this is trickier than I thought. I will have another look at how important the iteration order is.

In the meantime, @elasticmachine test this please

DaveCTurner · 2019-10-30T08:39:37Z

@elasticmachine update branch

DaveCTurner · 2019-10-30T08:39:58Z

@elasticmachine test this please

kkewwei · 2019-10-30T09:39:58Z

https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request+multijob+packaging-tests-windows-gcp-sample/253/console, @DaveCTurner , is there anything I could do or fix?

DaveCTurner · 2019-10-30T10:13:46Z

Failure looks unrelated to this, let's try again before digging deeper.

@elasticmachine please run elasticsearch-ci/packaging-sample-matrix

DaveCTurner

Ok, it does look like the order of these filtered lists isn't so important so let's go with the weaker property. I did a (hopefully final) pass and left a few suggestions, otherwise I think this is good to go.

DaveCTurner · 2019-10-30T11:35:59Z

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNode.java

+
+        if (shard.initializing()) {
+            initializingShards.add(shard);
+        } else if(shard.relocating()) {


Whitespace nit:

Suggested change

} else if(shard.relocating()) {

} else if (shard.relocating()) {

DaveCTurner · 2019-10-30T11:36:24Z

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNode.java

@@ -127,6 +178,14 @@ void remove(ShardRouting shard) {
     * @return number of shards
     */
    public int numberOfShardsWithState(ShardRoutingState... states) {
+        if (states.length == 1) {
+            if(states[0] == ShardRoutingState.INITIALIZING) {


Whitespace nit:

Suggested change

if(states[0] == ShardRoutingState.INITIALIZING) {

if (states[0] == ShardRoutingState.INITIALIZING) {

DaveCTurner · 2019-10-30T11:36:33Z

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNode.java

@@ -144,6 +203,14 @@ public int numberOfShardsWithState(ShardRoutingState... states) {
     * @return List of shards
     */
    public List<ShardRouting> shardsWithState(ShardRoutingState... states) {
+        if (states.length == 1) {
+            if(states[0] == ShardRoutingState.INITIALIZING) {


Whitespace nit:

Suggested change

if(states[0] == ShardRoutingState.INITIALIZING) {

if (states[0] == ShardRoutingState.INITIALIZING) {

DaveCTurner · 2019-10-30T11:36:47Z

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNode.java

@@ -164,6 +231,26 @@ public int numberOfShardsWithState(ShardRoutingState... states) {
    public List<ShardRouting> shardsWithState(String index, ShardRoutingState... states) {
        List<ShardRouting> shards = new ArrayList<>();

+        if (states.length == 1) {
+            if(states[0] == ShardRoutingState.INITIALIZING) {


Whitespace nit:

Suggested change

if(states[0] == ShardRoutingState.INITIALIZING) {

if (states[0] == ShardRoutingState.INITIALIZING) {

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNode.java

DaveCTurner · 2019-10-30T11:40:14Z

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNode.java

    }

    void remove(ShardRouting shard) {
        ShardRouting previousValue = shards.remove(shard.shardId());
        assert previousValue == shard : "expected shard " + previousValue + " but was " + shard;
+        if (shard.initializing()) {
+            boolean exist = initializingShards.remove(shard);
+            assert exist : "expected shard " + shard + "exists in initializingShards, but not ";


Wording/whitespace suggestion:

Suggested change

assert exist : "expected shard " + shard + "exists in initializingShards, but not ";

assert exist : "expected shard " + shard + " to exist in initializingShards";

DaveCTurner · 2019-10-30T11:40:38Z

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNode.java

+            assert exist : "expected shard " + shard + "exists in initializingShards, but not ";
+        } else if (shard.relocating()) {
+            boolean exist = relocatingShards.remove(shard);
+            assert exist : "expected shard " + shard + "exists in relocatingShards, but not ";


Wording/whitespace suggestion:

Suggested change

assert exist : "expected shard " + shard + "exists in relocatingShards, but not ";

assert exist : "expected shard " + shard + " to exist in relocatingShards";

kkewwei · 2019-10-30T15:01:58Z

@DaveCTurner, very thank you for your grammar check.

DaveCTurner

LGTM thanks @kkewwei

DaveCTurner · 2019-10-31T10:14:28Z

@elasticmachine test this please

Today a couple of allocation deciders iterate through all the shards on a node to find the `INITIALIZING` or `RELOCATING` ones, and this can slow down cluster state updates in clusters with very high-density nodes holding many thousands of shards even if those shards belong to closed or frozen indices. This commit pre-computes the sets of `INITIALIZING` and `RELOCATING` shards to speed up this search. Closes #46941 Relates #48579 Co-authored-by: "hongju.xhj" <hongju.xhj@alibaba-inc.com>

count initializing and relocating shards for RoutingNode to reduce th…

990cabb

…e cost time of updating ClusterState

$@polyfractal$ polyfractal added the :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) label Oct 15, 2019

DaveCTurner added >enhancement v7.5.0 v8.0.0 labels Oct 15, 2019

DaveCTurner self-requested a review October 15, 2019 20:31

DaveCTurner requested changes Oct 16, 2019

View reviewed changes

kewei03 added 2 commits October 19, 2019 10:58

add some unit tests, and run ./gradlew precommit success

48d7338

unit test success

896fffb

DaveCTurner mentioned this pull request Oct 28, 2019

improve reroute with many shards #48579

Closed

DaveCTurner requested changes Oct 28, 2019

View reviewed changes

simplify the invariant

be79a2d

DaveCTurner previously requested changes Oct 29, 2019

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNode.java Show resolved Hide resolved

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNode.java Show resolved Hide resolved

the order is consistent

bc5a058

don't keep strong property

28edf71

Merge branch 'master' into fix_46941

f969808

DaveCTurner reviewed Oct 30, 2019

View reviewed changes

DaveCTurner changed the title ~~reduce the time cost to update cluster state~~ Faster access to INITIALIZING/RELOCATING shards Oct 30, 2019

removed whitespace and add invariant

917655d

add whitespace

2cdd65a

DaveCTurner approved these changes Oct 31, 2019

View reviewed changes

DaveCTurner merged commit 30b0a4e into elastic:master Oct 31, 2019

kkewwei deleted the fix_46941 branch October 31, 2019 11:53

DaveCTurner added v7.6.0 and removed v7.5.0 labels Oct 31, 2019

This was referenced Feb 3, 2020

[meta] 7.6 release elastic/elasticsearch-net#4340

Closed

[meta] 7.6 release elastic/elasticsearch-net#4341

Closed

easyice mentioned this pull request Nov 24, 2020

add truncate support for a index #65189

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faster access to INITIALIZING/RELOCATING shards #47817

Faster access to INITIALIZING/RELOCATING shards #47817

kkewwei commented Oct 9, 2019 •

edited by DaveCTurner

Loading

elasticmachine commented Oct 15, 2019

DaveCTurner left a comment

kkewwei commented Oct 16, 2019

DaveCTurner commented Oct 16, 2019

DaveCTurner commented Oct 24, 2019

kkewwei commented Oct 25, 2019

DaveCTurner commented Oct 25, 2019

DaveCTurner left a comment

DaveCTurner Oct 28, 2019

DaveCTurner Oct 28, 2019

DaveCTurner Oct 28, 2019

DaveCTurner Oct 28, 2019

kkewwei commented Oct 29, 2019

DaveCTurner left a comment

kkewwei commented Oct 29, 2019 •

edited

Loading

DaveCTurner commented Oct 29, 2019

DaveCTurner commented Oct 29, 2019 •

edited

Loading

kkewwei commented Oct 30, 2019 •

edited

Loading

DaveCTurner commented Oct 30, 2019

DaveCTurner commented Oct 30, 2019

DaveCTurner commented Oct 30, 2019

kkewwei commented Oct 30, 2019

DaveCTurner commented Oct 30, 2019

DaveCTurner left a comment

DaveCTurner Oct 30, 2019

DaveCTurner Oct 30, 2019

DaveCTurner Oct 30, 2019

DaveCTurner Oct 30, 2019

DaveCTurner Oct 30, 2019

DaveCTurner Oct 30, 2019

kkewwei commented Oct 30, 2019

DaveCTurner left a comment

DaveCTurner commented Oct 31, 2019

	assert relocatingShards.stream().allMatch(shardRouting -> shards.containsValue(shardRouting));
	assert new ArrayList<>(relocatingShards)
	.equals(shards.values().stream().filter(ShardRouting::relocating).collect(Collectors.toList()));

	public void testshardsWithState() {
	public void testShardsWithStateInIndex() {

	} else if(shard.relocating()) {
	} else if (shard.relocating()) {

	if(states[0] == ShardRoutingState.INITIALIZING) {
	if (states[0] == ShardRoutingState.INITIALIZING) {

	assert exist : "expected shard " + shard + "exists in initializingShards, but not ";
	assert exist : "expected shard " + shard + " to exist in initializingShards";

Faster access to INITIALIZING/RELOCATING shards #47817

Faster access to INITIALIZING/RELOCATING shards #47817

Conversation

kkewwei commented Oct 9, 2019 • edited by DaveCTurner Loading

elasticmachine commented Oct 15, 2019

DaveCTurner left a comment

Choose a reason for hiding this comment

kkewwei commented Oct 16, 2019

DaveCTurner commented Oct 16, 2019

DaveCTurner commented Oct 24, 2019

kkewwei commented Oct 25, 2019

DaveCTurner commented Oct 25, 2019

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkewwei commented Oct 29, 2019

DaveCTurner left a comment

Choose a reason for hiding this comment

kkewwei commented Oct 29, 2019 • edited Loading

DaveCTurner commented Oct 29, 2019

DaveCTurner commented Oct 29, 2019 • edited Loading

kkewwei commented Oct 30, 2019 • edited Loading

DaveCTurner commented Oct 30, 2019

DaveCTurner commented Oct 30, 2019

DaveCTurner commented Oct 30, 2019

kkewwei commented Oct 30, 2019

DaveCTurner commented Oct 30, 2019

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkewwei commented Oct 30, 2019

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner commented Oct 31, 2019

kkewwei commented Oct 9, 2019 •

edited by DaveCTurner

Loading

kkewwei commented Oct 29, 2019 •

edited

Loading

DaveCTurner commented Oct 29, 2019 •

edited

Loading

kkewwei commented Oct 30, 2019 •

edited

Loading