-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
Indicate when errors represent timeouts #124936
Conversation
This commit adds a `timed_out` key to the error responses that represent a timeout condition. It also adds an `X-Timed-Out` header to the response indicating the same outside the response body.
Hi @rjernst, I've created a changelog YAML for you. |
Pinging @elastic/es-core-infra (Team:Core/Infra) |
I'm not particularly happy with this approach, but I think it's relatively small and achieves the goal: getting a timed out indicator in the response body and headers. I would have liked to not use subclassing a method to indicate the type is an exception, instead using the ElasticsearchTimeoutException as a base for all timeouts, but that doesn't work with SearchTimeoutException. |
@elasticmachine update branch |
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, couple of (kinda mutually-exclusive) nits but this can go as-is too.
"timed_out": true, | ||
"header": { | ||
"X-Timed-Out": "true" | ||
} |
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.
If we always render headers, do we need a separate timed_out
field?
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.
I was surprised to learn we have this header map. It's kind of odd we have it in the first place. Accessing it in this case would mean looking for error.header."X-Timed-Out"
and then the weird ?1
. IMO a json-specific field is nicer.
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.
Yep that's why I felt my two comments were mutually exclusive :)
public ElasticsearchException(Throwable cause) { | ||
super(cause); | ||
if (isTimeout()) { | ||
headers.put(TIMED_OUT_HEADER, List.of("true")); |
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.
RFC 8941 §4.1.9 specifies the string ?1
for a true boolean value in a HTTP header field.
💔 Backport failed
You can use sqren/backport to manually backport by running |
This commit adds a `timed_out` key to the error responses that represent a timeout condition. It also adds an `X-Timed-Out` header to the response indicating the same outside the response body.
This commit adds a `timed_out` key to the error responses that represent a timeout condition. It also adds an `X-Timed-Out` header to the response indicating the same outside the response body.
This commit adds a
timed_out
key to the error responses that represent a timeout condition. It also adds anX-Timed-Out
header to the response indicating the same outside the response body.