-
Notifications
You must be signed in to change notification settings - Fork 124
Review handling of status code in HTTP requests #1549
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
Review handling of status code in HTTP requests #1549
Conversation
1f5c8d9
to
2dc182d
Compare
// No docs yet. | ||
return &hits{}, nil | ||
} | ||
if resp.StatusCode == http.StatusServiceUnavailable && strings.Contains(resp.String(), "no_shard_available_exception") { |
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.
FTR, this is the error I saw that made me to handle this case:
Error: error running package system tests: could not complete test run: failed to search docs for data stream metrics-apache.status-ep: [503 Service Unavailable] {"error":{"root_cause":[{"type":"no_shard_available_action_exception","reason":null}],"type":"search_phase_execution_exception","reason":"all shards failed","phase":"query","grouped":true,"failed_shards":[{"shard":0,"index":".ds-metrics-apache.status-ep-2023.11.09-000001","node":null,"reason":{"type":"no_shard_available_action_exception","reason":null}}]},"status":503}
bf6b99a
to
f2246ac
Compare
/test |
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
Should we run a test integrations
in this PR before merging ?
test integrations |
Created or updated PR in integrations repository to test this version. Check elastic/integrations#8460 |
test integrations |
Created or updated PR in integrations repository to test this version. Check elastic/integrations#8460 |
test integrations |
Created or updated PR in integrations repository to test this version. Check elastic/integrations#8460 |
💚 Build Succeeded
History
cc @jsoriano |
I had to do some fixes after testing with integrations, it seems to be fine now @mrodm could you re-review? Thanks! |
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.
🚀
In a number of places we were not handling HTTP status codes, what may hide issues
or complicate debugging.
Also ensure that body is closed in all cases.
Fixes #1486