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 Refresh API for RestHighLevelClient #27799

Merged
merged 17 commits into from Feb 28, 2018

Conversation

Projects
None yet
5 participants
@PnPie
Copy link
Contributor

commented Dec 13, 2017

Hello,
This is for adding Refresh API to RestHighLevelClient. And about

add fromXContent method to existing response class

I just think that as RefreshResponse extends BroadcastResponse and it doesn't add new fields but "just sub-class it", is it maybe a way to move all the PARSE stuff to BroadcastResponse so that the other sub classes like FlushResponse, ForceMergeResponse, ClearIndicesCacheResponse can re-use it without repeating the similar implementation ? (seems in this way in each sub-class, we need to cast the BroadcastResponse to it).
Relates to #27205

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Dec 13, 2017

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@javanna javanna self-requested a review Dec 14, 2017

@javanna javanna self-assigned this Dec 14, 2017

@javanna javanna referenced this pull request Jan 16, 2018

Open

Java high-level REST client completeness #27205

75 of 80 tasks complete
@javanna
Copy link
Member

left a comment

this looks pretty godo @PnPie thanks a lot for opening the PR. Sorry about the delay in getting back to you, I took the liberty to merge master in and push such changes to your remote, so if you make changes, you need to pull first from your own remote. Besides the few comments I left (one of which requires some discussions internally in our team), the PR is missing docs, it would be great if you can add those.

RefreshResponse refreshResponse =
execute(refreshRequest, highLevelClient().indices()::refresh, highLevelClient().indices()::refreshAsync);
// 10 shards per index by default
assertThat(refreshResponse.getTotalShards(), equalTo(indices.length * 10));

This comment has been minimized.

Copy link
@javanna

javanna Jan 16, 2018

Member

shall we also check successful and failed?

This comment has been minimized.

Copy link
@PnPie

PnPie Jan 16, 2018

Author Contributor

Seems RefreshResponse is a BroadcastResponse instead of an AcknowledgedResponse, so don't need to check if it's acknowledged ?

This comment has been minimized.

Copy link
@javanna

javanna Jan 17, 2018

Member

sure I was asking if we can check successful and failed rather than only total. does that make sense?

This comment has been minimized.

Copy link
@tlrx

tlrx Jan 17, 2018

Member

Maybe also check that successful is greater than 0, failed is greater or equal to 0 and that successful + failed is equal to total? So that the test does not rely on a fixed number of shards and replicas?

This comment has been minimized.

Copy link
@PnPie

PnPie Jan 18, 2018

Author Contributor

In fact successful shards + failed shards not always equal to total shards because of unallocated shards. they are not included in failed shards, of course nor in successful shards. It depends on the test environment. I run it locally with a single machine and for an index all the replicas shards are not allocated. So this is why I didn't check it. Maybe we can check successful > 0, failed == 0 ?

This comment has been minimized.

Copy link
@javanna

javanna Jan 19, 2018

Member

thanks for the explanation, your proposal sounds good to me

This comment has been minimized.

Copy link
@tlrx

tlrx Jan 22, 2018

Member

Sounds good too


RefreshResponse() {
}

RefreshResponse(int totalShards, int successfulShards, int failedShards, List<ShardOperationFailedException> shardFailures) {
super(totalShards, successfulShards, failedShards, shardFailures);
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {

This comment has been minimized.

Copy link
@javanna

javanna Jan 16, 2018

Member

should this rather go to BroadcastResponse given that all the fields are declared there?

This comment has been minimized.

Copy link
@PnPie

PnPie Jan 16, 2018

Author Contributor

Modified in new commit.

shardsParser.declareInt(constructorArg(), new ParseField(Fields.TOTAL));
shardsParser.declareInt(constructorArg(), new ParseField(Fields.SUCCESSFUL));
shardsParser.declareInt(constructorArg(), new ParseField(Fields.FAILED));
PARSER.declareObject(constructorArg(), shardsParser, new ParseField(Fields._SHARDS));

This comment has been minimized.

Copy link
@javanna

javanna Jan 16, 2018

Member

I think that we never print out nor parse back the shard failures. Should we rather do so? Looking at the current behaviour, I think that for this API we return an http error code (taken from the first shard failure) , yet we may not want to throw an exception in such but rather return a proper RefreshResponse that includes shard failures? Not sure though, especially because I don't know what error codes we would have to set to be ignored in RestHighLevelClient. Maybe we would already have the exception with shard failures returned in this case. @tlrx what do you think?

This comment has been minimized.

Copy link
@tlrx

tlrx Jan 17, 2018

Member

I think that we never print out nor parse back the shard failures. Should we rather do so?

We do it (kind of) in ReplicationResponse: it contains ShardInfo that have failures which are ShardOperationFailedException. I'm not sure that we can reuse the code easily but I don't think that we should ignore the shard failures and always return null.

I also don't like the fact that BroadcastResponse picks up the first shard failure for its HTTP response code, maybe there's a rationale around this but I don't know it. Since the error codes could be anything, I'm afraid we'll need to parse the very first fields before being able to know if it's an exception or a refresh response with failures.

This comment has been minimized.

Copy link
@PnPie

PnPie Jan 18, 2018

Author Contributor

I've a quick look at what we have done in ReplicationResponse's ShardInfo, so here we shoud also do the same thing to parse and transfer shard failures ?

This comment has been minimized.

Copy link
@javanna

javanna Jan 19, 2018

Member

yea we should do something similar. To make this feasible though we need #28312 to go in, so that it's enforced that only one type of exception needs to be parsed (ShardOperationFailedException is an interface)

@elastic elastic deleted a comment from elasticmachine Jan 16, 2018

@PnPie

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2018

Thanks for your review @javanna, and I'll add docs soon.

@tlrx
Copy link
Member

left a comment

Thanks a lot @PnPie for this contribution, this is greatly appreciated!

I left some comments, but even if the refresh API seems simple we need to handle correctly the shard failures and exceptions and this is where it gets tricky :(

@@ -111,4 +113,25 @@ public void openIndexAsync(OpenIndexRequest openIndexRequest, ActionListener<Ope
listener, Collections.emptySet(), headers);
}

/**
* Refresh one or more index using the Refresh API

This comment has been minimized.

Copy link
@tlrx

tlrx Jan 17, 2018

Member

Nit: one or more index -> one or more indices

* Refresh one or more index using the Refresh API
* <p>
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-refresh.html">
* Refresh API on elastic.co</a>

This comment has been minimized.

Copy link
@tlrx

tlrx Jan 17, 2018

Member

Nit: it can fit on the same line as the link without exceeding 140 chars

(Same for the async method too)

RefreshResponse refreshResponse =
execute(refreshRequest, highLevelClient().indices()::refresh, highLevelClient().indices()::refreshAsync);
// 10 shards per index by default
assertThat(refreshResponse.getTotalShards(), equalTo(indices.length * 10));

This comment has been minimized.

Copy link
@tlrx

tlrx Jan 17, 2018

Member

Maybe also check that successful is greater than 0, failed is greater or equal to 0 and that successful + failed is equal to total? So that the test does not rely on a fixed number of shards and replicas?

String[] nonExistentIndices = randomIndices(1, 5);
for (String nonExistentIndex : nonExistentIndices) {
if (indexExists(nonExistentIndex)) {
return;

This comment has been minimized.

Copy link
@tlrx

tlrx Jan 17, 2018

Member

That would ignore the end of the test. Instead you could simply execute a refresh with a unique "_does_not_exist" index?

/**
* The response of a refresh action.
*/
public class RefreshResponse extends BroadcastResponse {
public class RefreshResponse extends BroadcastResponse implements ToXContentFragment {

This comment has been minimized.

Copy link
@tlrx

tlrx Jan 17, 2018

Member

ToXContentFragment is not needed here as it has been added to BroadcastResponse

static {
ConstructingObjectParser<RefreshResponse, Void> shardsParser = new ConstructingObjectParser<>("_shards", true,
arg -> new RefreshResponse((int) arg[0], (int) arg[1], (int) arg[2], null));
shardsParser.declareInt(constructorArg(), new ParseField(Fields.TOTAL));

This comment has been minimized.

Copy link
@tlrx

tlrx Jan 17, 2018

Member

These fields belongs to BroadcastResponse so I think we could have something similar to CreateIndexResponse/AcknowledgedResponse?

I mean only the delcaration of the static ConstructingObjectParser<RefreshResponse, Void> PARSER in RefreshResponse with a static initialization block like:

static {
   BroadcastResponse.declareBroadcastFields(PARSER)
   ...
}

and in BroadcastResponse a static method to declare the ObjectParser fields like AcknowledgedResponse.declareAcknowledgedField()

What do you think?

This comment has been minimized.

Copy link
@PnPie

PnPie Jan 18, 2018

Author Contributor

Yeah, as these fields belong to BroadcastResponse, we should do it there and then all the subclasses can use it.

@@ -127,4 +129,21 @@ public void writeTo(StreamOutput out) throws IOException {
exp.writeTo(out);
}
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {

This comment has been minimized.

Copy link
@tlrx

tlrx Jan 17, 2018

Member

If we want to make BroadcastResponse implementing ToXContentFragment then we need to bring in the logic from RestActions.buildBroadcastShardsHeader() (but maybe not the exceptions grouping thing) here? And then change the calling methods in server so that they use this toXContent() instead of buildBroadcastShardsHeader?

This comment has been minimized.

Copy link
@PnPie

PnPie Jan 18, 2018

Author Contributor

Yes, I see that we have build RefreshResponse in this way in server side ? So we don't even need to implement ToXContentFragment here ?

This comment has been minimized.

Copy link
@tlrx

tlrx Jan 22, 2018

Member

So you want to reuse RestActions.buildBroadcastShardsHeader() in RefreshResponse.toXContent() instead?

This comment has been minimized.

Copy link
@PnPie

PnPie Jan 27, 2018

Author Contributor

Hello @tlrx, srry for not having responded in time because I just want to see a bit more to discuss.I mean we don't need to make BroadcastResponse or RefreshResonse implementing ToXContentFragment (then to implement an toXContent() method), do we ? All the work has done here https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestRefreshAction.java#L64-L66 ?

This comment has been minimized.

Copy link
@tlrx

tlrx Jan 29, 2018

Member

srry for not having responded in time because I just want to see a bit more to discuss.

Don't be sorry at all! You're helping to build a better RestHighLevelClient and it's time consuming, it's normal :)

I mean we don't need to make BroadcastResponse or RefreshResonse implementing ToXContentFragment (then to implement an toXContent() method), do we ?

Well, the RestHighLevelClient does not render the response objects so I agree that we don't really need them to add the Refresh API. In the high level client we only need to parse the XContent corresponding to a ResfreshResponse object and that's the role of the RefreshResponse.fromXContent() method.

But we need to test this fromXContent method and we have to be sure that a RefreshResponse object will always be generated the same way, in a way that can be parsed by the fromXContent method. We also try to be a bit lenient when we parse back the XContent so that it does not break when new fields are added or when fields are shuffled in the XContent. If you search for testToAndFromXContent() in the source code you'll see many of tests like this, where we rely on the toXcontent/fromXcontent methods for testing parsing/rendering of request/response objects. This is something we do since the begining of the RestHighLevelClient coding and I should we need to continue like this. We are also ok to clean up parsing or rendering logic in core if that helps the client and makes things cleaner.

By having the XContent building logic in the RestRefreshAction we have a strong link between the Response object and the Rest layer (and all stuff brought by it). I think that we should be able to render a RefreshResponse by itself, so I'd love to see it implementing toXContent (or toXContentFragment if it's easier) at some point.

shardsParser.declareInt(constructorArg(), new ParseField(Fields.TOTAL));
shardsParser.declareInt(constructorArg(), new ParseField(Fields.SUCCESSFUL));
shardsParser.declareInt(constructorArg(), new ParseField(Fields.FAILED));
PARSER.declareObject(constructorArg(), shardsParser, new ParseField(Fields._SHARDS));

This comment has been minimized.

Copy link
@tlrx

tlrx Jan 17, 2018

Member

I think that we never print out nor parse back the shard failures. Should we rather do so?

We do it (kind of) in ReplicationResponse: it contains ShardInfo that have failures which are ShardOperationFailedException. I'm not sure that we can reuse the code easily but I don't think that we should ignore the shard failures and always return null.

I also don't like the fact that BroadcastResponse picks up the first shard failure for its HTTP response code, maybe there's a rationale around this but I don't know it. Since the error codes could be anything, I'm afraid we'll need to parse the very first fields before being able to know if it's an exception or a refresh response with failures.

PnPie added some commits Jan 27, 2018

@PnPie

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2018

Hello @javanna @tlrx, srry for the late update, I just pushed some changes to see and discuss.

@javanna
Copy link
Member

left a comment

thanks @PnPie I left some more comments. We are on the right track but I would like @tlrx to have a look too. Also we will need to add unit tests for RefreshResponse and DefaultShardOperationFailedException parsing code, as well as docs for the new API and at least a unit test to RequestTests which tests the newly added Request#refresh method.

@@ -206,6 +207,26 @@ public boolean existsAlias(GetAliasesRequest getAliasesRequest, Header... header
*/
public void existsAliasAsync(GetAliasesRequest getAliasesRequest, ActionListener<Boolean> listener, Header... headers) {
restHighLevelClient.performRequestAsync(getAliasesRequest, Request::existsAlias, RestHighLevelClient::convertExistsResponse,
listener, emptySet(), headers);
listener, emptySet(), headers);

This comment has been minimized.

Copy link
@javanna

javanna Jan 29, 2018

Member

can we avoid unnecessary changes like this and the ones above?

@@ -206,6 +207,11 @@ static Request putMapping(PutMappingRequest putMappingRequest) throws IOExceptio
return new Request(HttpPut.METHOD_NAME, endpoint, parameters.getParams(), entity);
}

static Request refresh(RefreshRequest refreshRequest) {
String endpoint = endpoint(refreshRequest.indices(), Strings.EMPTY_ARRAY, "_refresh");

This comment has been minimized.

Copy link
@javanna

javanna Jan 29, 2018

Member

you can now call endpoint(String[], String) instead

@@ -331,10 +335,34 @@ public void testCloseNonExistentIndex() throws IOException {

CloseIndexRequest closeIndexRequest = new CloseIndexRequest(nonExistentIndex);
ElasticsearchException exception = expectThrows(ElasticsearchException.class,
() -> execute(closeIndexRequest, highLevelClient().indices()::close, highLevelClient().indices()::closeAsync));
() -> execute(closeIndexRequest, highLevelClient().indices()::close, highLevelClient().indices()::closeAsync));

This comment has been minimized.

Copy link
@javanna

javanna Jan 29, 2018

Member

let's revert these formatting changes please?

RefreshResponse refreshResponse =
execute(refreshRequest, highLevelClient().indices()::refresh, highLevelClient().indices()::refreshAsync);
assertThat(refreshResponse.getTotalShards(), equalTo(DEFAULT_SHRADS_NUMBER_PER_INDEX));
assertThat(refreshResponse.getSuccessfulShards(), greaterThan(0));

This comment has been minimized.

Copy link
@javanna

javanna Jan 29, 2018

Member

should it be either 5 or 10 depending on the number of nodes? I think you can do either(equalTo(5)).or(equalTo(10)) .

final int DEFAULT_SHRADS_NUMBER_PER_INDEX = 10;
{
String index = "index";
createIndex(index);

This comment has been minimized.

Copy link
@javanna

javanna Jan 29, 2018

Member

you can also specify the number of shards yourself so you don't rely on any default.


private static final class Fields {

This comment has been minimized.

Copy link
@javanna

javanna Jan 29, 2018

Member

let's keep these constants as top-level fields, no need for the Fields inner class. This is something we used to do in the past that we want to stop doing it and remove in the future.

builder.field("index", index());
builder.field("shard", shardId());

This comment has been minimized.

Copy link
@javanna

javanna Jan 29, 2018

Member

why did you move the shard field after the index field?


private static final class Fields {
static final String INDEX = "index";
static final String SHARD_ID = "shardId";

This comment has been minimized.

Copy link
@javanna

javanna Jan 29, 2018

Member

I don't think that shardId is a valid field here. Should it rather be shard?

static {
PARSER.declareString(constructorArg(), new ParseField(Fields.INDEX));
PARSER.declareInt(constructorArg(), new ParseField(Fields.SHARD_ID));
PARSER.declareObject(optionalConstructorArg(), (p, c) -> ElasticsearchException.fromXContent(p), new ParseField(Fields.REASON));

This comment has been minimized.

Copy link
@javanna

javanna Jan 29, 2018

Member

I take it that you deliberately skipped status, I wonder if we should also skip index and shard, given that we will always have an ElasticsearchException back we should rather use the DefaultShardOperationFailedException(ElasticsearchException) constructor all the time, then index, shard and status will be extracted from such exception.

import java.util.List;

/**
* The response of a refresh action.
*/
public class RefreshResponse extends BroadcastResponse {

private static final ConstructingObjectParser<RefreshResponse, Void> PARSER = new ConstructingObjectParser<>("refresh", true,
arg -> {
BroadcastResponse response = (BroadcastResponse) arg[0];

This comment has been minimized.

Copy link
@javanna

javanna Jan 29, 2018

Member

I wonder if there's a way to avoid creating an instance of BroadcastResponse given that we use it only to transport the parsed values, maybe we can get multiple arguments directly.

This comment has been minimized.

Copy link
@javanna

javanna Feb 9, 2018

Member

I tried it out and understood that it is not possible. It is fine to do it the way you are doing it, by using an intermediate broadcast response object.

@PnPie

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2018

Hello @javanna @tlrx,
Thanks for your comments and explanation, I did some changes regarding to what you have mentioned. But I may have some problems and questions concerning DefaultShardOperationFailedException parsing, I just pushed the commit to see.

I tried to test parsing a RefreshResponse with DefaultShardOperationFailedException in a new RefreshResponseTests, but I have problems when I parsed back the RefreshResponse XContent here, I got a ParsingException:

ParsingException[[refresh] failed to parse field [_shards]]; nested: ParsingException[[_shards] failed to parse field [failures]]; nested: ParsingException[[failures] failed to parse field [reason]]; nested: ParsingException[Failed to build [failures] after last required field arrived]; nested: NullPointerException;

I didn't see very well why it failed in parsing the [reason] field, and the parse code seems fine ? do you have any idea about it ?

And for what you have mentioned @javanna, in BroadcastResponse, I also tried to use Object[] to tansfer fields to its subclass instead of using a BroadcastResponse instance, like this:
ConstructingObjectParser<Object[], Void> shardsParser = new ConstructingObjectParser<>("_shards", true, arg -> new Object[]{arg[0], arg[1], arg[2], arg[3]});

But in RefreshResponse I can't cast these args back to their original type (ex. Integer) with the exception:

ParsingException[[refresh] failed to parse field [_shards]]; nested: ParsingException[Failed to build [refresh] after last required field arrived]; nested: ClassCastException[java.base/[Ljava.lang.Object; cannot be cast to java.base/java.lang.Integer];

Seems as we have 3 Integer fields and a List<DefaultShardOperationFailedException> field, we can't put them in a primitive array to transfer ? we have to create an object which holds them (like BroadcastResponse here) ? Or are there some other ways ?

Thanks in advance.

@javanna

This comment has been minimized.

Copy link
Member

commented Feb 9, 2018

@PnPie I had a look, I think you're on the right track. The problem was caused by a bug in ElasticsearchException, which I fixed with a small commit on your remote. That should allow you to go on with your PR. I will leave a couple more comments that should help you. I do see that this is not trivial, so don't feel bad about asking for help, and it's actually a good change as it will allow to add support for other APIs later in no time.

private ShardId shardId;
private RestStatus status;

public FakeElasticsearchException(String index, int shardId, RestStatus status, String msg) {

This comment has been minimized.

Copy link
@javanna

javanna Feb 9, 2018

Member

do we need to subclass ElasticsearchException here? Can't we use ElasticsearchException directly?

This comment has been minimized.

Copy link
@PnPie

PnPie Feb 11, 2018

Author Contributor

Yeah we can use ElasticsearchException directly, I did this at the beginning just because I want to set an index and shardId for an ElasticsearchException. If we create it directly it will be more complicated to set them ? Otherwise we can leave it null, like what I do now. Seems it's also okay here and not a big problem ?

assertThat(response.getTotalShards(), equalTo(parsedResponse.getTotalShards()));
assertThat(response.getSuccessfulShards(), equalTo(parsedResponse.getSuccessfulShards()));
assertThat(response.getFailedShards(), equalTo(parsedResponse.getFailedShards()));
assertThat(response.getShardFailures(), equalTo(parsedResponse.getShardFailures()));

This comment has been minimized.

Copy link
@javanna

javanna Feb 9, 2018

Member

I am afraid that this assertion may not work, given that what we parse back is always an ElasticsearchException rather than the original type. That is expected and makes testing exceptions parsing trickier. See other tests around this.

@PnPie

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2018

@javanna thanks so much for the fix ! I did the changes and I hope it looks more like what it should be :)

@javanna
Copy link
Member

left a comment

hi @PnPie thanks for the changes! This is getting closer! I left some more comments, once you addressed those it should be ready. I also took the liberty to merge master in and add the new docs page where appropriate so it is rendered (remember to pull ;) ).

}

private void doFromXContentTestWithRandomFields(boolean addRandomFields) throws IOException {
RefreshResponse response = new RefreshResponse(10, 10, 0, failures);

This comment has been minimized.

Copy link
@javanna

javanna Feb 12, 2018

Member

would you mind adding a createTestItem method that returns a randomly created RefreshResponse ? And randomize the different numbers (still making sure that successful is never higher that total etc.)? Aldo make failed consistent with the number of failures maybe so that the test is more realistic?

private static List<DefaultShardOperationFailedException> failures;

@BeforeClass
public static void prepareException() {

This comment has been minimized.

Copy link
@javanna

javanna Feb 12, 2018

Member

maybe we can remove this method and just make this part of the proposed createTestItem method. I would prefer to have a random number of failures. from 0 to a few. Shall also try and create the DefaultShardOperationException by providing the index etc. (other constructor) ? It would be nice to also have it created in some cases from an ElasticsearchException that has such metadata set .

compareFailures(response.getShardFailures(), parsedResponse.getShardFailures());
}

private static void compareFailures(DefaultShardOperationFailedException[] original,

This comment has been minimized.

Copy link
@javanna

javanna Feb 12, 2018

Member

Maybe we want to have a separate test class DefaultShardOperationFailedExceptionTests where we can test toXContent and fromXContent for DefaultShardOperationFailedException , similarly to what we do in ElasticsearchExceptionTests

"failures", true, arg -> new DefaultShardOperationFailedException((ElasticsearchException) arg[0]));

static {
PARSER.declareObject(constructorArg(), (p, c) -> ElasticsearchException.fromXContent(p), new ParseField(REASON));

This comment has been minimized.

Copy link
@javanna

javanna Feb 12, 2018

Member

I think that if you ignore shard, index and status it's ok when they are derived from the inner ElasticsearchException, but if will not work when the reason is a Throwable, which I think we are currently not testing.

@@ -211,6 +212,11 @@ static Request putMapping(PutMappingRequest putMappingRequest) throws IOExceptio
return new Request(HttpPut.METHOD_NAME, endpoint, parameters.getParams(), entity);
}

static Request refresh(RefreshRequest refreshRequest) {
String endpoint = endpoint(refreshRequest.indices(), "_refresh");

This comment has been minimized.

Copy link
@javanna

javanna Feb 12, 2018

Member

we need to also take the indices options from the refresh request and set them as parameters. See Params#withIndicesOptions

<3> failed to refresh shards number
<4> an array of `DefaultShardOperationFailedException` if exist, otherwise it will be an empty array

If the indices were not found, an `ElasticsearchException` will be thrown:

This comment has been minimized.

Copy link
@javanna

javanna Feb 12, 2018

Member

add "by default" ? that's because you can affect this behaviour through the different indices options.


{
createIndex("index1", Settings.EMPTY);
createIndex("index2", Settings.EMPTY);

This comment has been minimized.

Copy link
@javanna

javanna Feb 12, 2018

Member

do we need to create the second index?

builder.endArray();
}
builder.endObject();
return builder;

This comment has been minimized.

Copy link
@javanna

javanna Feb 12, 2018

Member

why not calling RestActions#buildBroadcastShardsHeader instead ? Aren't we losing support for the group_shard_failures flag? It is not relevant for the high-level REST client as there is no way to set it but I think it's important given that the parsing code is in ES core. Which reminds me, we should probably test this as well in RefreshResponseTests. This param can be passed in as part of the ToXContent.Params when calling toXContent

This comment has been minimized.

Copy link
@PnPie

PnPie Feb 20, 2018

Author Contributor

It doesn't really matter if we just call RestActions#buildBroadcastShardsHeader here ? seems we have talked a little before with @tlrx , like this, this method will strongly depend on RestActions ? Do it seperately is to not have the XContent building logic in the RestRefreshAction and kind of "decouple" the Response object and REST Layer ?

This comment has been minimized.

Copy link
@javanna

javanna Feb 20, 2018

Member

I see why we may not want to call RestActions#buildBroadcastShardsHeader, but I think that we are missing a part of it right now, and I am not sure we want to duplicate it. I would call it and add a TODO, till we can move it completely to BRoadcastResponse and remove the method from RestActions. How does this sound?

This comment has been minimized.

Copy link
@tlrx

tlrx Feb 20, 2018

Member

This sounds good!

@@ -62,7 +62,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
@Override
public RestResponse buildResponse(RefreshResponse response, XContentBuilder builder) throws Exception {
builder.startObject();
buildBroadcastShardsHeader(builder, request, response);

This comment has been minimized.

Copy link
@javanna

javanna Feb 12, 2018

Member

remove the unused static import from this class?

@@ -62,7 +62,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
@Override
public RestResponse buildResponse(RefreshResponse response, XContentBuilder builder) throws Exception {
builder.startObject();
buildBroadcastShardsHeader(builder, request, response);
response.toXContent(builder, request);
builder.endObject();
return new BytesRestResponse(response.getStatus(), builder);

This comment has been minimized.

Copy link
@javanna

javanna Feb 12, 2018

Member

you can shorten this by using RestToXContentListener instead of RestBuilderListener. If a response implements ToXContent, we don't need to do anything special.

This comment has been minimized.

Copy link
@PnPie

PnPie Feb 21, 2018

Author Contributor

I see that for RestToXContentListener, its generic type should be a ToXContentObject but our BroadcastResponse is currently a ToXContentFragment. If we change it to ToXContentObject, we also need to modify all the subclasses of it. So maybe here we just keep it like this for this moment ?

This comment has been minimized.

Copy link
@javanna

javanna Feb 23, 2018

Member

right, we can do that as a followup

@javanna

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

heya @PnPie , how's it going? Did you have a chance to look at my comments? Please let me know if they make sense and if I can help in any way to get this in, as I said in my last review it's very close. Thanks!

@PnPie

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2018

Hello @javanna it's fine ! :) I looked at your comments and just did the changes these days. Last week I was kind of on vacation for a holiday. I will push the changes these days once I finished so that we can move on. An only quick question I commented on comments ! Thx

PnPie added some commits Feb 20, 2018

@PnPie

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2018

Hello @javanna , I made the changes, and .. srry for having made the history a mess...

@javanna
Copy link
Member

left a comment

hi @PnPie thanks for updating the PR, I left a few very minor comments but this looks great now, I will merge it as soon as you addressed those last nits. @tlrx do you want to have a last look?

public void testRefresh() throws IOException {
{
final int numberOfShards = randomIntBetween(1, 5);
final int numberOfReplicas = randomIntBetween(1, 3);

This comment has been minimized.

Copy link
@javanna

javanna Feb 23, 2018

Member

I think I changed my mind on this. Given that it's an integration test, let's remove the randomization which complicate things? Let's just set 1 shard and 0 replica?

This comment has been minimized.

Copy link
@PnPie

PnPie Feb 24, 2018

Author Contributor

Yes, I also find the assert part is too complicated .. :)


@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
// TODO: move BroadcastResponse building codes from RestActions to itself here

This comment has been minimized.

Copy link
@javanna

javanna Feb 23, 2018

Member

I know that I told you to add this TODO, but I looked deeper and I don't think we will address this anytime soon. Let's remove it, I think what we do now is fine.


public class DefaultShardOperationFailedException implements ShardOperationFailedException {

private static final String INDEX = "index";
private static final String SHARDID = "shard";

This comment has been minimized.

Copy link
@javanna

javanna Feb 23, 2018

Member

call it SHARD_ID please?

(builder, params) -> exception.toXContent(builder, params), XContentType.JSON, randomBoolean());
String exceptionJson = XContentHelper.convertToJson(exceptionBytes, false, XContentType.JSON);
assertEquals(exceptionJson, expectedJson);
}

This comment has been minimized.

Copy link
@javanna

javanna Feb 23, 2018

Member

this method can be removed in favour of doing assertEquals("expected json", Strings.toString(toXContent));

@PnPie

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2018

Hello @javanna, I updated it.
And as we discussed previously, is it worth changing BroadcastResponse from ToXContentFragment to ToXContentObject right after this ? I see the other Response classes are normally ToXContentObject, as the Response is rather a complete object than several fields to insert into a XContent ?

@javanna

This comment has been minimized.

Copy link
Member

commented Feb 27, 2018

jenkins test this please

@javanna

This comment has been minimized.

Copy link
Member

commented Feb 27, 2018

heya @PnPie thanks for the update. I am going to merge this in as soon as I get a green build.

And as we discussed previously, is it worth changing BroadcastResponse from ToXContentFragment to ToXContentObject right after this ? I see the other Response classes are normally ToXContentObject, as the Response is rather a complete object than several fields to insert into a XContent ?

Yes, that sounds good to me. I would be happy to review that if you send a PR for it ;)

@PnPie

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2018

@javanna Cool, I'll work on this. And seems tests failed cause a line > 140 characters. I updated it.

@javanna

This comment has been minimized.

Copy link
Member

commented Feb 27, 2018

retest this please

<1> total number of shards hit by the refresh request
<2> number of shards where the refresh has succeeded
<3> number of shards where the refresh has failed
<4> a list of failures if the operation failed on one or more shards

This comment has been minimized.

Copy link
@tlrx

tlrx Feb 28, 2018

Member

Nit: It would be nice if it's capitalized "number" -> "Number" etc

@tlrx

tlrx approved these changes Feb 28, 2018

Copy link
Member

left a comment

LGTM

Thanks a lot @PnPie and @javanna !

@javanna javanna merged commit 95dea24 into elastic:master Feb 28, 2018

1 check passed

CLA Commit author is a member of Elasticsearch
Details

javanna added a commit that referenced this pull request Feb 28, 2018

PnPie added a commit to PnPie/elasticsearch that referenced this pull request Mar 1, 2018

Change BroadcastResponse from ToXContentFragment to ToXContentObject
While working on elastic#27799, we find that it might make sense to change BroadcastResponse from ToXContentFragment to ToXContentObject, seeing that it's rather a complete XContent object and also the other Responses are normally ToXContentObject.

By doing this, we can also move the XContent build logic of BroadcastResponse's subclasses, from Rest Layer to the concrete classes themselves.

sebasjm pushed a commit to sebasjm/elasticsearch that referenced this pull request Mar 10, 2018

javanna added a commit that referenced this pull request Mar 23, 2018

Change BroadcastResponse from ToXContentFragment to ToXContentObject (#…
…28878)

While working on #27799, we find that it might make sense to change BroadcastResponse from ToXContentFragment to ToXContentObject, seeing that it's rather a complete XContent object and also the other Responses are normally ToXContentObject.

By doing this, we can also move the XContent build logic of BroadcastResponse's subclasses, from Rest Layer to the concrete classes themselves.

Relates to #3889

javanna added a commit that referenced this pull request Mar 23, 2018

Change BroadcastResponse from ToXContentFragment to ToXContentObject (#…
…28878)

While working on #27799, we find that it might make sense to change BroadcastResponse from ToXContentFragment to ToXContentObject, seeing that it's rather a complete XContent object and also the other Responses are normally ToXContentObject.

By doing this, we can also move the XContent build logic of BroadcastResponse's subclasses, from Rest Layer to the concrete classes themselves.

Relates to #3889

@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

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.