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

Make search failure cause rest failure #16889

Merged
merged 1 commit into from
Mar 10, 2016

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Mar 1, 2016

Indexing failures have caused the reindex http request to fail for a while
now. Both search and indexing failures cause it to abort. But search
failures didn't cause a non-200 response code from the http api. This
fixes that.

Also slips in a fix to some infrequently failing rest tests.

Closes #16037

@clintongormley
Copy link

@nik9000 while you're on this, I think this issue would be of interest to you too: #16555 (comment)

@@ -41,6 +43,12 @@ protected RestStatus getStatus(R response) {
status = failure.getStatus();
}
}
for (ShardSearchFailure failure: response.getSearchFailures()) {
RestStatus failureStatus = ExceptionsHelper.status(failure.getCause());
if (failureStatus.getStatus() > status.getStatus()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of a weird way to order failures, but I guess it works. I think adding a comment might help others in the future in understanding your thinking

@dakrone
Copy link
Member

dakrone commented Mar 10, 2016

LGTM left one minor comment

Indexing failures have caused the reindex http request to fail for a while
now. Both search and indexing failures cause it to abort. But search
failures didn't cause a non-200 response code from the http api. This
fixes that.

Also slips in a fix to some infrequently failing rest tests.

Closes elastic#16037
@nik9000 nik9000 force-pushed the reindex_status_from_search_failrues branch from 57dc548 to b2eec96 Compare March 10, 2016 18:47
@nik9000 nik9000 merged commit b2eec96 into elastic:master Mar 10, 2016
@nik9000
Copy link
Member Author

nik9000 commented Mar 14, 2016

Backported to 2.x with 7b1d567

@lcawl lcawl added :Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. and removed :Reindex API labels Feb 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. v2.3.0 v5.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reindex should copy status from any search failures
4 participants