Skip to content
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

Merged
merged 7 commits into from
Mar 17, 2025
Merged

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented Mar 15, 2025

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.
@rjernst rjernst added >enhancement :Core/Infra/REST API REST infrastructure and utilities auto-backport Automatically create backport pull requests when merged v8.18.1 v8.19.0 v9.0.1 v9.1.0 labels Mar 15, 2025
@rjernst rjernst requested a review from DaveCTurner March 15, 2025 01:28
@elasticsearchmachine
Copy link
Collaborator

Hi @rjernst, I've created a changelog YAML for you.

@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Mar 15, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@rjernst
Copy link
Member Author

rjernst commented Mar 15, 2025

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.

@rjernst
Copy link
Member Author

rjernst commented Mar 15, 2025

@elasticmachine update branch

Copy link
Contributor

@DaveCTurner DaveCTurner left a 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.

Comment on lines 1579 to 1582
"timed_out": true,
"header": {
"X-Timed-Out": "true"
}
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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"));
Copy link
Contributor

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.

@rjernst rjernst enabled auto-merge (squash) March 17, 2025 20:08
@rjernst rjernst merged commit 3e3c8a2 into elastic:main Mar 17, 2025
17 checks passed
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Mar 17, 2025

💔 Backport failed

Status Branch Result
9.0

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 124936

rjernst added a commit to rjernst/elasticsearch that referenced this pull request Mar 17, 2025
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.
@rjernst rjernst deleted the timed_out_response branch March 18, 2025 01:00
elasticsearchmachine pushed a commit that referenced this pull request Mar 18, 2025
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.
afoucret pushed a commit to afoucret/elasticsearch that referenced this pull request Mar 18, 2025
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged backport pending :Core/Infra/REST API REST infrastructure and utilities >enhancement Team:Core/Infra Meta label for core/infra team v9.0.1 v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants