Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/124936.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 124936
summary: Indicate when errors represent timeouts
area: Infra/REST API
type: enhancement
issues: []
31 changes: 31 additions & 0 deletions server/src/main/java/org/elasticsearch/ElasticsearchException.java
Original file line number Diff line number Diff line change
Expand Up @@ -108,13 +108,16 @@ public class ElasticsearchException extends RuntimeException implements ToXConte

private static final String TYPE = "type";
private static final String REASON = "reason";
private static final String TIMED_OUT = "timed_out";
private static final String CAUSED_BY = "caused_by";
private static final ParseField SUPPRESSED = new ParseField("suppressed");
public static final String STACK_TRACE = "stack_trace";
private static final String HEADER = "header";
private static final String ERROR = "error";
private static final String ROOT_CAUSE = "root_cause";

static final String TIMED_OUT_HEADER = "X-Timed-Out";

private static final Map<Integer, CheckedFunction<StreamInput, ? extends ElasticsearchException, IOException>> ID_TO_SUPPLIER;
private static final Map<Class<? extends ElasticsearchException>, ElasticsearchExceptionHandle> CLASS_TO_ELASTICSEARCH_EXCEPTION_HANDLE;
private final Map<String, List<String>> metadata = new HashMap<>();
Expand All @@ -123,8 +126,12 @@ public class ElasticsearchException extends RuntimeException implements ToXConte
/**
* Construct a <code>ElasticsearchException</code> with the specified cause exception.
*/
@SuppressWarnings("this-escape")
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.

}
}

/**
Expand All @@ -136,8 +143,12 @@ public ElasticsearchException(Throwable cause) {
* @param msg the detail message
* @param args the arguments for the message
*/
@SuppressWarnings("this-escape")
public ElasticsearchException(String msg, Object... args) {
super(LoggerMessageFormat.format(msg, args));
if (isTimeout()) {
headers.put(TIMED_OUT_HEADER, List.of("true"));
}
}

/**
Expand All @@ -151,8 +162,12 @@ public ElasticsearchException(String msg, Object... args) {
* @param cause the nested exception
* @param args the arguments for the message
*/
@SuppressWarnings("this-escape")
public ElasticsearchException(String msg, Throwable cause, Object... args) {
super(LoggerMessageFormat.format(msg, args), cause);
if (isTimeout()) {
headers.put(TIMED_OUT_HEADER, List.of("true"));
}
}

@SuppressWarnings("this-escape")
Expand Down Expand Up @@ -253,6 +268,13 @@ public RestStatus status() {
}
}

/**
* Returns whether this exception represents a timeout.
*/
public boolean isTimeout() {
return false;
}

/**
* Unwraps the actual cause from the exception for cases when the exception is a
* {@link ElasticsearchWrapperException}.
Expand Down Expand Up @@ -386,6 +408,15 @@ protected static void innerToXContent(
builder.field(TYPE, type);
builder.field(REASON, message);

boolean timedOut = false;
if (throwable instanceof ElasticsearchException exception) {
// TODO: we could walk the exception chain to see if _any_ causes are timeouts?
timedOut = exception.isTimeout();
}
if (timedOut) {
builder.field(TIMED_OUT, timedOut);
}

for (Map.Entry<String, List<String>> entry : metadata.entrySet()) {
headerToXContent(builder, entry.getKey().substring("es.".length()), entry.getValue());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,9 @@ public RestStatus status() {
// closest thing to "your request took longer than you asked for"
return RestStatus.TOO_MANY_REQUESTS;
}

@Override
public boolean isTimeout() {
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,9 @@ public ProcessClusterEventTimeoutException(StreamInput in) throws IOException {
public RestStatus status() {
return RestStatus.TOO_MANY_REQUESTS;
}

@Override
public boolean isTimeout() {
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

/**
* Specific instance of {@link SearchException} that indicates that a search timeout occurred.
* Always returns http status 504 (Gateway Timeout)
* Always returns http status 429 (Too Many Requests)
*/
public class SearchTimeoutException extends SearchException {
public SearchTimeoutException(SearchShardTarget shardTarget, String msg) {
Expand All @@ -34,6 +34,11 @@ public RestStatus status() {
return RestStatus.TOO_MANY_REQUESTS;
}

@Override
public boolean isTimeout() {
return true;
}

/**
* Propagate a timeout according to whether partial search results are allowed or not.
* In case partial results are allowed, a flag will be set to the provided {@link QuerySearchResult} to indicate that there was a
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1552,4 +1552,35 @@ private void testExceptionLoop(Exception rootException) throws IOException {
assertThat(ser.getMessage(), equalTo(rootException.getMessage()));
assertArrayEquals(ser.getStackTrace(), rootException.getStackTrace());
}

static class ExceptionSubclass extends ElasticsearchException {
@Override
public boolean isTimeout() {
return true;
}

ExceptionSubclass(String message) {
super(message);
}
}

public void testTimeout() throws IOException {
var e = new ExceptionSubclass("some timeout");
assertThat(e.getHeaderKeys(), hasItem(ElasticsearchException.TIMED_OUT_HEADER));

XContentBuilder builder = XContentFactory.jsonBuilder();
builder.startObject();
e.toXContent(builder, ToXContent.EMPTY_PARAMS);
builder.endObject();
String expected = """
{
"type": "exception_subclass",
"reason": "some timeout",
"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 :)

}""";
assertEquals(XContentHelper.stripWhitespace(expected), Strings.toString(builder));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,8 @@ public void testExceptionRegistration() throws IOException, URISyntaxException {
final Set<? extends Class<?>> ignore = Sets.newHashSet(
CancellableThreadsTests.CustomException.class,
RestResponseTests.WithHeadersException.class,
AbstractClientHeadersTestCase.InternalException.class
AbstractClientHeadersTestCase.InternalException.class,
ElasticsearchExceptionTests.ExceptionSubclass.class
);
FileVisitor<Path> visitor = new FileVisitor<Path>() {
private Path pkgPrefix = PathUtils.get(path).getParent();
Expand Down