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] Add bulk update function that directly updates using the esClient #191760

Merged
merged 9 commits into from
Sep 10, 2024

Conversation

ymao1
Copy link
Contributor

@ymao1 ymao1 commented Aug 29, 2024

Resolves #187704

Summary

Creates a new bulkPartialUpdate function in the task_store class that uses the ES client to perform bulk partial updates instead of the Saved Objects client. Updates the update in the mget task claimer to use this new function.

To verify

Run this branch with the xpack.task_manager.claim_strategy: 'mget' and ensure that all tasks are running as expected.

@ymao1 ymao1 force-pushed the tm-partial-update branch from 09928f5 to defa6ba Compare September 3, 2024 18:30
@ymao1 ymao1 changed the title wip [Response Ops][Task Manager] Add bulk update function that directly updates using the esClient Sep 4, 2024
@ymao1 ymao1 self-assigned this Sep 9, 2024
@ymao1 ymao1 added v8.16.0 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) labels Sep 9, 2024
@@ -302,23 +307,13 @@ export class TaskStore {
*/
public async bulkUpdate(
docs: ConcreteTaskInstance[],
{ validate, excludeLargeFields = false }: BulkUpdateOpts
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these changes were originally made so we could exclude the state and params from being updated in the mget claim strategy but they are not needed anymore since we can do real partial updates through the ES client.

@kibanamachine
Copy link
Contributor

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6900

[✅] x-pack/test/task_manager_claimer_mget/config.ts: 100/100 tests passed.

see run history

@ymao1 ymao1 marked this pull request as ready for review September 9, 2024 17:59
@ymao1 ymao1 requested a review from a team as a code owner September 9, 2024 17:59
@elasticmachine
Copy link
Contributor

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

@ymao1 ymao1 requested review from pmuellr and guskovaue September 9, 2024 18:00
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 and works great!

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM, left a nit comment

task.status = TaskStatus.Unrecognized;
tasksToRemoveUpdates.push({
id: task.id,
version: task.version,
Copy link
Member

Choose a reason for hiding this comment

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

I think we probably have the seqNum/primaryTerm for these, in docLatestVersions, however I'm guessing you used the version to save from having to create another extremely similar type :-)

I'm wondering, for the removal of unrecognized tasks, maybe we don't even need the version? These docs shouldn't be updated by anyone anyway, except this code to update their status.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me. Updated in e05a977

@ymao1 ymao1 enabled auto-merge (squash) September 10, 2024 22:11
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #12 / Uptime app with real-world data overview page clears pagination parameters when size changes

Metrics [docs]

✅ unchanged

History

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

cc @ymao1

@ymao1 ymao1 merged commit a141818 into elastic:main Sep 10, 2024
38 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Sep 10, 2024
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.

[Task Manager] Perform true partial updates when claiming and releasing tasks
6 participants