-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Adding background task to mark removed task types as unrecognized
#199057
Conversation
4791b9e
to
5d27ab3
Compare
@@ -95,6 +95,34 @@ export class SampleTaskManagerFixturePlugin | |||
return res.ok({ body: {} }); | |||
} | |||
); | |||
|
|||
router.post( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all the functional tests that test this functionality load data from an ES archive. doing this overwrites the task manager index so we have to ensure this task is scheduled before scheduling it to run.
unrecognized
f16a7c1
to
6e43d46
Compare
Pinging @elastic/response-ops (Team:ResponseOps) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I'd like some changes:
- moving the body of the try in the run() method of the task
- change search to not return source since we don't need it
Everything else is just commentary ...
...
argggggghhhhh
I did this entire PR review in VS code, and it published my review comment, but none of the individual comments. FFS. Will do another review here, copying it over ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welp, here are the two comments I wanted to leave in the previous review. Can't find the other comments I made, but they weren't important.
Lesson learned: vscode PR reviews are not ready for prime time, at least for commenting / submitting the review
return () => { | ||
return { | ||
async run() { | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to have the body of the try
moved to a local function, to make this easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 9bb0899
const esClient = elasticsearch.client.asInternalUser; | ||
|
||
const result = await esClient.search<ConcreteTaskInstance>({ | ||
index: TASK_MANAGER_INDEX, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make sure this search doesn't return source, since we don't need it, and don't know how big it could be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! Updated in 9bb0899
💚 Build Succeeded
Metrics [docs]
History
cc @ymao1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks for the changes!
Starting backport for target branches: 8.x |
…ask types as `unrecognized` (elastic#199057) Resolves elastic#192686 ## Summary Creates a background task to search for removed task types and mark them as unrecognized. Removes the current logic that does this during the task claim cycle for both task claim strategies. --------- Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit be949d6)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…ask types as `unrecognized` (elastic#199057) Resolves elastic#192686 ## Summary Creates a background task to search for removed task types and mark them as unrecognized. Removes the current logic that does this during the task claim cycle for both task claim strategies. --------- Co-authored-by: kibanamachine <[email protected]>
…oved task types as `unrecognized` (#199057) (#199706) # Backport This will backport the following commits from `main` to `8.x`: - [[Response Ops][Task Manager] Adding background task to mark removed task types as `unrecognized` (#199057)](#199057) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Ying Mao","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-11T20:17:46Z","message":"[Response Ops][Task Manager] Adding background task to mark removed task types as `unrecognized` (#199057)\n\nResolves https://github.com/elastic/kibana/issues/192686\r\n\r\n## Summary\r\n\r\nCreates a background task to search for removed task types and mark them\r\nas unrecognized. Removes the current logic that does this during the\r\ntask claim cycle for both task claim strategies.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"be949d66e43ae24e9bce4a13a4613ad00e1dce9a","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":"[Response Ops][Task Manager] Adding background task to mark removed task types as `unrecognized`","number":199057,"url":"https://github.com/elastic/kibana/pull/199057","mergeCommit":{"message":"[Response Ops][Task Manager] Adding background task to mark removed task types as `unrecognized` (#199057)\n\nResolves https://github.com/elastic/kibana/issues/192686\r\n\r\n## Summary\r\n\r\nCreates a background task to search for removed task types and mark them\r\nas unrecognized. Removes the current logic that does this during the\r\ntask claim cycle for both task claim strategies.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"be949d66e43ae24e9bce4a13a4613ad00e1dce9a"}},"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/199057","number":199057,"mergeCommit":{"message":"[Response Ops][Task Manager] Adding background task to mark removed task types as `unrecognized` (#199057)\n\nResolves https://github.com/elastic/kibana/issues/192686\r\n\r\n## Summary\r\n\r\nCreates a background task to search for removed task types and mark them\r\nas unrecognized. Removes the current logic that does this during the\r\ntask claim cycle for both task claim strategies.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"be949d66e43ae24e9bce4a13a4613ad00e1dce9a"}},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Ying Mao <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
…ask types as `unrecognized` (elastic#199057) Resolves elastic#192686 ## Summary Creates a background task to search for removed task types and mark them as unrecognized. Removes the current logic that does this during the task claim cycle for both task claim strategies. --------- Co-authored-by: kibanamachine <[email protected]>
Resolves #192686
Summary
Creates a background task to search for removed task types and mark them as unrecognized. Removes the current logic that does this during the task claim cycle for both task claim strategies.