Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove stale tasks from task conflict count during task claiming #198416

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Oct 30, 2024

In this PR, I'm removing the count of stale tasks from the number of conflicts during the claiming cycle. I am also adding a new property to the task manager health report (claim_stale_tasks) so we can track those separately to ensure we have the proper page size.

To verify

Apply the following diff, observe the new claim_stale_tasks in the TM health API and that conflicts are 0

diff --git a/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts b/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts
index 4e74454e8c9..35d7fd872d8 100644
--- a/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts
+++ b/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts
@@ -145,6 +145,7 @@ async function claimAvailableTasks(opts: TaskClaimerOpts): Promise<ClaimOwnershi
     }

     if (
+      false &&
       searchVersion.seqNo === latestVersion.seqNo &&
       searchVersion.primaryTerm === latestVersion.primaryTerm
     ) {

@mikecote mikecote added Feature:Task Manager Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Oct 30, 2024
@mikecote mikecote self-assigned this Oct 30, 2024
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

cc @mikecote

@mikecote mikecote added release_note:skip Skip the PR/issue when compiling release notes v9.0.0 backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.17.0 labels Oct 31, 2024
@mikecote mikecote marked this pull request as ready for review October 31, 2024 12:28
@mikecote mikecote requested a review from a team as a code owner October 31, 2024 12:28
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@mikecote
Copy link
Contributor Author

If someone has a better name than claim_stale_tasks, let me know! These are the sibling properties:

          duration: [],
          claim_duration: [],
          claim_conflicts: [],
          claim_mismatches: [],
          claim_stale_tasks: [],
          result_frequency_percent_as_number: [],
          persistence: [],

Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mikecote mikecote merged commit 37ebf29 into elastic:main Nov 1, 2024
45 of 46 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11629228885

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 1, 2024
…stic#198416)

In this PR, I'm removing the count of stale tasks from the number of
conflicts during the claiming cycle. I am also adding a new property to
the task manager health report (`claim_stale_tasks`) so we can track
those separately to ensure we have the proper page size.

## To verify
Apply the following diff, observe the new `claim_stale_tasks` in the TM
health API and that conflicts are 0
```
diff --git a/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts b/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts
index 4e74454e8c9..35d7fd872d8 100644
--- a/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts
+++ b/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts
@@ -145,6 +145,7 @@ async function claimAvailableTasks(opts: TaskClaimerOpts): Promise<ClaimOwnershi
     }

     if (
+      false &&
       searchVersion.seqNo === latestVersion.seqNo &&
       searchVersion.primaryTerm === latestVersion.primaryTerm
     ) {
```

(cherry picked from commit 37ebf29)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Nov 1, 2024
#198416) (#198666)

# Backport

This will backport the following commits from `main` to `8.x`:
- [Remove stale tasks from task conflict count during task claiming
(#198416)](#198416)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Mike
Côté","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-01T12:19:04Z","message":"Remove
stale tasks from task conflict count during task claiming
(#198416)\n\nIn this PR, I'm removing the count of stale tasks from the
number of\r\nconflicts during the claiming cycle. I am also adding a new
property to\r\nthe task manager health report (`claim_stale_tasks`) so
we can track\r\nthose separately to ensure we have the proper page
size.\r\n\r\n## To verify\r\nApply the following diff, observe the new
`claim_stale_tasks` in the TM\r\nhealth API and that conflicts are
0\r\n```\r\ndiff --git
a/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts
b/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts\r\nindex
4e74454e8c9..35d7fd872d8 100644\r\n---
a/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts\r\n+++
b/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts\r\n@@
-145,6 +145,7 @@ async function claimAvailableTasks(opts:
TaskClaimerOpts): Promise<ClaimOwnershi\r\n }\r\n\r\n if (\r\n+ false
&&\r\n searchVersion.seqNo === latestVersion.seqNo &&\r\n
searchVersion.primaryTerm === latestVersion.primaryTerm\r\n )
{\r\n```","sha":"37ebf29f87047e8b96e3c2cd378c647a4f2ca797","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Feature:Task
Manager","Team:ResponseOps","v9.0.0","backport:prev-minor","v8.17.0"],"title":"Remove
stale tasks from task conflict count during task
claiming","number":198416,"url":"https://github.com/elastic/kibana/pull/198416","mergeCommit":{"message":"Remove
stale tasks from task conflict count during task claiming
(#198416)\n\nIn this PR, I'm removing the count of stale tasks from the
number of\r\nconflicts during the claiming cycle. I am also adding a new
property to\r\nthe task manager health report (`claim_stale_tasks`) so
we can track\r\nthose separately to ensure we have the proper page
size.\r\n\r\n## To verify\r\nApply the following diff, observe the new
`claim_stale_tasks` in the TM\r\nhealth API and that conflicts are
0\r\n```\r\ndiff --git
a/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts
b/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts\r\nindex
4e74454e8c9..35d7fd872d8 100644\r\n---
a/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts\r\n+++
b/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts\r\n@@
-145,6 +145,7 @@ async function claimAvailableTasks(opts:
TaskClaimerOpts): Promise<ClaimOwnershi\r\n }\r\n\r\n if (\r\n+ false
&&\r\n searchVersion.seqNo === latestVersion.seqNo &&\r\n
searchVersion.primaryTerm === latestVersion.primaryTerm\r\n )
{\r\n```","sha":"37ebf29f87047e8b96e3c2cd378c647a4f2ca797"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/198416","number":198416,"mergeCommit":{"message":"Remove
stale tasks from task conflict count during task claiming
(#198416)\n\nIn this PR, I'm removing the count of stale tasks from the
number of\r\nconflicts during the claiming cycle. I am also adding a new
property to\r\nthe task manager health report (`claim_stale_tasks`) so
we can track\r\nthose separately to ensure we have the proper page
size.\r\n\r\n## To verify\r\nApply the following diff, observe the new
`claim_stale_tasks` in the TM\r\nhealth API and that conflicts are
0\r\n```\r\ndiff --git
a/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts
b/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts\r\nindex
4e74454e8c9..35d7fd872d8 100644\r\n---
a/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts\r\n+++
b/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts\r\n@@
-145,6 +145,7 @@ async function claimAvailableTasks(opts:
TaskClaimerOpts): Promise<ClaimOwnershi\r\n }\r\n\r\n if (\r\n+ false
&&\r\n searchVersion.seqNo === latestVersion.seqNo &&\r\n
searchVersion.primaryTerm === latestVersion.primaryTerm\r\n )
{\r\n```","sha":"37ebf29f87047e8b96e3c2cd378c647a4f2ca797"}},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Mike Côté <[email protected]>
nreese pushed a commit to nreese/kibana that referenced this pull request Nov 1, 2024
…stic#198416)

In this PR, I'm removing the count of stale tasks from the number of
conflicts during the claiming cycle. I am also adding a new property to
the task manager health report (`claim_stale_tasks`) so we can track
those separately to ensure we have the proper page size.

## To verify
Apply the following diff, observe the new `claim_stale_tasks` in the TM
health API and that conflicts are 0
```
diff --git a/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts b/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts
index 4e74454e8c9..35d7fd872d8 100644
--- a/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts
+++ b/x-pack/plugins/task_manager/server/task_claimers/strategy_mget.ts
@@ -145,6 +145,7 @@ async function claimAvailableTasks(opts: TaskClaimerOpts): Promise<ClaimOwnershi
     }

     if (
+      false &&
       searchVersion.seqNo === latestVersion.seqNo &&
       searchVersion.primaryTerm === latestVersion.primaryTerm
     ) {
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:Task Manager release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants