Clamp auto-expand replicas to the closest value #87505

pxsalehi · 2022-06-08T11:40:15Z

Ensure that the number of replicas chosen for an auto-expand-able index
is within the range of the available data nodes, i.e., excluding those
nodes that cannot be assigned a replica.

Closes #84788

elasticsearchmachine · 2022-06-08T11:41:02Z

Hi @pxsalehi, I've created a changelog YAML for you.

pxsalehi · 2022-06-09T17:11:28Z

@elasticmachine update branch

Ensure that the number of replicas chosen for an auto-expand-able shard is within the range of the available data nodes, i.e., excluding those nodes that cannot be assigned a replica. Closes elastic#84788

server/src/main/java/org/elasticsearch/cluster/metadata/AutoExpandReplicas.java

elasticmachine · 2022-06-09T17:53:34Z

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

henningandersen

I left a few comments, otherwise this looks good.

henningandersen · 2022-06-09T19:56:25Z

...er/src/internalClusterTest/java/org/elasticsearch/cluster/metadata/AutoExpandReplicasIT.java

+                equalTo(String.valueOf(initialReplicas))
+            );
+        });
+        ensureGreen(indexName);


This seems unimportant to the test?

IMO, at least for the sake of making it clear what the test expects, we should leave it here. Removing it didn't seem to really speedup the test.

It is not about test speed. But waiting for green here signals that it is important to the test. But the test will pass without this, since our auto-expand calculation is independent of the status of the index. Hence I think we should simply remove it.

henningandersen · 2022-06-09T19:57:29Z

...er/src/internalClusterTest/java/org/elasticsearch/cluster/metadata/AutoExpandReplicasIT.java

+        });
+
+        ensureGreen(indexName);
+    }


Can we also remove the setting and see that it jumps back to the initialReplicas value, thus checking that it clamps to the higher bound too?

Done in ec7d7f9.

henningandersen · 2022-06-09T20:04:13Z

server/src/main/java/org/elasticsearch/cluster/metadata/AutoExpandReplicas.java


+    // Package private only for testing purposes!
+    int calculateDesiredNumberOfReplicas(int numMatchingDataNodes) {
        final int min = minReplicas();
        final int max = getMaxReplicas(numMatchingDataNodes);


The Math.min done in getMaxReplicas seems redundant now, I think we should remove it, it is somewhat confusing now.

That's a very good point! It also simplifies the code and the parsing test. Done in ec7d7f9.

server/src/main/java/org/elasticsearch/cluster/metadata/AutoExpandReplicas.java

henningandersen · 2022-06-09T20:09:20Z

server/src/test/java/org/elasticsearch/cluster/metadata/AutoExpandReplicasTests.java

@@ -282,4 +282,16 @@ public void testOnlyAutoExpandAllocationFilteringAfterAllNodesUpgraded() {
            terminate(threadPool);
        }
    }
+
+    public void testCalculateDesiredNumberOfReplicas() {
+        String settingValue = randomFrom("0-all", "0-3");


Can we explore the space a bit more here, for instance by choosing both lower and upper bound randomly like:

int lowerBound = between(0,9); int upperBound = between(lowerBound, 10); String settingValue = lowerBound + "-" + upperBound;

Having a dedicated test (or random variation) with "all" also makes sense.

Done in ec7d7f9.

...er/src/internalClusterTest/java/org/elasticsearch/cluster/metadata/AutoExpandReplicasIT.java

…pandReplicas.java Co-authored-by: Henning Andersen <33268011+henningandersen@users.noreply.github.com>

…metadata/AutoExpandReplicasIT.java Co-authored-by: Henning Andersen <33268011+henningandersen@users.noreply.github.com>

pxsalehi · 2022-06-13T07:19:02Z

@elasticmachine update branch

henningandersen

LGTM, provided tests pass

henningandersen · 2022-06-13T13:21:21Z

...er/src/internalClusterTest/java/org/elasticsearch/cluster/metadata/AutoExpandReplicasIT.java

+                equalTo(String.valueOf(initialReplicas))
+            );
+        });
+        ensureGreen(indexName);


