Add Node Weight to GetDesiredBalance #131025

joshua-adams-1 · 2025-07-10T15:16:53Z

Extends the _internal/desired_balance API to return the node weights. These are found within the DesiredBalanceResponse.ClusterBalanceStats.NodeBalanceStats object. If no node weights have been calculated, then this value defaults to 0.

Github issue: #126579
Jira Ticket: ES-11546

Extends the `_internal/desired_balance` API to return the node weights. These are found within the `DesiredBalanceResponse.ClusterBalanceStats .NodeBalanceStats` object. If no node weights have been calculated, then this value defaults to 0. Issue: elastic#126579

elasticsearchmachine · 2025-07-11T15:04:26Z

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

ywangd

Thanks for working on this. Looks promising. I left some comments.

...rc/main/java/org/elasticsearch/cluster/routing/allocation/allocator/ClusterBalanceStats.java

...st/java/org/elasticsearch/cluster/routing/allocation/allocator/ClusterBalanceStatsTests.java

ywangd · 2025-07-14T05:41:46Z

...-api-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.desired_balance/10_basic.yml

+  - requires:
+      capabilities:
+        - method: GET
+          path: _internal/desired_balance
+          capabilities: [ cluster_balance-node_balance_stats-node_weights_returned ]
+      test_runner_features: [ capabilities ]
+      reason: "Node weights returned in the node balance stats was added in version 9.2.0"


I think we should not add this requires section to existing tests since (1) they do not test the new field and (2) it reduces the test coverage for old versions. Instead we should add a new test with this requires section and checks the new field in the test.

I had issues with the CI tests failing without this - will wait for the CI tests to run on my most recent commit and try and debug from there

I am surprised that the existing tests would fail without it. If you share a buildscan of the failure, I can also help looking into it.

...r/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetDesiredBalanceAction.java

...rc/main/java/org/elasticsearch/cluster/routing/allocation/allocator/ClusterBalanceStats.java

ywangd

Looking great. I only had minor comments.

...st/java/org/elasticsearch/cluster/routing/allocation/allocator/ClusterBalanceStatsTests.java

ywangd · 2025-07-16T06:57:17Z

...-api-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.desired_balance/10_basic.yml

+  - requires:
+      capabilities:
+        - method: GET
+          path: _internal/desired_balance
+          capabilities: [ cluster_balance-node_balance_stats-node_weights_returned ]
+      test_runner_features: [ capabilities ]
+      reason: "Node weights returned in the node balance stats was added in version 9.2.0"


I am surprised that the existing tests would fail without it. If you share a buildscan of the failure, I can also help looking into it.

ywangd

Some more minor comments.

...-api-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.desired_balance/10_basic.yml

test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java

...st/java/org/elasticsearch/cluster/routing/allocation/allocator/ClusterBalanceStatsTests.java

ywangd

Looking great. Some minor comments over testing code.

.../test/java/org/elasticsearch/cluster/routing/allocation/allocator/NodeBalanceStatsTests.java

ywangd · 2025-07-21T01:10:18Z

.../test/java/org/elasticsearch/cluster/routing/allocation/allocator/NodeBalanceStatsTests.java

