[Response Ops][Task Manager] Adding jest integration test to test capacity based claiming#189431
Conversation
ymao1
added
release_note:skip
|
|
||
| expect(taskRunAtDates.length).toBe(10); | ||
|
|
||
| // run at dates should be within a few seconds of each other |
There was a problem hiding this comment.
should be within milliseconds, but I'm adding some fudge factor
There was a problem hiding this comment.
We shall see how much fudge we will need to apply! :-)
| const searchedTypes = Array.from(taskTypes) | ||
| .concat(Array.from(removedTypes)) | ||
| .filter((type) => !excludedTypes.has(type)); | ||
| .filter((type) => !isTaskTypeExcluded(excludedTaskTypes, type)); |
There was a problem hiding this comment.
this allows us to do wildcard exclusions like alerting* instead of specifying the full task type. this is the same as what the default task claimer is doing
There was a problem hiding this comment.
thanks for fixing this bug!
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
Pinging @elastic/response-ops (Team:ResponseOps) |
|
Moving back to draft bc we had to revert the original PR |
|
@elasticmachine merge upstream |
| unsafe: { | ||
| exclude_task_types: flatMap(alphabet, (letter) => [ | ||
| `${letter}*`, | ||
| `${letter.toUpperCase()}*`, |
There was a problem hiding this comment.
trying to exclude all task types so the only ones that run are the ones registered in this integration test. I'm prefixing the test task types with _.
There was a problem hiding this comment.
Since we're using minimatch, can we just use a simple pattern like: [A-Za-z]* ?
There was a problem hiding this comment.
that's much better thanks! updated in 2c1ee89
|
@pmuellr @js-jankisalvi This is ready for review again! |
pmuellr
left a comment
pmuellr
left a comment
There was a problem hiding this comment.
Sorry for the delay! LGTM, made a suggestion about making the excluded task spec for the test a little simpler (hopefully)
| }; | ||
| } | ||
|
|
||
| export function isTaskTypeExcluded(excludedTaskTypes: string[], taskType: string) { |
There was a problem hiding this comment.
excludedTaskTypes should probably be something like excludedTaskTypePatterns or something, to indicate it has a wildcard aspect
| unsafe: { | ||
| exclude_task_types: flatMap(alphabet, (letter) => [ | ||
| `${letter}*`, | ||
| `${letter.toUpperCase()}*`, |
There was a problem hiding this comment.
Since we're using minimatch, can we just use a simple pattern like: [A-Za-z]* ?
|
|
||
| expect(taskRunAtDates.length).toBe(10); | ||
|
|
||
| // run at dates should be within a few seconds of each other |
There was a problem hiding this comment.
We shall see how much fudge we will need to apply! :-)
| const searchedTypes = Array.from(taskTypes) | ||
| .concat(Array.from(removedTypes)) | ||
| .filter((type) => !excludedTypes.has(type)); | ||
| .filter((type) => !isTaskTypeExcluded(excludedTaskTypes, type)); |
There was a problem hiding this comment.
thanks for fixing this bug!
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @ymao1 |
6 participants
Resolves #189111
Summary
Adds jest integration test to test cost capacity based claiming with the
mgetclaim strategy. Using this integration test, we can exclude running other tasks other than our test types. We register a normal cost task and an XL cost task. We test both that we can claim tasks up to 100% capacity and that we will stop claiming tasks if the next task puts us over capacity, even if that means we're leaving capacity on the table.