It is not about test speed. But waiting for green here signals that it is important to the test. But the test will pass without this, since our auto-expand calculation is independent of the status of the index. Hence I think we should simply remove it.

pxsalehi · 2022-06-13T14:25:56Z

Thanks Henning! If index status is irrelevant here, then I agree with you. I removed them! :-)

pxsalehi added >bug :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) labels Jun 8, 2022

elasticsearchmachine added the v8.4.0 label Jun 8, 2022

pxsalehi and others added 3 commits June 9, 2022 19:41

Reproducer integration test

d8bd0e4

Update docs/changelog/87505.yaml

bf9254e

Clamp auto-expand replicas to the closest value

1c1b9ea

Ensure that the number of replicas chosen for an auto-expand-able shard is within the range of the available data nodes, i.e., excluding those nodes that cannot be assigned a replica. Closes elastic#84788

pxsalehi force-pushed the ps-auto-expand-issue-84788 branch from 11e398e to 1c1b9ea Compare June 9, 2022 17:44

pxsalehi commented Jun 9, 2022

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/metadata/AutoExpandReplicas.java Show resolved Hide resolved

pxsalehi marked this pull request as ready for review June 9, 2022 17:53

elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Jun 9, 2022

pxsalehi requested a review from henningandersen June 9, 2022 17:53

henningandersen reviewed Jun 9, 2022

View reviewed changes

pxsalehi and others added 2 commits June 13, 2022 09:18

Update server/src/main/java/org/elasticsearch/cluster/metadata/AutoEx…

2ea7155

…pandReplicas.java Co-authored-by: Henning Andersen <33268011+henningandersen@users.noreply.github.com>

Update server/src/internalClusterTest/java/org/elasticsearch/cluster/…

95b3bce

…metadata/AutoExpandReplicasIT.java Co-authored-by: Henning Andersen <33268011+henningandersen@users.noreply.github.com>

elasticmachine and others added 2 commits June 13, 2022 08:19

Merge branch 'master' into ps-auto-expand-issue-84788

8a1474d

Address review comments

ec7d7f9

pxsalehi requested a review from henningandersen June 13, 2022 12:59

henningandersen approved these changes Jun 13, 2022

View reviewed changes

Remove ensureGreen statements

4848a62

pxsalehi merged commit 60efb9d into elastic:master Jun 13, 2022

pxsalehi added v8.3.0 auto-backport-and-merge and removed auto-backport-and-merge v8.3.0 labels Jun 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clamp auto-expand replicas to the closest value #87505

Clamp auto-expand replicas to the closest value #87505

pxsalehi commented Jun 8, 2022 •

edited

Loading

elasticsearchmachine commented Jun 8, 2022

pxsalehi commented Jun 9, 2022

elasticmachine commented Jun 9, 2022

henningandersen left a comment

henningandersen Jun 9, 2022

pxsalehi Jun 13, 2022

henningandersen Jun 13, 2022

henningandersen Jun 9, 2022

pxsalehi Jun 13, 2022

henningandersen Jun 9, 2022

pxsalehi Jun 13, 2022

henningandersen Jun 9, 2022

pxsalehi Jun 13, 2022

pxsalehi commented Jun 13, 2022

henningandersen left a comment

henningandersen Jun 13, 2022

pxsalehi commented Jun 13, 2022

Clamp auto-expand replicas to the closest value #87505

Clamp auto-expand replicas to the closest value #87505

Conversation

pxsalehi commented Jun 8, 2022 • edited Loading

elasticsearchmachine commented Jun 8, 2022

pxsalehi commented Jun 9, 2022

elasticmachine commented Jun 9, 2022

henningandersen 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pxsalehi commented Jun 13, 2022

henningandersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pxsalehi commented Jun 13, 2022

pxsalehi commented Jun 8, 2022 •

edited

Loading