+    public void testSerializationWithTransportVersionV_8_7_0() throws IOException {
+        ClusterBalanceStats.NodeBalanceStats instance = createTestInstance();
+        // Serialization changes based on this version
+        TransportVersion oldVersion = TransportVersions.V_8_7_0;


It's common to randomize from a broader version range for the oldVersion, i.e.:

Suggested change

TransportVersion oldVersion = TransportVersions.V_8_7_0;

final var oldVersion = TransportVersionUtils.randomVersionBetween(

random(),

TransportVersions.MINIMUM_COMPATIBLE,

TransportVersionUtils.getPreviousVersion(TransportVersions.V_8_8_0)

);

Fixed in 4a7ec19

Hey, so MINIMUM_COMPATIBLE refers to Elastic version 8.19. Therefore any transport version less than this can be safely removed, as per the comment on line 51 of this file, and the Jira ticket https://elasticco.atlassian.net/browse/ES-10337.

Therefore, as a follow up to this PR, I will remove all references to TransportVersion.V_8_8_0 and TransportVersions.V_8_12_0 in the NodeBalanceStats.readFrom() method (and then delete these tests since they will then not be necessary).

Until that time, I have just randomly generated a TransportVersion between V_8_0_0 and V_8_8_0

Ah you are right. 👍

.../test/java/org/elasticsearch/cluster/routing/allocation/allocator/NodeBalanceStatsTests.java

...rc/main/java/org/elasticsearch/cluster/routing/allocation/allocator/ClusterBalanceStats.java

cla-checker-service · 2025-07-21T15:14:44Z

❌ Author of the following commits did not sign a Contributor Agreement:
f7cb702

Please, read and sign the above mentioned agreement if you want to contribute to this project

ywangd

LGTM

One comment about handling not-yet-included node is somewhat important and please address it. Thanks for the iterations!

ywangd · 2025-07-22T00:18:29Z

...st/java/org/elasticsearch/cluster/routing/allocation/allocator/ClusterBalanceStatsTests.java

@@ -300,18 +372,45 @@ private static ClusterState createClusterState(List<DiscoveryNode> nodes, List<T
            .build();
    }

-    private static DesiredBalance createDesiredBalance(ClusterState state) {
+    private static DesiredBalance createDesiredBalanceWithEmptyNodeWeights(ClusterState state) {
+        return createDesiredBalance(state, randomDoubleBetween(-1, 1, true), true);


Nit: seems wasteful to randomize given its unused

Suggested change

return createDesiredBalance(state, randomDoubleBetween(-1, 1, true), true);

return createDesiredBalance(state, 0, true);

ywangd · 2025-07-22T00:33:45Z

...rc/main/java/org/elasticsearch/cluster/routing/allocation/allocator/ClusterBalanceStats.java

+            Double nodeWeight = desiredBalance.weightsPerNode().isEmpty()
+                ? null
+                : desiredBalance.weightsPerNode().get(routingNode.node()).nodeWeight();


Two suggestions

We can add assert desiredBalance != null before this line to make the intention clear that it should not be null. Otherwise if it throws a NPE in some future tests, it would make people pause and wonder whether null handling is needed here.

Desired balance is compueted asynchrounously. It is possible that a new node from the latest cluster state is not yet included. So we need to count for it as well, e.g. something like:

Double nodeWeight = Optional.of(desiredBalance.weightsPerNode().get(routingNode.node()) .map(NodeWeightStats::nodeWeight) .orElse(null);

joshua-adams-1 self-assigned this Jul 10, 2025

elasticsearchmachine added the v9.2.0 label Jul 10, 2025

joshua-adams-1 added 2 commits July 11, 2025 10:28

Fix tests

1c168da

Merge branch 'main' into get-desired-balance-node-weights

aeb59b2

joshua-adams-1 force-pushed the get-desired-balance-node-weights branch from 78aa356 to aeb59b2 Compare July 11, 2025 09:30

joshua-adams-1 added 2 commits July 11, 2025 11:19

Ran ./gradlew spotlessApply precommit

54d7d3a

Merge branch 'main' into get-desired-balance-node-weights

3ac4def

joshua-adams-1 force-pushed the get-desired-balance-node-weights branch from 3ed1d0d to 3ac4def Compare July 11, 2025 10:26

joshua-adams-1 added 3 commits July 11, 2025 11:35

Revert refactoring

038abf4

Add Capabilities to fix tests

918807a

Merge branch 'main' into get-desired-balance-node-weights

f59c5e1

joshua-adams-1 requested a review from ywangd July 11, 2025 15:03

joshua-adams-1 added >non-issue :Distributed Coordination/Distributed A catch all label for anything in the Distributed Coordination area. Please avoid if you can. labels Jul 11, 2025

joshua-adams-1 marked this pull request as ready for review July 11, 2025 15:04

elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Jul 11, 2025

ywangd reviewed Jul 14, 2025

View reviewed changes

joshua-adams-1 added 2 commits July 15, 2025 14:53

Yang Comments

e959108

Merge branch 'main' into get-desired-balance-node-weights

7cd784c

ywangd reviewed Jul 16, 2025

View reviewed changes

joshua-adams-1 added 5 commits July 16, 2025 11:33

Adds extra ClusterBalanceStatsTests

2bc42c7

Removes comments

e3c3779

Add YAML Rest Tests

a684af1

Ran ./gradlew spotlessApply precommit

df2d3a0

Merge branch 'main' into get-desired-balance-node-weights

d255680

joshua-adams-1 force-pushed the get-desired-balance-node-weights branch from 129ab38 to d255680 Compare July 16, 2025 10:59

joshua-adams-1 added 5 commits July 16, 2025 12:06

Minor updates

79f3177

Yang Comments

66173ae

Nit

fc9ec1e

Fix Forbidden API verification failed error

7764c0d

Merge branch 'main' into get-desired-balance-node-weights

4b25fe0

ywangd reviewed Jul 17, 2025

View reviewed changes

joshua-adams-1 added 3 commits July 17, 2025 12:51

Remove randomSubset

9fd9a73

Create NodeBalanceStatsTests

e1a0c23

Merge branch 'main' into get-desired-balance-node-weights

08d6cc8

joshua-adams-1 force-pushed the get-desired-balance-node-weights branch from 9b99150 to 08d6cc8 Compare July 17, 2025 14:00

joshua-adams-1 added 3 commits July 17, 2025 15:07

Change is_true to exists

4ae875d

OptionalDouble can sometimes produce nulls

154d6d1

Merge branch 'main' into get-desired-balance-node-weights

cd5e8f4

joshua-adams-1 force-pushed the get-desired-balance-node-weights branch from 594fc20 to cd5e8f4 Compare July 18, 2025 08:36

joshua-adams-1 requested a review from ywangd July 18, 2025 11:12

ywangd reviewed Jul 21, 2025

View reviewed changes

joshua-adams-1 added 2 commits July 21, 2025 16:01

Yang Comments

4a7ec19

Merge branch 'main' into get-desired-balance-node-weights

72635ca

joshua-adams-1 force-pushed the get-desired-balance-node-weights branch from 3955ca7 to 72635ca Compare July 21, 2025 15:03

Merge branch 'main' into get-desired-balance-node-weights

dfe3506

joshua-adams-1 force-pushed the get-desired-balance-node-weights branch from dca823a to dfe3506 Compare July 21, 2025 17:15

[CI] Auto commit changes from spotless

f7cb702

ywangd approved these changes Jul 22, 2025

View reviewed changes

-        TransportVersion oldVersion = TransportVersions.V_8_7_0;
+        final var oldVersion = TransportVersionUtils.randomVersionBetween(
+            random(),
+            TransportVersions.MINIMUM_COMPATIBLE,
+            TransportVersionUtils.getPreviousVersion(TransportVersions.V_8_8_0)
+        );

	return createDesiredBalance(state, randomDoubleBetween(-1, 1, true), true);
	return createDesiredBalance(state, 0, true);

Add Node Weight to GetDesiredBalance #131025

Are you sure you want to change the base?

Add Node Weight to GetDesiredBalance #131025

Conversation

joshua-adams-1 commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 11, 2025

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cla-checker-service bot commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joshua-adams-1 commented Jul 10, 2025 •

edited

Loading

cla-checker-service bot commented Jul 21, 2025 •

edited

Loading