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

Handle failed request when auto create index is disabled #8163

Closed
wants to merge 1 commit into from
Closed

Handle failed request when auto create index is disabled #8163

wants to merge 1 commit into from

Conversation

markharwood
Copy link
Contributor

If a bulk request contains a mix of indexing requests for an existing index and one that needs to be auto-created but a cluster configuration prevents the auto-create of the new index, the ingest process hangs. The exception for the failure to create an index was not caught or reported back properly. Added a Junit test to recreate the issue and the associated fix is in TransportBulkAction.

Closes #8125

@bleskes
Copy link
Contributor

bleskes commented Oct 20, 2014

I think adding an explicit handling of IndexMissingException which will not cause the entire request to fail. However, the source of the problem is deeper and lies in the fact that we don't deal with failure correctly in the executeBulk method while returning from the create index command. IMHO this is where the proper fix belongs.

@bleskes bleskes removed the review label Oct 20, 2014
@markharwood
Copy link
Contributor Author

@bleskes can you review the last change please? thx

@@ -354,6 +359,16 @@ private boolean addFailureIfIndexIsClosed(DocumentRequest request, BulkRequest b
concreteIndex = concreteIndices.resolveIfAbsent(request.index(), request.indicesOptions());
} catch (IndexClosedException ice) {
isClosed = true;
} catch (IndexMissingException ime) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we simplify this code by merging the two use cases. The only difference is the exception that goes into the BulkItemResponse.Failure object. Can we have that as a variable instead of isClosed , i.e., Exception unavailableException = null and later check:

 if (unavailableException != null) {
             BulkItemResponse.Failure failure = new BulkItemResponse.Failure(request.index(), request.type(), request.id(), unavailableException);
             BulkItemResponse bulkItemResponse = new BulkItemResponse(idx, "index", failure);
             responses.set(idx, bulkItemResponse);
             // make sure the request gets never processed again
             bulkRequest.requests.set(idx, null);
         }

@bleskes
Copy link
Contributor

bleskes commented Oct 21, 2014

@markharwood I left one more comment, once that's done I think it's good.

@bleskes bleskes removed the review label Oct 21, 2014
If a bulk request contains a mix of indexing requests for an existing index and one that needs to be auto-created but a cluster configuration prevents the auto-create of the new index the ingest process hangs. The exception for the failure to create an index was not caught or reported back properly. Added a Junit test to recreate the issue and the associated fix is in TransportBulkAction.

Closes #8125
@markharwood
Copy link
Contributor Author

Rebased and consolidated exception handling

@bleskes
Copy link
Contributor

bleskes commented Oct 23, 2014

LGTM. Thx @markharwood

@markharwood
Copy link
Contributor Author

@bleskes Pushed to master, 1.x and 1.4 but TransportBulkAction in 1.3 looks different and is missing addFailureIfIndexIsClosed() method.
Are we back-porting this fix and the related checks for closed indexes into 1.3?

@bleskes
Copy link
Contributor

bleskes commented Oct 27, 2014

@markharwood thx. I think this an important bug fix. It can go into 1.3 as well.

@markharwood
Copy link
Contributor Author

@bleskes @GaelTadh 1.3 is missing 61c21f9a which addresses #6410
My patch here extends the above's logic for testing for closed indices - any reason that didn't go into 1.3, Brian?

@clintongormley
Copy link

@markharwood @GaelTadh I think #6410 should go into 1.3 as well

@GaelTadh
Copy link
Contributor

Looks like both 61c21f9 and this depend on 5d987ad.

@GaelTadh
Copy link
Contributor

5d987ad addresses #7223, this is a fairly big change that is only in 1.4B1 and up.

@clintongormley
Copy link

OK - then let's merge this only into 1.4. thanks @GaelTadh

@s1monw
Copy link
Contributor

s1monw commented Oct 27, 2014

from the history here this seems to be committed to 1.4 and up, can we close it?

@markharwood
Copy link
Contributor Author

Committed to 1.4 and up, see d12ae19

@clintongormley clintongormley changed the title Bulk indexing: Fix 8125 hanged request when auto create index is off. Bulk API: Handle failed request when auto create index is disabled Nov 3, 2014
@clintongormley clintongormley changed the title Bulk API: Handle failed request when auto create index is disabled Handle failed request when auto create index is disabled Jun 7, 2015
@lcawl lcawl added :Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. and removed :Bulk 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. v1.4.0 v1.5.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bulk request hangs when one index can be auto created an another cannot
6 participants