Skip to content

Commit

Permalink
Remove unhelpful BulkResponse#status() method (elastic#99865)
Browse files Browse the repository at this point in the history
BulkResponse#status() always returns 200 OK, and consumers wishing to check
if there were any errors during the bulk update need to consult the hasFailures()
method. This is confusing and error-prone, as can be seen in a couple of tests that
are checking status() to know whether or not to continue.

This commit removes status() entirely from BulkResponse.

Related to elastic#99824
  • Loading branch information
romseygeek committed Sep 25, 2023
1 parent db10810 commit 1a48c59
Show file tree
Hide file tree
Showing 5 changed files with 7 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,8 @@
import org.elasticsearch.common.collect.Iterators;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.StatusToXContentObject;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.xcontent.ToXContentObject;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.XContentParser;

Expand All @@ -32,7 +31,7 @@
* bulk requests. Each item holds the index/type/id is operated on, and if it failed or not (with the
* failure message).
*/
public class BulkResponse extends ActionResponse implements Iterable<BulkItemResponse>, StatusToXContentObject {
public class BulkResponse extends ActionResponse implements Iterable<BulkItemResponse>, ToXContentObject {

private static final String ITEMS = "items";
private static final String ERRORS = "errors";
Expand Down Expand Up @@ -134,11 +133,6 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeZLong(ingestTookInMillis);
}

@Override
public RestStatus status() {
return RestStatus.OK;
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import org.elasticsearch.rest.RestRequest;
import org.elasticsearch.rest.Scope;
import org.elasticsearch.rest.ServerlessScope;
import org.elasticsearch.rest.action.RestStatusToXContentListener;
import org.elasticsearch.rest.action.RestToXContentListener;
import org.elasticsearch.search.fetch.subphase.FetchSourceContext;

import java.io.IOException;
Expand Down Expand Up @@ -93,7 +93,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
request.getRestApiVersion()
);

return channel -> client.bulk(bulkRequest, new RestStatusToXContentListener<>(channel));
return channel -> client.bulk(bulkRequest, new RestToXContentListener<>(channel));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.elasticsearch.index.query.SearchExecutionContext;
import org.elasticsearch.index.shard.IndexShard;
import org.elasticsearch.index.shard.ShardId;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.search.DocValueFormat;
import org.elasticsearch.search.aggregations.AggregationExecutionContext;
import org.elasticsearch.search.aggregations.BucketCollector;
Expand Down Expand Up @@ -280,7 +281,7 @@ public void afterBulk(long executionId, BulkRequest request, BulkResponse respon
bulkIngestTookMillis,
bulkTookMillis,
response.hasFailures(),
response.status().getStatus()
RestStatus.OK.getStatus()
)
);
task.updateBulkInfo(bulkIngestTookMillis, bulkTookMillis);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import org.elasticsearch.common.Strings;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.index.query.QueryBuilders;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.search.SearchHit;
import org.elasticsearch.xcontent.ToXContent;
import org.elasticsearch.xcontent.XContentBuilder;
Expand Down Expand Up @@ -290,7 +289,7 @@ private void testExpiredDeletion(Float customThrottle, int numUnusedState) throw
}

// Before we call the delete-expired-data action we need to make sure the unused state docs were indexed
assertThat(indexUnusedStateDocsResponse.get().status(), equalTo(RestStatus.OK));
assertFalse(indexUnusedStateDocsResponse.get().hasFailures());

// Now call the action under test
assertThat(deleteExpiredData(customThrottle).isDeleted(), is(true));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,6 @@ public void testConcurrentPrewarming() throws Exception {
bulkRequest.add(client().prepareIndex(indexName).setSource("foo", randomBoolean() ? "bar" : "baz"));
}
final BulkResponse bulkResponse = bulkRequest.get();
assertThat(bulkResponse.status(), is(RestStatus.OK));
assertThat(bulkResponse.hasFailures(), is(false));
}
docsPerIndex.put(indexName, nbDocs);
Expand Down

0 comments on commit 1a48c59

Please sign in to comment.