Add helper to pick node for reindex relocation by PeteGillinElastic · Pull Request #139081 · elastic/elasticsearch

PeteGillinElastic · 2025-12-04T18:12:51Z

This adds a helper class which can be used to pick a node to relocate a reindex task to on shutdown.

The plugin exposes an implementation of the
ReindexRelocationNodePicker interface as a component, so that in a future change it can be injected into the transport action, and from there passed into the Reindexer, and from there used to actually relocate the task on shutdown.

By default, it uses the DefaultReindexRelocationNodePicker. Another plugin may override this by extending the reindex plugin and making the implementation available via SPI.

Closes elastic/elasticsearch-team#2092

This adds a helper class which can be used to pick a node to relocate a reindex task to on shutdown. The plugin exposes an implementation of the `ReindexRelocationNodePicker` interface as a component, so that in a future change it can be injected into the transport action, and from there passed into teh `Reindexer`, and from there used to actually relocate the task on shutdown. By default, it uses the `DefaultReindexRelocationNodePicker`. Another plugin may override this by extending the `reindex` plugin and making the implementation available via SPI.

elasticsearchmachine · 2025-12-09T13:09:05Z

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

PeteGillinElastic · 2025-12-09T12:55:16Z

modules/reindex/src/main/java/org/elasticsearch/reindex/DefaultReindexRelocationNodePicker.java

+            );
+            return null;
+        }
+        List<String> eligibleDedicatedCoordinatingNodes = nodes.getCoordinatingOnlyNodes()


Question for @Tim-Brooks: Is this the correct way to find what the doc describes as "dedicated coordinating nodes"? The name sounds right, but the implementation just filters out data, master, and ingest nodes — so nodes with roles like ml or voting_only would be included.

IMO - it is likely the case that this logic is bugged. A dedicated coordinating node should have no roles as outlined by the documentation: https://www.elastic.co/docs/deploy-manage/distributed-architecture/clusters-nodes-shards/node-roles#coordinating-only-node-role.

Looking at this logic I don't think you're introducing that issue. But you are changing this code (nodes.getCoordinatingOnlyNodes ) to be used in production opposed to just tests. So either I think the logic needs to be fixed here or you need to write your own function.

I don't think it's actually 100% true that DiscoveryNodes.getCoordinatingOnlyNodes() is used only in tests now. I think there are ways it is exposed to the user, although the only ways I've found are somewhat obscure. I created #139587 for this. I agree with you that, on the face of it, this is a bug which we should fix. But since that would be a behavioural change, I think it deserves its own issue, so that we can make sure we're aligned on it, and so that it can go in the release notes.

In the meantime, I simply won't use the method in this PR.

PeteGillinElastic · 2025-12-09T13:07:23Z

modules/reindex/src/main/java/org/elasticsearch/reindex/DefaultReindexRelocationNodePicker.java

+            logger.debug("Chose dedicated coordinating node ID {} for relocating a reindex task from node {}", newNodeId, currentNodeId);
+            return newNodeId;
+        }
+        List<String> eligibleDataNodes = nodes.getDataNodes().keySet().stream().filter(id -> id.equals(currentNodeId) == false).toList();


Question for @Tim-Brooks: The doc says "a valid node for index coordinating (data node, no dedicated master nodes)". Is this the correct interpretation of that? (I wasn't quite sure what "no dedicated master nodes" meant, as my understanding was that would mean nodes with only the master role, which therefore could never be a data node.)

Yes. This looks fine to me (can contain data excluded dedicated master nodes).

Tim-Brooks

All the plugin infrastructure here looks good. But I do think we need to use the proper definition of coordinating only node which is nodes without any assigned roles.

Tim-Brooks · 2025-12-15T23:27:50Z

modules/reindex/src/main/java/org/elasticsearch/reindex/DefaultReindexRelocationNodePicker.java

+            logger.debug("Chose dedicated coordinating node ID {} for relocating a reindex task from node {}", newNodeId, currentNodeId);
+            return newNodeId;
+        }
+        List<String> eligibleDataNodes = nodes.getDataNodes().keySet().stream().filter(id -> id.equals(currentNodeId) == false).toList();


Yes. This looks fine to me (can contain data excluded dedicated master nodes).

Tim-Brooks · 2025-12-15T23:34:38Z

modules/reindex/src/main/java/org/elasticsearch/reindex/DefaultReindexRelocationNodePicker.java

+            );
+            return null;
+        }
+        List<String> eligibleDedicatedCoordinatingNodes = nodes.getCoordinatingOnlyNodes()


IMO - it is likely the case that this logic is bugged. A dedicated coordinating node should have no roles as outlined by the documentation: https://www.elastic.co/docs/deploy-manage/distributed-architecture/clusters-nodes-shards/node-roles#coordinating-only-node-role.

Looking at this logic I don't think you're introducing that issue. But you are changing this code (nodes.getCoordinatingOnlyNodes ) to be used in production opposed to just tests. So either I think the logic needs to be fixed here or you need to write your own function.

…#139587

…node-picker

PeteGillinElastic

Thanks, PTAL.

PeteGillinElastic · 2025-12-16T11:32:59Z

modules/reindex/src/main/java/org/elasticsearch/reindex/DefaultReindexRelocationNodePicker.java

+            );
+            return null;
+        }
+        List<String> eligibleDedicatedCoordinatingNodes = nodes.getCoordinatingOnlyNodes()


I don't think it's actually 100% true that DiscoveryNodes.getCoordinatingOnlyNodes() is used only in tests now. I think there are ways it is exposed to the user, although the only ways I've found are somewhat obscure. I created #139587 for this. I agree with you that, on the face of it, this is a bug which we should fix. But since that would be a behavioural change, I think it deserves its own issue, so that we can make sure we're aligned on it, and so that it can go in the release notes.

In the meantime, I simply won't use the method in this PR.

PeteGillinElastic · 2025-12-16T12:06:23Z

Thanks, PTAL.

Just in case, repeating this reply which I left but might have been stranded 'cos the code it was attached to went away:

I don't think it's actually 100% true that DiscoveryNodes.getCoordinatingOnlyNodes() is used only in tests now. I think there are ways it is exposed to the user, although the only ways I've found are somewhat obscure. I created #139587 for this. I agree with you that, on the face of it, this is a bug which we should fix. But since that would be a behavioural change, I think it deserves its own issue, so that we can make sure we're aligned on it, and so that it can go in the release notes.

In the meantime, I simply won't use the method in this PR.

…node-picker

Tim-Brooks

LGTM

assuming the test failure is not related.

thanks for creating the issue.

* upstream/main: (253 commits) Adds ST_SIMPLIFY geo spatial function (elastic#136309) Take control of max clause count verification in Lucene searcher (elastic#139752) [ML] Unmute Inference Test (elastic#139765) Parameterize the vector operation benchmark tests (elastic#139735) Fix node reduction pushdown tests for release tests (elastic#139548) Fix flakiness in TSDataGenerationHelper (elastic#139759) CPS: Copy existing resolved index expressions when constructing a new `SearchRequest` from an existing one (elastic#139596) Add release notes for v9.1.9 release (elastic#139674) Add lucene query for wildcards on high cardinality keyword fields. (elastic#139746) Suppress Tika entitlement warnings from AWT (elastic#139711) Check field storage when synthetic source is enabled, in tests (elastic#139715) Refactor VectorSimilarityType to know about its corresponding Function (elastic#139678) Merge fixes from patch branch back into main (elastic#139721) Define native bulk operations for vector square distance (elastic#139198) Use LongUpDownCounter for Linked Project Error Metrics (elastic#139657) ESQL: Add javadoc that explains version-aware planning (elastic#139706) Add helper to pick node for reindex relocation (elastic#139081) Fix auth serialization randomized version test (elastic#139182) ES|QL - Add parsing, preanalysis and analysis timing information to profile (elastic#139540) Mute org.elasticsearch.persistent.ClusterPersistentTasksCustomMetadataTests testMinVersionSerialization elastic#139741 ...

This adds an implmentation of the helper which picks a node to relocate a reindex task to on shutdown to be used in stateless. This changes the mechanism for choosing the correct implementation that was added in elastic#139081. The previous mechanism assumed that we'd want *serverless* to behave differently, and so it defined a default implementation and allowed for that to be swapped out via another plugin. But we actually want *stateless* to behave differently, so we can take the simpler approach of doing everything inside the `reindex` plugin and just pick the correct implementation based on settings. The implementation added previously has been renamed, because it is now explicitly used in stateful rather than just being used by default.

This adds an implmentation of the helper which picks a node to relocate a reindex task to on shutdown to be used in stateless. This changes the mechanism for choosing the correct implementation that was added in #139081. The previous mechanism assumed that we'd want *serverless* to behave differently, and so it defined a default implementation and allowed for that to be swapped out via another plugin. But we actually want *stateless* to behave differently, so we can take the simpler approach of doing everything inside the `reindex` plugin and just pick the correct implementation based on settings. The implementation added previously has been renamed, because it is now explicitly used in stateful rather than just being used by default. Co-authored-by: Szymon Bialkowski <szybia@tuta.io> Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

This adds an implmentation of the helper which picks a node to relocate a reindex task to on shutdown to be used in stateless. This changes the mechanism for choosing the correct implementation that was added in elastic#139081. The previous mechanism assumed that we'd want *serverless* to behave differently, and so it defined a default implementation and allowed for that to be swapped out via another plugin. But we actually want *stateless* to behave differently, so we can take the simpler approach of doing everything inside the `reindex` plugin and just pick the correct implementation based on settings. The implementation added previously has been renamed, because it is now explicitly used in stateful rather than just being used by default. Co-authored-by: Szymon Bialkowski <szybia@tuta.io> Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

PeteGillinElastic added >non-issue :Distributed/Reindex Issues relating to reindex that are not caused by issues further down labels Dec 4, 2025

elasticsearchmachine added the v9.3.0 label Dec 4, 2025

PeteGillinElastic force-pushed the reindex-relocation-node-picker branch from 24788c9 to f86685f Compare December 9, 2025 13:03

PeteGillinElastic marked this pull request as ready for review December 9, 2025 13:08

elasticsearchmachine added the Team:Distributed Indexing (obsolete) Meta label for Distributed Indexing team. Obsolete. Please do not use. label Dec 9, 2025

PeteGillinElastic commented Dec 9, 2025

View reviewed changes

Merge branch 'main' into reindex-relocation-node-picker

7ba85b5

Tim-Brooks reviewed Dec 15, 2025

View reviewed changes

PeteGillinElastic added 2 commits December 16, 2025 12:02

Stop using DiscoveryNodes.getCoordinatingOnlyNodes() due to elastic…

dc669d7

…#139587

Merge remote-tracking branch 'upstream/main' into reindex-relocation-…

f849562

…node-picker

PeteGillinElastic commented Dec 16, 2025

View reviewed changes

PeteGillinElastic requested a review from Tim-Brooks December 16, 2025 12:06

PeteGillinElastic added 3 commits December 16, 2025 13:35

self-nit: omit trailing periods in log messages

14c7a9c

Merge remote-tracking branch 'upstream/main' into reindex-relocation-…

52683fd

…node-picker

Merge branch 'main' into reindex-relocation-node-picker

2fe7fa8

elasticsearchmachine added v9.4.0 and removed v9.3.0 labels Dec 17, 2025

Tim-Brooks approved these changes Dec 18, 2025

View reviewed changes

Merge branch 'main' into reindex-relocation-node-picker

799e245

PeteGillinElastic merged commit 76bfeb4 into elastic:main Dec 18, 2025
35 checks passed

PeteGillinElastic deleted the reindex-relocation-node-picker branch December 18, 2025 12:42

PeteGillinElastic mentioned this pull request Jan 7, 2026

Add stateless ReindexRelocationNodePicker impl #140267

Merged

Conversation

PeteGillinElastic commented Dec 4, 2025 • edited by szybia Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Dec 9, 2025

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!

Tim-Brooks 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!

PeteGillinElastic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeteGillinElastic commented Dec 16, 2025

Uh oh!

Tim-Brooks left a comment • 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PeteGillinElastic commented Dec 4, 2025 •

edited by szybia

Loading

Tim-Brooks left a comment •

edited

Loading