[ResponseOps] Retry bulk update conflicts in task manager #147808
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ymao1
changed the title
ymao1
added
Feature:Task Manager
Team:ResponseOps
|
Pinging @elastic/response-ops (Team:ResponseOps) |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
ersin-erdal
reviewed
Dec 21, 2022
| const exponentialDelayMultiplier = getExponentialDelayMultiplier(retries); | ||
|
|
||
| await new Promise((resolve) => | ||
| setTimeout(resolve, RETRY_IF_CONFLICTS_DELAY * exponentialDelayMultiplier + randomDelayMs) |
There was a problem hiding this comment.
OOC why do we need randomDelayMs here?
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @ymao1 |
This was referenced Dec 21, 2022
mikecote
added a commit
to mikecote/kibana
that referenced
this pull request
Jan 16, 2023
…astic#147808)" This reverts commit a6232c4.
mikecote
added a commit
that referenced
this pull request
Jan 17, 2023
mikecote
added a commit
that referenced
this pull request
Jan 26, 2023
Resolves #148914 Resolves #149090 Resolves #149091 Resolves #149092 In this PR, I'm making the following Task Manager bulk APIs retry whenever conflicts are encountered: `bulkEnable`, `bulkDisable`, and `bulkUpdateSchedules`. To accomplish this, the following had to be done: - Revert the original PR (#147808) because the retries didn't load the updated documents whenever version conflicts were encountered and the approached had to be redesigned. - Create a `retryableBulkUpdate` function that can be re-used among the bulk APIs. - Fix a bug in `task_store.ts` where `version` field wasn't passed through properly (no type safety for some reason) - Remove `entity` from being returned on bulk update errors. This helped re-use the same response structure when objects weren't found - Create a `bulkGet` API on the task store so we get the latest documents prior to a ES refresh happening - Create a single mock task function that mocks task manager tasks for unit test purposes. This was necessary as other places were doing `as unknown as BulkUpdateTaskResult` and escaping type safety Flaky test runs: - [Framework] https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1776 - [Kibana Security] https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1786 Co-authored-by: kibanamachine <[email protected]>
kqualters-elastic
pushed a commit
to kqualters-elastic/kibana
that referenced
this pull request
Feb 6, 2023
Resolves elastic#148914 Resolves elastic#149090 Resolves elastic#149091 Resolves elastic#149092 In this PR, I'm making the following Task Manager bulk APIs retry whenever conflicts are encountered: `bulkEnable`, `bulkDisable`, and `bulkUpdateSchedules`. To accomplish this, the following had to be done: - Revert the original PR (elastic#147808) because the retries didn't load the updated documents whenever version conflicts were encountered and the approached had to be redesigned. - Create a `retryableBulkUpdate` function that can be re-used among the bulk APIs. - Fix a bug in `task_store.ts` where `version` field wasn't passed through properly (no type safety for some reason) - Remove `entity` from being returned on bulk update errors. This helped re-use the same response structure when objects weren't found - Create a `bulkGet` API on the task store so we get the latest documents prior to a ES refresh happening - Create a single mock task function that mocks task manager tasks for unit test purposes. This was necessary as other places were doing `as unknown as BulkUpdateTaskResult` and escaping type safety Flaky test runs: - [Framework] https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1776 - [Kibana Security] https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1786 Co-authored-by: kibanamachine <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #145316, #141849, #141864
Summary
Adds a retry on conflict error to the saved objects bulk update call made by task manager. Errors are returned by the saved object client inside an array (with a success response). Previously, we were not inspecting the response array, just returning the full data. With this PR, we are inspecting the response array specifically for conflict errors and retrying the update for just those tasks.
This
bulkUpdatefunction is used both internally by task manager and externally by the rules client. I default the number of retries to 0 for bulk updates from the task manager in order to preserve existing behavior (and in order not to increase the amount of time it takes for task manager to run) but use 3 retries when used externally.Also unskipped the two flaky disable tests and ran them through the flaky test runner 400 times with no failures.