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

[Response Ops][Task Manager] Setting task status directly to running in mget claim strategy #191669

Merged
merged 6 commits into from
Sep 3, 2024

Conversation

ymao1
Copy link
Contributor

@ymao1 ymao1 commented Aug 28, 2024

Resolves #184739

Summary

During the mget task claim strategy, we set the task directly to running with the appropriate retryAt value instead of setting it to claiming and letting the task runner set the task to running. This removes a task update from the claiming process. Updates the task runner to skip the markTaskAsRunning function if the claim strategy is mget and move the task directly to "ready to run".

@ymao1 ymao1 changed the title Setting task directly to running in mget claim strategy [Response Ops][Task Manager] Setting task status directly to running in mget claim strategy Aug 29, 2024
@ymao1 ymao1 self-assigned this Aug 29, 2024
@ymao1 ymao1 added release_note:skip Skip the PR/issue when compiling release notes Feature:Task Manager Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.16.0 labels Aug 29, 2024
@ymao1 ymao1 marked this pull request as ready for review August 29, 2024 13:17
@ymao1 ymao1 requested a review from a team as a code owner August 29, 2024 13:17
@elasticmachine
Copy link
Contributor

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

@ymao1 ymao1 requested review from pmuellr and js-jankisalvi August 29, 2024 13:17
Copy link
Contributor

@js-jankisalvi js-jankisalvi left a comment

Choose a reason for hiding this comment

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

code review only, changes look good 👍

@ymao1
Copy link
Contributor Author

ymao1 commented Sep 3, 2024

@elasticmachine merge upstream

Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Changes LGTM! Tested locally using update_by_query and mget and worked as expected.

@ymao1
Copy link
Contributor Author

ymao1 commented Sep 3, 2024

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

✅ unchanged

History

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

cc @ymao1

@ymao1 ymao1 merged commit ba0485e into elastic:main Sep 3, 2024
40 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Sep 3, 2024
@ymao1 ymao1 deleted the tm-mget-skip-claiming branch September 3, 2024 16:26
ymao1 added a commit to ymao1/kibana that referenced this pull request Sep 6, 2024
…`running` in `mget` claim strategy (elastic#191669)"

This reverts commit ba0485e.
ymao1 added a commit that referenced this pull request Sep 6, 2024
## Summary

Reverting PR #191669 because it is
causing a flaky test #192023
that requires additional investigation.

Unskipped the flaky test and ran the flaky test runner against this PR
ymao1 added a commit to ymao1/kibana that referenced this pull request Sep 6, 2024
… `running` in `mget` claim strategy (elastic#191669)"

This reverts commit 689f227.
ymao1 added a commit that referenced this pull request Sep 9, 2024
… `running` in `mget` claim strategy (#192303)

Re-doing this PR: #191669

Reverted because it was causing a flaky test. After a lot of
investigation, it looks like the flakiness was caused by interference
from long-running tasks scheduled as part of other tests. The task
partitions test uses task IDs `1`, `2` and `3` and the tasks were being
short circuited when there were other tasks with UUIDs that started with
`1`, `2` or `3` due to the logic in the task runner that tries to
prevent duplicate recurring tasks from running. That logic just used
`startsWith` to test for duplicates where the identifier is
`${task.id}::${task.executionUUID}`. Updated that logic instead to check
for duplicate `task.id` instead of just using `startsWith` in this
commit:
1646ae9

---------

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting 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.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the mget task claimer skip the claiming phase and update the task document directly to running
6 participants