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

Add BulkRequest support to High Level Rest client #23312

Merged
merged 4 commits into from Feb 23, 2017

Conversation

Projects
None yet
2 participants
@tlrx
Copy link
Member

commented Feb 22, 2017

This commit adds support for BulkRequest execution in the High Level Rest client.

@javanna
Copy link
Member

left a comment

left a few comments

client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java Outdated
parameters.withRefreshPolicy(bulkRequest.getRefreshPolicy());

// Bulk API only supports newline delimited JSON
XContentType xContentType = XContentType.JSON;

This comment has been minimized.

Copy link
@javanna

javanna Feb 22, 2017

Member

it also supports smile. should we validate that the format of all the index/update requests is the same and use that one? I wouldn't want to make things too complicated though given that most likely nobody uses smile here. People may also theoretically be using cbor or yaml in the single index requests now using the index api, as they don't have to go through REST.

if (opType == DocWriteRequest.OpType.INDEX || opType == DocWriteRequest.OpType.CREATE) {
IndexRequest indexRequest = (IndexRequest) request;
BytesReference indexSource = indexRequest.source();
XContentType indexXContentType = indexRequest.getContentType();

This comment has been minimized.

Copy link
@javanna

javanna Feb 22, 2017

Member

interesting idea, to convert all the index requests to the target format, that way we can choose to always use either json or smile, without validating anything. Maybe that's better than what I suggested above. Although maybe tricky cause if a user sends cbor or smile, maybe they really expect cbor or smile to be stored in lucene rather than the json obtained from the conversion, but that is not possible when going through REST. I am on the fence. Maybe we should rather do some validation before we run the request, make sure that all of the requests have the same content-type and that can only be either json or smile. That way if something is not expected users are notified with an error. Thoughts?

This comment has been minimized.

Copy link
@tlrx

tlrx Feb 23, 2017

Author Member

I always forgot about Smile... I think it makes sense to check that all requests have the same content type and that it's either JSON or Smile. I updated the code and added tests for that, the first content type found dictates what's expected for the other requests.

assertEquals(errors[i] ? RestStatus.INTERNAL_SERVER_ERROR : RestStatus.CREATED, bulkItemResponse.status());
} else if (requestOpType == DocWriteRequest.OpType.UPDATE) {
assertEquals(errors[i], bulkItemResponse.isFailed());
assertEquals(errors[i] ? RestStatus.INTERNAL_SERVER_ERROR : RestStatus.OK, bulkItemResponse.status());

This comment has been minimized.

Copy link
@javanna

javanna Feb 22, 2017

Member

are these internal server errors ok? this seems fishy.

This comment has been minimized.

Copy link
@tlrx

tlrx Feb 23, 2017

Author Member

They are ElasticsearchException and ElasticsearchException almost always return INTERNAL_SERVER_ERROR as the status.

They come from shard info failures for the bulk item request, but the type is lost because the bulk response (and associated bulk item responses) are parsed back by the high level rest client.

I checked the status, I'm not sure that the test should check more than that.

This comment has been minimized.

Copy link
@javanna

javanna Feb 23, 2017

Member

ok let's try and fix this separately.

@tlrx

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2017

@javanna Round 2, ready

@javanna
Copy link
Member

left a comment

thanks @tlrx left a couple more comments. it's close though.

client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java Outdated
if (bulkContentType == null) {
bulkContentType = requestContentType;
} else {
match = (requestContentType == bulkContentType);

This comment has been minimized.

Copy link
@javanna

javanna Feb 23, 2017

Member

I wish we could throw exception here already rather than setting a flag. maybe add a static method that throws the exception so we don't worry about copying the message around?

client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java Outdated
// Bulk API only supports newline delimited JSON or Smile. Before executing
// the bulk, we need to check that all requests have the same content-type
// and this content-type is supported by the Bulk API.
List<XContentType> allowedContentTypes = Arrays.asList(XContentType.JSON, XContentType.SMILE);

This comment has been minimized.

Copy link
@javanna

javanna Feb 23, 2017

Member

add a short method that checks the content type and get rid of the list? If this class becomes too big I am good with getting the bulk part out. some of this methods are going to be useful for msearch too at some point.

client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java Outdated
bulkContentType = requestContentType;
} else {
match = (requestContentType == bulkContentType);
}

This comment has been minimized.

Copy link
@javanna

javanna Feb 23, 2017

Member

this code block is repeated and always deals with index requests I think. We can probably factor it out to a method.

@tlrx

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2017

@javanna Thanks again. Would you like to have another look, please?

@javanna
Copy link
Member

left a comment

left a few minors, LGTM otherwise

client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java Outdated
return requestContentType;
}
if (requestContentType != xContentType) {
throw new IllegalStateException("Mismatching content-type found for request with content-type [" + requestContentType

This comment has been minimized.

Copy link
@javanna

javanna Feb 23, 2017

Member

maybe we should throw illegal argument rather than illegal state.


/**
* Ensure that the {@link IndexRequest}'s content type is supported by the Bulk API and that it conforms
* to the current {@link BulkRequest}'s content type (if it's known at the time of this method get called).

This comment has been minimized.

Copy link
@javanna

javanna Feb 23, 2017

Member

mention the return type in the docs?

client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java Outdated
* Ensure that the {@link IndexRequest}'s content type is supported by the Bulk API and that it conforms
* to the current {@link BulkRequest}'s content type (if it's known at the time of this method get called).
*/
static XContentType ensureBulkContentType(IndexRequest indexRequest, @Nullable XContentType xContentType) {

This comment has been minimized.

Copy link
@javanna

javanna Feb 23, 2017

Member

enforceSameContentType or enforceContentType ?

assertEquals(errors[i] ? RestStatus.INTERNAL_SERVER_ERROR : RestStatus.CREATED, bulkItemResponse.status());
} else if (requestOpType == DocWriteRequest.OpType.UPDATE) {
assertEquals(errors[i], bulkItemResponse.isFailed());
assertEquals(errors[i] ? RestStatus.INTERNAL_SERVER_ERROR : RestStatus.OK, bulkItemResponse.status());

This comment has been minimized.

Copy link
@javanna

javanna Feb 23, 2017

Member

ok let's try and fix this separately.

tlrx added some commits Feb 22, 2017

Add BulkRequest support to High Level Rest client
This commit adds support for BulkRequest execution in the High Level Rest client.

@tlrx tlrx force-pushed the tlrx:add-bulk-request-to-high-level-rest-client branch to d823ca7 Feb 23, 2017

@tlrx tlrx merged commit 7e3c06c into elastic:master Feb 23, 2017

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details

@tlrx tlrx deleted the tlrx:add-bulk-request-to-high-level-rest-client branch Feb 23, 2017

@tlrx

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2017

Thanks @javanna !

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 24, 2017

Merge branch 'master' into HEAD
* master: (54 commits)
  Keep the pipeline handler queue small initially
  Do not create String instances in 'Strings' methods accepting StringBuilder (elastic#22907)
  Tests: fix AwsS3ServiceImplTests
  Remove abstract InternalMetricsAggregation class (elastic#23326)
  Add BulkRequest support to High Level Rest client (elastic#23312)
  Wrap getCredentials() in a doPrivileged() block (elastic#23297)
  Respect promises on pipelined responses
  Align REST specs for HEAD requests
  Remove unnecessary result sorting in SearchPhaseController (elastic#23321)
  Fix SamplerAggregatorTests to have stable and predictable docIds
  Tests: Ensure multi node integ tests wait on first node
  Relocate a comment in HttpPipeliningHandler
  Add comments to HttpPipeliningHandler
  [TEST] Fix incorrect test cluster name in cluster health doc tests
  Build: Change location in zip of license and notice inclusion for plugins (elastic#23316)
  Script: Fix value of `ctx._now` to be current epoch time in milliseconds (elastic#23175)
  Build: Rework integ test setup and shutdown to ensure stop runs when desired (elastic#23304)
  Handle long overflow when adding paths' totals
  Don't set local node on cluster state used for node join validation (elastic#23311)
  Ensure that releasing listener is called
  ...

@javanna javanna referenced this pull request Nov 1, 2017

Open

Java high-level REST client completeness #27205

75 of 80 tasks complete
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.