TransportGetTaskAction: Wait for the task asynchronously by arteam · Pull Request #93375 · elastic/elasticsearch

arteam · 2023-01-31T10:34:27Z

Wait for the requested task asynchronously in a similar fashion to TransportListTaskAction from #90977

See #90977

DaveCTurner

Nice work 😄

.../main/java/org/elasticsearch/action/admin/cluster/node/tasks/get/TransportGetTaskAction.java

arteam · 2023-01-31T14:07:29Z

@elasticmachine update branch

DaveCTurner

Looks good, now it just needs a test 😁

.../main/java/org/elasticsearch/action/admin/cluster/node/tasks/get/TransportGetTaskAction.java

…de/tasks/get/TransportGetTaskAction.java Co-authored-by: David Turner <david.turner@elastic.co>

…tion-async

arteam · 2023-02-01T12:33:23Z

@elasticmachine update branch

arteam · 2023-02-01T18:02:41Z

modules/reindex/src/yamlRestTest/resources/rest-api-spec/test/reindex/30_search.yml

  - do:
+      # Inherits the warning from the previous task
+      warnings:
+        - The sort option in reindex is deprecated. Instead consider using query


It looks like with the latest change getTask now inherits warnings from the previous task (not sure why it wasn't the case with a blocking call in the generic pool?) which is technically is a breaking change :(

I suspect this is because we call the RemovedTaskListener (and therefore complete the future) in the thread context of the task that's being removed. We need to fix that, we can't just leak context across responses like this.

elasticsearchmachine · 2023-02-01T18:04:17Z

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

elasticsearchmachine · 2023-02-01T18:04:17Z

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

DaveCTurner · 2023-02-01T18:46:10Z

modules/reindex/src/yamlRestTest/resources/rest-api-spec/test/reindex/30_search.yml

  - do:
+      # Inherits the warning from the previous task
+      warnings:
+        - The sort option in reindex is deprecated. Instead consider using query


I suspect this is because we call the RemovedTaskListener (and therefore complete the future) in the thread context of the task that's being removed. We need to fix that, we can't just leak context across responses like this.

DaveCTurner · 2023-02-01T18:50:16Z

.../main/java/org/elasticsearch/action/admin/cluster/node/tasks/get/TransportGetTaskAction.java

+                    // The task has already finished, we can run the completion listener in the same thread
+                    waitedForCompletionListener.onResponse(null);
+                } else {
+                    future.addListener(waitedForCompletionListener);


... like this

Suggested change

future.addListener(waitedForCompletionListener);

future.addListener(

ContextPreservingActionListener.wrapPreservingContext(waitedForCompletionListener, threadPool.getThreadContext())

);

We also need to do this for the list-tasks action, and ideally we'd have a test for it there too.

Ah sorry this was wrong, when we wrap it like this the response headers will still propagate. We have to do it like this to drop the response headers:

new ContextPreservingActionListener<>(threadPool.getThreadContext().newRestorableContext(false), waitedForCompletionListener)

…de/tasks/get/TransportGetTaskAction.java Co-authored-by: David Turner <david.turner@elastic.co>

DaveCTurner · 2023-02-02T07:17:19Z

#93431 (almost) adds a test for this. The only missing bit is the // TODO line. So if we merge #93431 first we can uncomment that line in this PR and then it's good to go.

…tion-async

DaveCTurner · 2023-02-02T10:46:04Z

...r/src/internalClusterTest/java/org/elasticsearch/action/admin/cluster/tasks/ListTasksIT.java


        // briefly fill up the generic pool so that (a) we know the wait has started and (b) we know it's not blocking
-        // flushThreadPool(threadPool, ThreadPool.Names.GENERIC); // TODO it _is_ blocking right now!!, unmute this in #93375
+        flushThreadPool(threadPool, ThreadPool.Names.GENERIC);


Hm now that I look at this again, with these changes we're not using the GENERIC pool any more (indeed there's no forking at all when waiting for completion) so this line doesn't make sense. We can just drop this (and therefore inline flushThreadPool at its only other call-site).

This reverts commit 9f6a7bb.

…tion-async

DaveCTurner · 2023-02-06T08:07:32Z

Just a couple of outstanding comments here but I'd quite like to get this in today @arteam - let us know if you won't have capacity and I'll find some elsewhere.

…tion-async

arteam · 2023-02-06T13:52:38Z

@DaveCTurner I spent some time looking at it on Thursday, but becase the reindex/30_search/Sorting deprecated wait_for_completion false test was still failing due to inheriting of deprecation messages.

Using ContextPreservingActionListener.wrapPreservingContext didn't help there. Also, unmuting the flushThreadPool lined in failed the ListTasksIT test.

DaveCTurner · 2023-02-06T14:03:25Z

