Introduce no_items_on_success option to the bulk API #55088

Mpdreamz · 2020-04-10T20:37:42Z

This allows the _bulk API to return an empty array for items if so explicitly requested by the user.

This also introduces an extra items_omitted property to broadcast the act of omitting the items from the response very loudly.

POST /_bulk?no_items_on_success
{ "index": { "_index" : "a" } } 
{}
{ "index": { "_index" : "a" } } 
{}
{ "index": { "_index" : "a" } }
{}

{
    "took": 530,
    "errors": false,
    "items_omitted": true,
    "items": []
}

If no_items_on_success is set but the bulk has failures items will always return all the items and items_omitted will be false.

{ "index": { "_index" : "a" } } 
{}
{ "index": { "_index" : "a" } } 
{ "i": 2}
{ "index": { "_index" : "a" } }
{ "i": "string" }

{
    "took": 56,
    "errors": true,
    "items_omitted": false,
    "items": [
		...
        {
            "index": {
                "_index": "a",
                "_id": "64vOZXEBvXhNefbuM4g2",
                "status": 400,
                "error": {
                   ...
                }
            }
        }
    ]
}

elasticmachine · 2020-04-10T21:01:05Z

Pinging @elastic/es-distributed (:Distributed/CRUD)

martijnvg · 2020-04-14T14:02:42Z

server/src/main/java/org/elasticsearch/action/bulk/BulkResponse.java

        builder.startArray(ITEMS);
