Use TaskId in TaskAwareRequest.createRequest #130131

GalLalouche · 2025-06-26T17:18:12Z

This PR resolves the TODOs introduced in #127472.

Use TaskId instead of passing a node name and ID individually.
After consideration, I've also decided to remove the other TODO in this file. Forcing the hundred+ implementors to use the version with the task ID isn't better design and just introduces stamp coupling to those implementations.

Use TaskId instead of passing a node name and ID individually.

elasticsearchmachine · 2025-06-30T06:55:02Z

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

rjernst · 2025-06-30T19:07:53Z

server/src/main/java/org/elasticsearch/tasks/TaskManager.java

@@ -141,9 +149,9 @@ public Task register(String type, String action, TaskAwareRequest request, boole
                headers.put(key, httpHeader);
            }
        }
+        String localNodeId = Optional.ofNullable(lastDiscoveryNodes.getLocalNodeId()).orElse(initialNodeId);


The node id from NodeEnvironment isn't the "initial" node id, it is the node id. That's what is passed in to the local node in DiscoveryNodes, see the LocalNodeFactory in NodeConstruction.

But can't lastDiscoverNodes change, thereby changing the node ID? Or is the applyClusterState not really meant for mutation, but rather is just a way to set this value post-construction?

The local node in DiscoveryNodes won't effectively change, see DiscoveryNodes.readFrom where the local node id is passed in, not ready from the wire.

OK, I've renamed this to nodeId, and added a comment on the field to make its existence clearer.

rjernst · 2025-06-30T23:03:30Z

server/src/main/java/org/elasticsearch/tasks/TaskManager.java

@@ -141,9 +150,9 @@ public Task register(String type, String action, TaskAwareRequest request, boole
                headers.put(key, httpHeader);
            }
        }
+        String localNodeId = Optional.ofNullable(lastDiscoveryNodes.getLocalNodeId()).orElse(nodeId);


Sorry I didn't make my point clear: there is no need to look at DiscoveryNodes at all, this is the node id, it can't change.

Ah, got it! I've replaced to code to only use nodeId.

…alouche/elasticsearch into refactor/task_aware_request_task_id

rjernst

seems there are a few remaining places calling it "initial" node id. Also, the PR title should be updated, the "TODO refactor" is not needed

rjernst · 2025-07-01T16:25:52Z

test/framework/src/main/java/org/elasticsearch/test/transport/MockTransportService.java

+        ThreadPool threadPool,
+        Set<String> taskHeaders,
+        Tracer tracer,
+        @Nullable String initialNodeId


This should not be nullable, and also not "initial"?

Good catch! I've went over and removed all new @Nullable, and renamed initialNodeId to nodeId.

GalLalouche · 2025-07-02T11:12:54Z

plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/EC2RetriesTests.java

@@ -59,9 +59,7 @@ protected MockTransportService createTransportService() {
                new NoneCircuitBreakerService(),
                new SharedGroupFactory(Settings.EMPTY)
            ),
-            threadPool,
-            TransportService.NOOP_TRANSPORT_INTERCEPTOR,


It seemed all clients passed the same Settings.EMPTY, interceptor, and null in the clusterSettings to this constructor (refactored to a factory method), so I've simplified the method by removing those parameters.

rjernst

LGTM

rjernst · 2025-07-03T19:55:53Z

server/src/main/java/org/elasticsearch/tasks/TaskManager.java

@@ -96,10 +98,15 @@ public TaskManager(Settings settings, ThreadPool threadPool, Set<String> taskHea
    }

    public TaskManager(Settings settings, ThreadPool threadPool, Set<String> taskHeaders, Tracer tracer) {


please mention with a comment this is just for tests (creating the uuid). we should move all these uses to explicitly pass a test nodeid, but that can be a followup

GalLalouche · 2025-07-06T11:58:45Z

Thanks for the review @rjernst!

GalLalouche added :Distributed Coordination/Task Management Issues for anything around the Tasks API - both persistent and node level. >refactoring Team:Distributed Coordination Meta label for Distributed Coordination team labels Jun 26, 2025

elasticsearchmachine added the v9.2.0 label Jun 26, 2025

TODO refactor: Use TaskId in TaskAwareRequest.createRequest

0f9d2bd

Use TaskId instead of passing a node name and ID individually.

GalLalouche force-pushed the refactor/task_aware_request_task_id branch from 429db8b to 0f9d2bd Compare June 29, 2025 14:43

GalLalouche requested a review from rjernst June 30, 2025 06:54

GalLalouche marked this pull request as ready for review June 30, 2025 06:54

GalLalouche requested a review from a team as a code owner June 30, 2025 06:54

rjernst reviewed Jun 30, 2025

View reviewed changes

GalLalouche added 2 commits June 30, 2025 23:17

CR fixes

717d483

Merge branch 'main' into refactor/task_aware_request_task_id

9cbfdf2

rjernst reviewed Jun 30, 2025

View reviewed changes

GalLalouche added 3 commits July 1, 2025 13:52

CR fixes

7594b8b

Merge branch 'refactor/task_aware_request_task_id' of github.com:GalL…

be57c9b

…alouche/elasticsearch into refactor/task_aware_request_task_id

Merge branch 'main' into refactor/task_aware_request_task_id

28eefa8

rjernst reviewed Jul 1, 2025

View reviewed changes

GalLalouche added 3 commits July 2, 2025 14:02

More CR fixes

2a6102e

Merge branch 'main' into refactor/task_aware_request_task_id

8216022

Simplify new static method

d4de01c

GalLalouche force-pushed the refactor/task_aware_request_task_id branch from 7f0b984 to d4de01c Compare July 2, 2025 11:10

GalLalouche changed the title ~~TODO refactor: Use TaskId in TaskAwareRequest.createRequest~~ Use TaskId in TaskAwareRequest.createRequest Jul 2, 2025

GalLalouche commented Jul 2, 2025

View reviewed changes

GalLalouche requested a review from rjernst July 2, 2025 11:14

elasticsearchmachine and others added 2 commits July 2, 2025 11:20

[CI] Auto commit changes from spotless

a1a5803

Fix compilation error

57324d9

rjernst approved these changes Jul 3, 2025

View reviewed changes

GalLalouche added 2 commits July 6, 2025 14:36

Merge branch 'main' into refactor/task_aware_request_task_id

164de88

CR Fixes

8fe47d9

GalLalouche force-pushed the refactor/task_aware_request_task_id branch from 26af770 to 8fe47d9 Compare July 6, 2025 11:58

GalLalouche enabled auto-merge (squash) July 6, 2025 11:58

GalLalouche merged commit 96315ec into elastic:main Jul 6, 2025
32 checks passed

		@@ -96,10 +98,15 @@ public TaskManager(Settings settings, ThreadPool threadPool, Set<String> taskHea
		}

		public TaskManager(Settings settings, ThreadPool threadPool, Set<String> taskHeaders, Tracer tracer) {

Use TaskId in TaskAwareRequest.createRequest #130131

Use TaskId in TaskAwareRequest.createRequest #130131

Uh oh!

Conversation

GalLalouche commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jun 30, 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!

Choose a reason for hiding this comment

Uh oh!

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

GalLalouche Jul 2, 2025 • 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!

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

GalLalouche commented Jul 6, 2025

Uh oh!

Uh oh!

Uh oh!

GalLalouche commented Jun 26, 2025 •

edited

Loading

GalLalouche Jul 2, 2025 •

edited

Loading