Hmm, it seems to work for me? I pushed the fix I think we need, let's see what I'm missing in CI.

I think we don't need the flushThreadPool call either, see #93375 (comment).

arteam · 2023-02-06T14:07:41Z

Thank you! I missed the comment about using newRestorableContext(false) and committed the first suggestion. It's obvious in the retrospective why the test failed.

DaveCTurner

LGTM 😄

elasticsearchmachine added the v8.7.0 label Jan 31, 2023

DaveCTurner reviewed Jan 31, 2023

View reviewed changes

TransportGetTaskAction: Wait for the task asynchronously

a3c20f3

arteam force-pushed the transport-get-task-action-async branch from 938eb31 to a3c20f3 Compare January 31, 2023 11:08

arteam added 3 commits January 31, 2023 12:34

Remove unused method waitForTaskCompletion

4633906

Don't fork waitedForCompletionListener

a8eba99

Remove unused imports

8d79499

Merge branch 'main' into transport-get-task-action-async

5469643

DaveCTurner reviewed Jan 31, 2023

View reviewed changes

.../main/java/org/elasticsearch/action/admin/cluster/node/tasks/get/TransportGetTaskAction.java Outdated Show resolved Hide resolved

arteam and others added 5 commits January 31, 2023 15:37

Update server/src/main/java/org/elasticsearch/action/admin/cluster/no…

d80ffb0

…de/tasks/get/TransportGetTaskAction.java Co-authored-by: David Turner <david.turner@elastic.co>

Inherit the warning from the previous task

d39b976

Merge remote-tracking branch 'origin/main' into transport-get-task-ac…

8d0b6a1

…tion-async

Propagate failure if task has been cancelled due timeout

0501970

Ignore reindex/30_search/Sorting deprecated wait_for_completion false

1445188

arteam added the :Distributed/Task Management Issues for anything around the Tasks API - both persistent and node level. label Feb 1, 2023

Merge branch 'main' into transport-get-task-action-async

c0f3883

arteam commented Feb 1, 2023

View reviewed changes

arteam marked this pull request as ready for review February 1, 2023 18:03

arteam added the >enhancement label Feb 1, 2023

elasticsearchmachine added the Team:Distributed Meta label for distributed team. label Feb 1, 2023

Update docs/changelog/93375.yaml

8b67372

DaveCTurner reviewed Feb 1, 2023

View reviewed changes

DaveCTurner mentioned this pull request Feb 1, 2023

List-tasks API response context leak #93428

Closed

arteam and others added 2 commits February 1, 2023 22:57

Update server/src/main/java/org/elasticsearch/action/admin/cluster/no…

9bd2539

…de/tasks/get/TransportGetTaskAction.java Co-authored-by: David Turner <david.turner@elastic.co>

Improve naming

0978a52

arteam added 4 commits February 2, 2023 10:29

Add missed import

7db58ea

Revert keeping warnings for getTask

395c808

Merge remote-tracking branch 'origin/main' into transport-get-task-ac…

65be7c5

…tion-async

Unmute blocking the thread pool

9f6a7bb

DaveCTurner reviewed Feb 2, 2023

View reviewed changes

arteam added 3 commits February 2, 2023 14:52

Revert "Unmute blocking the thread pool"

55f347f

This reverts commit 9f6a7bb.

Merge remote-tracking branch 'origin/main' into transport-get-task-ac…

9073672

…tion-async

Merge remote-tracking branch 'origin/main' into transport-get-task-ac…

b9f62d1

…tion-async

Merge remote-tracking branch 'origin/main' into transport-get-task-ac…

10e8958

…tion-async

DaveCTurner added 3 commits February 6, 2023 13:56

Revert

4c5a55f

Stop preserving response headers

f50e739

Remove bogus test comment

b3f415a

arteam requested a review from DaveCTurner February 6, 2023 14:55

DaveCTurner approved these changes Feb 6, 2023

View reviewed changes

arteam merged commit 7bd5613 into elastic:main Feb 6, 2023

arteam deleted the transport-get-task-action-async branch February 6, 2023 15:32

Conversation

arteam commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arteam commented Jan 31, 2023

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arteam commented Feb 1, 2023

Uh oh!

arteam Feb 1, 2023 • 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!

DaveCTurner Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Feb 1, 2023

Uh oh!

elasticsearchmachine commented Feb 1, 2023

Uh oh!

DaveCTurner Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

DaveCTurner commented Feb 2, 2023

Uh oh!

DaveCTurner Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

DaveCTurner commented Feb 6, 2023

Uh oh!

arteam commented Feb 6, 2023

Uh oh!

DaveCTurner commented Feb 6, 2023

Uh oh!

arteam commented Feb 6, 2023

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

arteam commented Jan 31, 2023 •

edited

Loading

arteam Feb 1, 2023 •

edited

Loading