-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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 jest integration test to test capacity based claiming #189431
Conversation
|
||
expect(taskRunAtDates.length).toBe(10); | ||
|
||
// run at dates should be within a few seconds of each other |
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.
should be within milliseconds, but I'm adding some fudge factor
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 shall see how much fudge we will need to apply! :-)
taskStore, | ||
size, | ||
taskPartitioner, | ||
}: OwnershipClaimingOpts): Promise<SearchAvailableTasksResponse> { | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're using minimatch, can we just use a simple pattern like: [A-Za-z]*
?
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.
that's much better thanks! updated in 2c1ee89
@pmuellr @js-jankisalvi This is ready for review again! |
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
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.
Sorry for the delay! LGTM, made a suggestion about making the excluded task spec for the test a little simpler (hopefully)
@@ -72,3 +73,13 @@ export function getEmptyClaimOwnershipResult(): ClaimOwnershipResult { | |||
docs: [], | |||
}; | |||
} | |||
|
|||
export function isTaskTypeExcluded(excludedTaskTypes: string[], taskType: string) { |
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.
excludedTaskTypes
should probably be something like excludedTaskTypePatterns
or something, to indicate it has a wildcard aspect
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.
updated in 2c1ee89
unsafe: { | ||
exclude_task_types: flatMap(alphabet, (letter) => [ | ||
`${letter}*`, | ||
`${letter.toUpperCase()}*`, |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shall see how much fudge we will need to apply! :-)
taskStore, | ||
size, | ||
taskPartitioner, | ||
}: OwnershipClaimingOpts): Promise<SearchAvailableTasksResponse> { | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Resolves #189111
Summary
Adds jest integration test to test cost capacity based claiming with the
mget
claim 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.