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

Do not fail whole request on closed index #6790

Conversation

Projects
None yet
5 participants
@spinscale
Copy link
Member

commented Jul 8, 2014

The bulk API request was marked as completely failed,
in case a request with a closed index was referred in
any of the requests inside of a bulk one.

Implementation Note: Currently the implementation is a bit more verbose in order to prevent an instanceof check and another cast - if that is fast enough, we could execute that logic only once at the beginning of the loop (thinking this might be a bit overoptimization here).

Closes #6410

@spinscale spinscale added review and removed bug labels Jul 8, 2014

@clintongormley

This comment has been minimized.

Copy link
Member

commented Jul 9, 2014

@spinscale would this fix also handle the case mentioned in #6410 (comment) when indexing into a non-existent index with action.auto_create_index set to false?

@s1monw

View changes

src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java Outdated
String type = null;
String id = null;
boolean isClosed = false;
if (request instanceof IndexRequest) {

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 9, 2014

Contributor

dude can we extract and interface that we can implement on all of them that allows use to return index, type, id?

@s1monw

View changes

src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java Outdated
deleteRequest.routing(clusterState.metaData().resolveIndexRouting(deleteRequest.routing(), deleteRequest.index()));
deleteRequest.index(clusterState.metaData().concreteSingleIndex(deleteRequest.index()));
} else if (request instanceof UpdateRequest) {
UpdateRequest updateRequest = (UpdateRequest) request;
boolean isClosed = addFailureIfIndexIsClosed(updateRequest.index(), updateRequest.type(), updateRequest.id(), bulkRequest, responses, i);

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 9, 2014

Contributor

can we do:

if (addFailureIfIndexIsClosed(updateRequest.index(), updateRequest.type(), updateRequest.id(), bulkRequest, responses, i)) {
  continue;
}
@s1monw

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2014

added some comments

@s1monw s1monw removed the review label Jul 9, 2014

spinscale added some commits Jul 8, 2014

Bulk API: Do not fail whole request on closed index
The bulk API request was marked as completely failed,
in case a request with a closed index was referred in
any of the requests inside of a bulk one.

Closes #6410
@spinscale

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2014

@clintongormley added another test when action.auto_create_index is set to false
@s1monw added an interface and thus refactored to the code...

*
* Forces this class return index/type/id getters
*/
public interface SingleDocumentWriteRequest {

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 15, 2014

Contributor

maybe we should call it Indexable or DocumentRequest something like this and it should also have a routing or even better the common denominator of update / delete / index ie all the methods they share?

// make sure the request gets never processed again
bulkRequest.requests.set(i, null);
}
} else if (request instanceof DeleteRequest) {

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 15, 2014

Contributor

if we add routing() to the interface we can make this on else if block instead of two?

@@ -96,26 +100,15 @@ protected void doExecute(final BulkRequest bulkRequest, final ActionListener<Bul
if (autoCreateIndex.needToCheck()) {
final Set<String> indices = Sets.newHashSet();
for (ActionRequest request : bulkRequest.requests) {

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 15, 2014

Contributor

honestly, BulkRequest should then only allow you to add Indexable or whatever we call it and then we can skip all the instance of crap here too :) and trash the ElasticsearchException at the bottom

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2014

I added some comments

@s1monw s1monw removed the v1.4.0.Beta1 label Sep 12, 2014

@GaelTadh GaelTadh closed this Sep 19, 2014

@clintongormley clintongormley added the :Bulk label Jun 7, 2015

@clintongormley clintongormley changed the title Bulk API: Do not fail whole request on closed index Do not fail whole request on closed index Jun 7, 2015

@lcawl lcawl added :Distributed/CRUD 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
You can’t perform that action at this time.