-
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] Propagate msearch
error status code so backpressure mechanism responds correctly
#197501
base: main
Are you sure you want to change the base?
Conversation
msearch
error status code so backpressure mechanism responds correctly
2687a3f
to
3b0d716
Compare
Pinging @elastic/response-ops (Team:ResponseOps) |
@elasticmachine merge upstream |
💚 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.
Changes LGTM! Left a few nits but testing locally worked as expected!
); | ||
|
||
try { | ||
await store.msearch([{}]); |
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.
optional nit: not sure if we need this to ensure the catch
path goes through.
await store.msearch([{}]); | |
await store.msearch([{}]); | |
throw new Error('should have thrown'); |
@@ -574,8 +575,11 @@ export class TaskStore { | |||
let allTasks = new Array<ConcreteTaskInstance>(); | |||
|
|||
for (const response of responses) { | |||
if (response.status !== 200) { | |||
const err = new Error(`Unexpected status code from taskStore::msearch: ${response.status}`); | |||
if (response.status && response.status !== 200) { |
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.
nit: It's interesting that response.status
could be null. If ever that occurs, should we consider it an unexpected status code as well?
I added the |
Resolves https://github.com/elastic/response-ops-team/issues/240
Summary
Creating an
MsearchError
class that preserves the status code from any msearch errors. These errors are already piped to the managed configuration observable that watches for and responds to ES errors from the update by query claim strategy so I updated that filter to filter for msearch 429 and 503 errors as well.To Verify
xpack.task_manager.claim_strategy: 'mget'
) and start ES and Kibana.