Add documentation around desired balance #119902

DiannaHohensee · 2025-01-09T21:50:07Z

More documentation again. I'm trying to figure out how everything plugs together so I can hook in my metric collection.

Relates ES-10341

elasticsearchmachine · 2025-01-09T21:50:31Z

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

nicktindall

LGTM with some minor comments (some are probably just preference so up to you how you address)

nicktindall · 2025-01-09T22:43:44Z

server/src/main/java/org/elasticsearch/cluster/routing/allocation/allocator/DesiredBalance.java

+ * @param lastConvergedIndex Identifies what input data the balancer computation round used to produce this {@link DesiredBalance}. See
+ *                           {@link DesiredBalanceInput#index()} for details. Each reroute request gets assigned a monotonically increasing
+ *                           sequence number, and the balancer, which runs async to reroute, uses the latest request's data to compute the
+ *                           desired balance.


Nit: I think this would be "strictly increasing", "monotonically increasing" means values can be repeated? Perhaps "sequence number" is enough as (I think) it implies the same?

Sounds good, applied. Simpler.

On master failover, the index gets set back to -1

elasticsearch/server/src/main/java/org/elasticsearch/cluster/routing/allocation/allocator/DesiredBalanceShardsAllocator.java

Lines 378 to 379 in 80729f9

private void onNoLongerMaster() {

if (indexGenerator.getAndSet(-1) != -1) {

If the node later becomes master again, the index will again begin from zero. So I think we should qualified it as "strictly increasing in the same master term".

Updated 👍 Thanks!

nicktindall · 2025-01-09T23:04:31Z

...va/org/elasticsearch/cluster/routing/allocation/allocator/DesiredBalanceShardsAllocator.java

+     * produces a new ClusterState with the changes made by {@link DesiredBalanceReconciler#reconcile}. The {@link RerouteStrategy} provided
+     * to the callback calls into {@link #desiredBalanceReconciler} for the changes. The {@link #masterServiceTaskQueue} will apply the
+     * cluster state update.
+     */


This comment seems overly specific to me? Given it's an interface, I feel like I'd rather know what it does rather than how it does it.

I think it's the the "to run ...." bit that I find jarring. If it's a good abstraction, only the what should matter, not the how. We can use our IDEs to find the implementation(s). Also would be less likely to go stale if we were less specific.

I agree that a good abstraction would explain what and not how. The problem with this area of the code is that it's like spaghetti and difficult to follow. Right now the Allocator has a callback to the AllocationService, which has a callback to the Allocator, which produces a result for the AllocationService to feed back into the Allocator's MasterServiceTaskQueue..... The first step to improve the code, in my mind, is to document what's happening, later we can hopefully refactor the code.

produces a result for the AllocationService to feed back into the Allocator's MasterServiceTaskQueue

I don't quite get this part. Seems also related to the last sentence in the comment

The {@link #masterServiceTaskQueue} will apply the cluster state update.

IIUC, submitting the ReconcileDesiredBalanceTask should be the first step to trigger the callback to AllocationService, i.e. reconciler. It also feels a bit odd to say a master task queue "apply" the cluster state update. In general, master service computes the new cluster state. The ClusterApplierService then applies the state update.

I've changed the text from apply to publish, per the MasterService terminology. And mentioned that the ReconcileDesiredBalanceExecutor constructs the cluster state. Hopefully that makes things a bit clearer, let me know.

In general, master service computes the new cluster state. The ClusterApplierService then applies the state update.

The MasterServiceTaskQueue is created here with the *Executor, and the queue appears to know how to execute and publish a cluster state update. I didn't dig into the details, though.

Thanks. Yeah, MasterService computes (executes) and publishes the new cluster state. So Publish sounds good. There relevant applier here is IndicesClusterStateService.

...va/org/elasticsearch/cluster/routing/allocation/allocator/DesiredBalanceShardsAllocator.java

nicktindall · 2025-01-09T23:09:32Z

...va/org/elasticsearch/cluster/routing/allocation/allocator/DesiredBalanceShardsAllocator.java

+     * Accepts listeners with an index value (see {#link #indexGenerator}) and run them whenever a DesiredBalance computation completes with
+     * an equal or greater index value.
+     */
+    private final PendingListenersQueue pendingListenersQueue;


I think the javadoc on the pending listeners queue is enough? or we're duplicating it a bit (i.e. more to maintain)

Oops, you're right. Rewrote to just say that it tracks and runs listeners for after computation completes

…ation

DiannaHohensee

Updated

DiannaHohensee · 2025-01-10T18:48:03Z

server/src/main/java/org/elasticsearch/cluster/routing/allocation/allocator/DesiredBalance.java

+ * @param lastConvergedIndex Identifies what input data the balancer computation round used to produce this {@link DesiredBalance}. See
+ *                           {@link DesiredBalanceInput#index()} for details. Each reroute request gets assigned a monotonically increasing
+ *                           sequence number, and the balancer, which runs async to reroute, uses the latest request's data to compute the
+ *                           desired balance.


Sounds good, applied. Simpler.

DiannaHohensee · 2025-01-10T18:53:23Z

...va/org/elasticsearch/cluster/routing/allocation/allocator/DesiredBalanceShardsAllocator.java

+     * produces a new ClusterState with the changes made by {@link DesiredBalanceReconciler#reconcile}. The {@link RerouteStrategy} provided
+     * to the callback calls into {@link #desiredBalanceReconciler} for the changes. The {@link #masterServiceTaskQueue} will apply the
+     * cluster state update.
+     */


I agree that a good abstraction would explain what and not how. The problem with this area of the code is that it's like spaghetti and difficult to follow. Right now the Allocator has a callback to the AllocationService, which has a callback to the Allocator, which produces a result for the AllocationService to feed back into the Allocator's MasterServiceTaskQueue..... The first step to improve the code, in my mind, is to document what's happening, later we can hopefully refactor the code.

...va/org/elasticsearch/cluster/routing/allocation/allocator/DesiredBalanceShardsAllocator.java

DiannaHohensee · 2025-01-10T19:06:36Z

...va/org/elasticsearch/cluster/routing/allocation/allocator/DesiredBalanceShardsAllocator.java

+     * Accepts listeners with an index value (see {#link #indexGenerator}) and run them whenever a DesiredBalance computation completes with
+     * an equal or greater index value.
+     */
+    private final PendingListenersQueue pendingListenersQueue;


Oops, you're right. Rewrote to just say that it tracks and runs listeners for after computation completes

…ation

DiannaHohensee

Made some updates from Yang's review

ywangd

LGTM

Relates ES-10341

Add documentation around desired balance

19d2126

DiannaHohensee added >non-issue :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) Team:Distributed Coordination Meta label for Distributed Coordination team labels Jan 9, 2025

DiannaHohensee self-assigned this Jan 9, 2025

elasticsearchmachine added the v9.0.0 label Jan 9, 2025

DiannaHohensee requested review from nicktindall and pxsalehi January 9, 2025 21:52

nicktindall approved these changes Jan 9, 2025

View reviewed changes

DiannaHohensee added 2 commits January 9, 2025 23:54

Merge branch 'main' into 2025/01/09/ES-10341-desired-balance-document…

ebb5528

…ation

improvements per review

9507d31

DiannaHohensee commented Jan 10, 2025

View reviewed changes

DiannaHohensee added 2 commits January 13, 2025 09:14

Merge branch 'main' into 2025/01/09/ES-10341-desired-balance-document…

52917f7

…ation

improvements per Yang's review

c983f65

DiannaHohensee commented Jan 13, 2025

View reviewed changes

DiannaHohensee requested a review from ywangd January 13, 2025 22:50

ywangd approved these changes Jan 13, 2025

View reviewed changes

DiannaHohensee merged commit 455fde2 into elastic:main Jan 14, 2025
16 checks passed

martijnvg pushed a commit to martijnvg/elasticsearch that referenced this pull request Jan 14, 2025

Add documentation around desired balance (elastic#119902)

93fd8ce

Relates ES-10341

	private void onNoLongerMaster() {
	if (indexGenerator.getAndSet(-1) != -1) {

Add documentation around desired balance #119902

Add documentation around desired balance #119902

Uh oh!

Conversation

DiannaHohensee commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jan 9, 2025

Uh oh!

nicktindall 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DiannaHohensee 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!

Choose a reason for hiding this comment

Uh oh!

DiannaHohensee left a comment

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!

DiannaHohensee commented Jan 9, 2025 •

edited

Loading