Only publish desired balance gauges on master #115383

nicktindall · 2024-10-23T05:45:45Z

Closes ES-9834

nicktindall · 2024-10-23T05:48:26Z

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

@@ -168,6 +171,7 @@ public String toString() {
            if (event.localNodeMaster() == false) {
                onNoLongerMaster();
            }
+            nodeIsMaster.set(event.localNodeMaster());


Perhaps there is an alternative pattern for doing this kind of thing?

Nit: we can reduce the number of atomic set calls with a check of event.localNodeMaster() != event.previousState().nodes().isLocalNodeElectedMaster().

Alternatively, we can register the metrics in this class since we are already accessing the fields like desiredBalanceReconciler.unassignedShards directly in onNoLongerMater. This way we can keep a volatile field for master check and no need to pass it down. Not necessarily better than what you have. Happy for you to decide.

I've done a fairly significant refactor to move all the metrics and their state & logic to a dedicated class, it means we can unit test, get rid of all the Atomics, and make a nicer interface for setting whether we're master and getting and setting the metric values.

elasticsearchmachine · 2024-10-23T05:50:00Z

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

elasticsearchmachine · 2024-10-23T06:06:29Z

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

nicktindall · 2024-10-23T06:14:21Z

...in/java/org/elasticsearch/cluster/routing/allocation/allocator/DesiredBalanceReconciler.java

            "es.allocator.desired_balance.allocations.undesired.current",
            "Total number of shards allocated on undesired nodes excluding shutting down nodes",
-            "{shard}"
+            "{shard}",
+            this::getUndesiredAllocations
        );


These return a Closeable which (in the APM implementation at least) deregisters the metric when closed, we were ignoring it previously when we used the synchronous API and these metrics live for the life of the node so I assume we don't need to worry about it here.

It's fine since a node may become master again. So we don't want to close it since there is no way to re-open. Muting with empty result is what we want here.

idegtiarenko · 2024-10-23T07:11:36Z

...in/java/org/elasticsearch/cluster/routing/allocation/allocator/DesiredBalanceReconciler.java

        );
-        undesiredAllocations = LongGaugeMetric.create(
-            meterRegistry,
+        meterRegistry.registerLongsGauge(


I am new to this one.
Does it emit a single document with array of metrics or number of separate documents?
In other words, if not an elected master, would we get empty array or no documents?

The places I've seen it used is where you have a bunch of values for a metric that have different labels. So a number of separate documents. In this case we're just using it to be able to publish nothing when we are not the master (i.e. no documents)

idegtiarenko · 2024-10-23T07:13:09Z

docs/changelog/115383.yaml

+pr: 115383
+summary: Only publish desired balance gauges on master
+area: Allocation
+type: bug


I am not entirely convinced this is bug. Sounds more like an enhancement when we only publish metric from elected master and do not publish zero metrics from everywhere else.

elasticsearchmachine · 2024-10-23T08:03:12Z

Hi @nicktindall, I've updated the changelog YAML for you.

…desired_balance_metrics_on_current_master

ywangd

LGTM

ywangd · 2024-10-23T23:56:45Z

...in/java/org/elasticsearch/cluster/routing/allocation/allocator/DesiredBalanceReconciler.java

            "es.allocator.desired_balance.allocations.undesired.current",
            "Total number of shards allocated on undesired nodes excluding shutting down nodes",
-            "{shard}"
+            "{shard}",
+            this::getUndesiredAllocations
        );


It's fine since a node may become master again. So we don't want to close it since there is no way to re-open. Muting with empty result is what we want here.

ywangd · 2024-10-24T00:12:56Z

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

@@ -168,6 +171,7 @@ public String toString() {
            if (event.localNodeMaster() == false) {
                onNoLongerMaster();
            }
+            nodeIsMaster.set(event.localNodeMaster());


Nit: we can reduce the number of atomic set calls with a check of event.localNodeMaster() != event.previousState().nodes().isLocalNodeElectedMaster().

Alternatively, we can register the metrics in this class since we are already accessing the fields like desiredBalanceReconciler.unassignedShards directly in onNoLongerMater. This way we can keep a volatile field for master check and no need to pass it down. Not necessarily better than what you have. Happy for you to decide.

…desired_balance_metrics_on_current_master

ywangd

LGTM

Thanks for the extra refinement.

ywangd · 2024-10-24T04:42:26Z

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

@@ -168,6 +170,7 @@ public String toString() {
            if (event.localNodeMaster() == false) {
                onNoLongerMaster();
            }
+            desiredBalanceMetrics.setNodeIsMaster(event.localNodeMaster());


Nit: I still think checking event.localNodeMaster() != event.previousState().nodes().isLocalNodeElectedMaster() before calling this method is helpful since otherwise most of the time it ends up setting the same value.

Closes ES-9834

Only publish desired balance gauges on master

8cce810

Closes ES-9834

elasticsearchmachine added the v9.0.0 label Oct 23, 2024

nicktindall commented Oct 23, 2024

View reviewed changes

nicktindall added >bug :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) labels Oct 23, 2024

nicktindall requested review from idegtiarenko and DaveCTurner and removed request for DaveCTurner October 23, 2024 05:49

Update docs/changelog/115383.yaml

ae297a0

nicktindall requested a review from ywangd October 23, 2024 05:50

nicktindall marked this pull request as ready for review October 23, 2024 06:06

elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Oct 23, 2024

nicktindall added 2 commits October 23, 2024 17:07

Fix test naming

35f93cc

Remove redundant comment

7a17048

nicktindall commented Oct 23, 2024

View reviewed changes

idegtiarenko reviewed Oct 23, 2024

View reviewed changes

nicktindall added >enhancement and removed >bug labels Oct 23, 2024

Update docs/changelog/115383.yaml

dcc39e6

Merge remote-tracking branch 'origin/main' into ES-9834_only_publish_…

b9a9988

…desired_balance_metrics_on_current_master

ywangd approved these changes Oct 24, 2024

View reviewed changes

nicktindall added 2 commits October 24, 2024 12:52

Merge remote-tracking branch 'origin/main' into ES-9834_only_publish_…

e893d1d

…desired_balance_metrics_on_current_master

Move metrics and associated state/logic into a dedicated class

6332f59

nicktindall requested a review from ywangd October 24, 2024 02:38

ywangd approved these changes Oct 24, 2024

View reviewed changes

Minimise volatile writes

01fccc8

idegtiarenko approved these changes Oct 24, 2024

View reviewed changes

nicktindall merged commit cc9a08a into elastic:main Oct 24, 2024
15 of 16 checks passed

nicktindall deleted the ES-9834_only_publish_desired_balance_metrics_on_current_master branch October 24, 2024 07:43

davidkyle pushed a commit to davidkyle/elasticsearch that referenced this pull request Oct 24, 2024

Only publish desired balance gauges on master (elastic#115383)

e88a357

Closes ES-9834

georgewallace pushed a commit to georgewallace/elasticsearch that referenced this pull request Oct 25, 2024

Only publish desired balance gauges on master (elastic#115383)

efa3f60

Closes ES-9834

jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request Nov 4, 2024

Only publish desired balance gauges on master (elastic#115383)

b25bcb1

Closes ES-9834

Only publish desired balance gauges on master #115383

Only publish desired balance gauges on master #115383

Uh oh!

Conversation

nicktindall commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

elasticsearchmachine commented Oct 23, 2024

Uh oh!

elasticsearchmachine commented Oct 23, 2024

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!

nicktindall Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Oct 23, 2024

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!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nicktindall commented Oct 23, 2024 •

edited

Loading

nicktindall Oct 23, 2024 •

edited

Loading