-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Propagate index errors in field_caps #70245
Conversation
8e0e659
to
9995863
Compare
This is an example of the response with failures with the changes in this PR:
|
Pinging @elastic/es-search (Team:Search) |
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.
The approach looks good to me. I left one comment regarding the reporting of the exception.
server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesIndexResponse.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/fieldcaps/TransportFieldCapabilitiesAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/fieldcaps/TransportFieldCapabilitiesAction.java
Outdated
Show resolved
Hide resolved
@jimczi thanks for the review, I will rework the part around how to transport back the exceptions. |
@jimczi I pushed and update reworking how the exceptions get transported back. |
countDown.run(); | ||
}, e -> { | ||
// individual index failures should show up as remote responses containing an exception | ||
countDown.run(); |
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.
This case is not handled automatically. We need to add all the indices from that cluster in the index failures map. Since all indices on that cluster failed, maybe that the key in the map could be the cluster name instead ?
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.
Individual index failures on a remote should be handled (there is also a test for it now that should fail otherwise). So you mean this should cover general remote failures? How could this happen? Also asking since I cannot thing around a way of testing this atm...
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.
Looks good to me. Could you add a test a yml test for ccs as well ?
@Override | ||
public RestStatus status() { | ||
RestStatus status = RestStatus.OK; | ||
if (indices.length == 0 && failures.size() > 0) { |
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.
Do we need this logic now that we throw an exception if all indices fail ? We could even assert in the ctr that failures must be empty if indices is empty.
@jimczi thanks, I pushed changes that remove the rest status logic you mentioned, reworked the failure collection a bit to be all on a central place and added the test you asked for. |
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, thanks @cbuescher !
Currently we don't report any exceptions occuring during field_caps requests back to the user. This PR adds a new failure section to the response which contains exceptions per index. In addition the response contains another field, `failed_indices`, with the number of indices that threw an exception. If all of the requested indices fail, the whole request fails, otherwise the request succeeds and it is up to the caller to check for potential errors in the response body. Closes elastic#68994
Currently we don't report any exceptions occuring during field_caps requests back to the user. This PR adds a new failure section to the response which contains exceptions per index. In addition the response contains another field, `failed_indices`, with the number of indices that threw an exception. If all of the requested indices fail, the whole request fails, otherwise the request succeeds and it is up to the caller to check for potential errors in the response body. Closes #68994
Currently we don't report any exceptions occuring during
field_caps
requestrs back to the user.This PR adds a new
failure
section to the response which contains exceptions per index, keyed bythe index name the error occured in. In addition the response contains another field
failed_indices
which reports the number of indices that threw an exception. If all of the requested indices failed,
the whole request fails with a 500 error code, otherwise the request succeeds and it is up to the caller
to check for potential errors in the response body.
Closes #68994