[task manager] do not sort tasks to be claimed by score if no pinned tasks #80692

pmuellr · 2020-10-15T15:57:36Z

resolves: #80371

Previously, when claiming tasks, we were always sorting the tasks to claim by
the score and then by the time they should be run. We sort by score to
capture runNow() tasks, also referred to internally as "pinned" tasks
in the update by query.

The change in this PR is to only sort by score if there are pinned tasks, and
to not sort by score at all if there aren't any.

pmuellr · 2020-10-15T20:13:03Z

It would be nice to add some kind of "load" functional test for this. That could test 2 things:

that we don't end up with "zombie" tasks
that runNow() tasks still run as soon as possible

The first is tough, as I've only managed to see this in one case with ~100K tasks queued to run. And if we use the default poll interval and worker size, we can only run about 10 workers every 3 seconds (can't remember if we poll earlier if there are workers finishing before the poll interval goes off). Perhaps we need a script to do an ad-hoc test we can run over the course of a few hours.

The second should be easier, I think. Queue up a bunch of tasks (say 100), then do a runNow(), and it should fire within 2 * the next poll interval (I think). [edit: we already have some here]

gmmorris · 2020-10-20T17:28:59Z

x-pack/plugins/task_manager/server/task_store.ts

@@ -278,6 +279,12 @@ export class TaskStore {
      )
    );

+    // the sort should use score first, but only if there are pinned tasks


Can we add a comment about score being lower for old tasks and that that is the opposite to what TM actually needs, so whenever score is used TM is likely to pick up newer expired tasks rather than the older ones.

I'll add more explanation - but it's not entirely clear to me that the score IS strictly lower for older tasks - I didn't actually go down the rabbit hole to figure out how the scores were calculated :-). That would be my guess as well - that older docs score lower - but sometimes things actually worked out fairly well, so I think there may be more to it than just that. I've been assuming the score is basically a random number at this point, except for the pinned tasks that will score better ...

gmmorris · 2020-10-20T17:29:46Z

LG but we should definitely test this, at least at unit level.

pmuellr · 2020-10-20T18:57:39Z

LG but we should definitely test this, at least at unit level.

Yup, was planning a jest test, not quite sure how to build a test for this that can run as a function test though.

…tasks resolves: elastic#80371 Previously, when claiming tasks, we were always sorting the tasks to claim by the score and then by the time they should be run. We sort by score to capture `runNow()` tasks, also referred to internally as "pinned" tasks in the update by query. The change in this PR is to only sort by score if there are pinned tasks, and to not sort by score at all if there aren't any.

kibanamachine · 2020-10-20T22:43:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: b181ff6

Metrics [docs]

✅ unchanged

History

💚 Build #82912 succeeded d641462
💚 Build #82878 succeeded d2930d4e61a7f9e33afa76eb5f1bea081b68fb1a
💚 Build #82858 succeeded d2930d4e61a7f9e33afa76eb5f1bea081b68fb1a
💚 Build #81937 succeeded d2930d4e61a7f9e33afa76eb5f1bea081b68fb1a

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

elasticmachine · 2020-10-21T03:42:39Z

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

ymao1

Makes sense. LGTM!

gmmorris

LGTM 👍

…tasks (elastic#80692) resolves: elastic#80371 Previously, when claiming tasks, we were always sorting the tasks to claim by the score and then by the time they should be run. We sort by score to capture `runNow()` tasks, also referred to internally as "pinned" tasks in the update by query. The change in this PR is to only sort by score if there are pinned tasks, and to not sort by score at all if there aren't any.

…tasks (elastic#80692) resolves: elastic#80371 Previously, when claiming tasks, we were always sorting the tasks to claim by the score and then by the time they should be run. We sort by score to capture `runNow()` tasks, also referred to internally as "pinned" tasks in the update by query. The change in this PR is to only sort by score if there are pinned tasks, and to not sort by score at all if there aren't any. # Conflicts: # x-pack/plugins/task_manager/server/task_store.test.ts

…tasks (#80692) (#81495) resolves: #80371 Previously, when claiming tasks, we were always sorting the tasks to claim by the score and then by the time they should be run. We sort by score to capture `runNow()` tasks, also referred to internally as "pinned" tasks in the update by query. The change in this PR is to only sort by score if there are pinned tasks, and to not sort by score at all if there aren't any.

…pinned tasks (#80692) (#81497) resolves: #80371 Previously, when claiming tasks, we were always sorting the tasks to claim by the score and then by the time they should be run. We sort by score to capture `runNow()` tasks, also referred to internally as "pinned" tasks in the update by query. The change in this PR is to only sort by score if there are pinned tasks, and to not sort by score at all if there aren't any. Also had to fix type check error after fixing a. merge conflict during the backport:

pmuellr mentioned this pull request Oct 15, 2020

[Task Manager] old idle task not run when there's a large backlog of idle tasks #80371

Closed

pmuellr mentioned this pull request Oct 16, 2020

Task remains in "running" state when last attempt timed out #79165

Closed

gmmorris self-requested a review October 19, 2020 08:59

gmmorris reviewed Oct 20, 2020

View reviewed changes

pmuellr force-pushed the task-manager/old-zombie-tasks branch from d2930d4 to d641462 Compare October 20, 2020 20:34

add function test, provide better comment on sort

b181ff6

pmuellr added v7.11.0 v8.0.0 Feature:Task Manager Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) release_note:skip Skip the PR/issue when compiling release notes labels Oct 21, 2020

pmuellr marked this pull request as ready for review October 21, 2020 03:42

pmuellr requested a review from a team as a code owner October 21, 2020 03:42

ymao1 approved these changes Oct 21, 2020

View reviewed changes

gmmorris approved these changes Oct 22, 2020

View reviewed changes

pmuellr added the v7.10.0 label Oct 22, 2020

pmuellr merged commit 069e842 into elastic:master Oct 22, 2020

pmuellr mentioned this pull request Oct 22, 2020

[7.x] [task manager] do not sort tasks to be claimed by score if no pinned tasks (#80692) #81495

Merged

pmuellr mentioned this pull request Oct 22, 2020

[7.10] [task manager] do not sort tasks to be claimed by score if no pinned tasks (#80692) #81497

Merged

pmuellr added the backported label Oct 22, 2020

pmuellr mentioned this pull request Dec 1, 2020

[Alerting] Index threshold: Actions not fired as expected #84335

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[task manager] do not sort tasks to be claimed by score if no pinned tasks #80692

[task manager] do not sort tasks to be claimed by score if no pinned tasks #80692

pmuellr commented Oct 15, 2020

pmuellr commented Oct 15, 2020 •

edited

Loading

gmmorris Oct 20, 2020

pmuellr Oct 20, 2020

gmmorris commented Oct 20, 2020

pmuellr commented Oct 20, 2020

kibanamachine commented Oct 20, 2020

elasticmachine commented Oct 21, 2020

ymao1 left a comment

gmmorris left a comment

[task manager] do not sort tasks to be claimed by score if no pinned tasks #80692

[task manager] do not sort tasks to be claimed by score if no pinned tasks #80692

Conversation

pmuellr commented Oct 15, 2020

pmuellr commented Oct 15, 2020 • edited Loading

gmmorris Oct 20, 2020

Choose a reason for hiding this comment

pmuellr Oct 20, 2020

Choose a reason for hiding this comment

gmmorris commented Oct 20, 2020

pmuellr commented Oct 20, 2020

kibanamachine commented Oct 20, 2020

💚 Build Succeeded

Metrics [docs]

History

elasticmachine commented Oct 21, 2020

ymao1 left a comment

Choose a reason for hiding this comment

gmmorris left a comment

Choose a reason for hiding this comment

pmuellr commented Oct 15, 2020 •

edited

Loading