-        for (BulkItemResponse item : this) {
-            item.toXContent(builder, params);
+        if (noFailuresAnNoItemsRequested == false) {


Would be great if the decision on whether to include the items can be made earlier.
Maybe in TransportBulkAction in the finishHim() method around line 541?
This would then save creating bulk item response array.

Would be great if we could make this decision even earlier, so that no shard level response items
are collected. Then in the case that there are failures then we could serialize the successful items
as null elements? But then I'm afraid that we break the bulk response format.

Yeah I could do the first, that would definitely help 👍

Not sure the second warrants the additional complexity? The goal of this PR is to shave off serialization time on the consumer. Perhaps something to consider once this lands?

@martijnvg revisiting this I think it's best not to introduce the complexity in the transport layer and only expose this on the REST layer.

It would introduce a set of new problems

shards would need to send back how many items they would have returned

reducing all shard information becomes more complex to expand the empty shard responses

We need to find a new representation for successful bulk operations in case of failures.

The PR as it stands does not preclude taking this on as follow up work but is useful as is already.

Keen to hear your thoughts!

@Mpdreamz I agree, this PR is useful in its current form.

However I think it makes sense to not include the items in the bulk response if there are no failures and no_items_on_success is set. This can simply be done by iterating over the response items and see if there is an error and then have an additional constructor of bulk responses which accepts no response items and noFailuresAnNoItemsRequested (or sets that always to true).

server/src/main/java/org/elasticsearch/rest/action/document/RestBulkAction.java

martijnvg · 2020-12-08T12:44:52Z

server/src/main/java/org/elasticsearch/action/bulk/BulkResponse.java

@@ -153,9 +167,14 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
            builder.field(INGEST_TOOK, ingestTookInMillis);
        }
        builder.field(ERRORS, hasFailures());
+        boolean noItems = noItemsOnSuccess != null && noItemsOnSuccess;
+        boolean noFailuresAndNoItemsRequested = noItems && this.hasFailures() == false;
+        builder.field(ITEMS_OMITTED, noFailuresAndNoItemsRequested);


maybe we can return items with null value instead of returning the items_omitted field?
It looks like we currently never return null, so we could use that.

The third state for items_omitted adds more complexity to the users of the API IMO. Knowing the property is always there makes checking for it all the easier.

This allows the bulk to return an empty array for `items` if so explicitly instructed by the user. This also introduces an extra `items_omitted` property to broadcast the act of omitting the items from the response very loudly.

…if its never going to be serialized

martijnvg

I left a few more comments.

martijnvg · 2020-12-14T14:47:34Z

docs/reference/docs/bulk.asciidoc

@@ -4,7 +4,7 @@
 <titleabbrev>Bulk</titleabbrev>
 ++++

-Performs multiple indexing or delete operations in a single API call. 
+Performs multiple indexing or delete operations in a single API call.


maybe undo these whitespace changes?

martijnvg · 2020-12-14T14:52:17Z

server/src/main/java/org/elasticsearch/action/bulk/BulkResponse.java


    public static final long NO_INGEST_TOOK = -1L;

    private final BulkItemResponse[] responses;
    private final long tookInMillis;
    private final long ingestTookInMillis;
+    @Nullable
+    private final Boolean noItemsOnSuccess;


can this be a primitive boolean? I think we default to false.

martijnvg · 2020-12-14T15:10:28Z

server/src/main/java/org/elasticsearch/action/bulk/BulkResponse.java

+
+    public BulkResponse(BulkItemResponse[] responses, long tookInMillis, long ingestTookInMillis, Boolean noItemsOnSuccess) {
+        final boolean hasError = responses != null && Arrays.stream(responses).anyMatch(r-> r != null && r.isFailed());
+        this.responses = noItemsOnSuccess != null && noItemsOnSuccess && hasError == false


maybe add a comment that the bulk items are omitted because these are no longer needed if noItemsOnSuccess is true, which would allow the gc to clean the items up before the response has been serialized over the network.

martijnvg · 2020-12-14T15:21:22Z

server/src/main/java/org/elasticsearch/action/bulk/BulkResponse.java

@@ -49,28 +50,42 @@
    private static final String ERRORS = "errors";
    private static final String TOOK = "took";
    private static final String INGEST_TOOK = "ingest_took";
+    private static final String ITEMS_OMITTED = "items_omitted";


maybe rename to no_items? Then it is more inline with the field name noItemsOnSuccess ?

Mpdreamz requested review from dliappis and removed request for dliappis April 10, 2020 20:39

jtibshirani added :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. >enhancement labels Apr 10, 2020

martijnvg reviewed Apr 14, 2020

View reviewed changes

Mpdreamz force-pushed the feature/master/bulk-empty-items-on-success branch from 2b7ab7d to c9f3cb0 Compare May 4, 2020 07:57

rjernst added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label May 4, 2020

Mpdreamz mentioned this pull request Jul 30, 2020

Consider using 207 Multi-Status for responses with varying operation results #60442

Open

Mpdreamz force-pushed the feature/master/bulk-empty-items-on-success branch from a4a4c7d to 99654a5 Compare August 5, 2020 14:49

Mpdreamz force-pushed the feature/master/bulk-empty-items-on-success branch from 99654a5 to 2d393f4 Compare December 7, 2020 12:26

martijnvg reviewed Dec 8, 2020

View reviewed changes

server/src/main/java/org/elasticsearch/rest/action/document/RestBulkAction.java Show resolved Hide resolved

martijnvg reviewed Dec 8, 2020

View reviewed changes

Mpdreamz added 17 commits December 10, 2020 11:28

Introduce no_items_on_success option to bulk

cec8bb1

This allows the bulk to return an empty array for `items` if so explicitly instructed by the user. This also introduces an extra `items_omitted` property to broadcast the act of omitting the items from the response very loudly.

fix checkstyle issues

42e4b54

add skip version to rest spec tests for no_items_on_success

788b2cc

update bulk docs

1cad0ae

skip is not a property of do

8e1cf0a

typo

019ee47

update bulk.asciidoc

5653c97

update bulk duplicate id in docs

664abb2

missing comma in docs example

934670b

remove empty constructor

49fc00e

bulkRequest is cleaned up before handler is executed after rebasing

5a557f7

Make sure we do not sent the full responseitem array to bulkresponse …

5066cdf

…if its never going to be serialized

move response filtering to constructor

8adcce4

check for null on Boolean

b185d21

fix unused imports

47ac505

fix unused imports

b897011

guard constructor against null items in responses

e623aa6

Mpdreamz force-pushed the feature/master/bulk-empty-items-on-success branch from af86291 to e623aa6 Compare December 10, 2020 10:29

Mpdreamz requested a review from martijnvg December 14, 2020 11:27

martijnvg reviewed Dec 14, 2020

View reviewed changes

Mpdreamz mentioned this pull request Aug 10, 2021

Add Warning Http Header when one ore more documents fails in bulk #76216

Open

elasticsearchmachine changed the base branch from master to main July 22, 2022 23:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce no_items_on_success option to the bulk API #55088

Introduce no_items_on_success option to the bulk API #55088

Mpdreamz commented Apr 10, 2020

elasticmachine commented Apr 10, 2020

martijnvg Apr 14, 2020

Mpdreamz Apr 15, 2020

Mpdreamz May 4, 2020

martijnvg Dec 8, 2020

martijnvg Dec 8, 2020

Mpdreamz Dec 8, 2020

martijnvg left a comment

martijnvg Dec 14, 2020

martijnvg Dec 14, 2020

martijnvg Dec 14, 2020

martijnvg Dec 14, 2020

Introduce no_items_on_success option to the bulk API #55088

Are you sure you want to change the base?

Introduce no_items_on_success option to the bulk API #55088

Conversation

Mpdreamz commented Apr 10, 2020

elasticmachine commented Apr 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment