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.

Sign up for GitHub

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
Merged

Add Refresh API for RestHighLevelClient #27799

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
e8efd85
Add Refresh API for RestHighLevelClient
PnPie Dec 13, 2017
6613822
Merge branch 'master' into rest_highlevel_client_refresh
javanna Jan 16, 2018
77da3d5
changes
PnPie Jan 16, 2018
1753a33
update
PnPie Jan 27, 2018
90756b9
Merge branch 'master' of github.com:elastic/elasticsearch into rest_h…
PnPie Jan 27, 2018
a7cbc48
fix conflicts
PnPie Jan 27, 2018
1e7c019
update
PnPie Feb 4, 2018
18086a3
shardId can be null, shardId().getId() may throw NPE.
javanna Feb 9, 2018
ed89a7a
update
PnPie Feb 11, 2018
8ee43f8
Merge branch 'master' into rest_highlevel_client_refresh
javanna Feb 12, 2018
7581273
add refresh page
javanna Feb 12, 2018
0168ed5
update
PnPie Feb 19, 2018
7d4ec22
update
PnPie Feb 20, 2018
fa9cf77
update
PnPie Feb 21, 2018
9c03b74
small changes
PnPie Feb 24, 2018
9fc4529
reformat code
PnPie Feb 27, 2018
364de26
address last tiny bits
javanna Feb 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions client/rest-high-level/src/main/java/org/elasticsearch/client/IndicesClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
import org.elasticsearch.action.admin.indices.delete.DeleteIndexResponse;
import org.elasticsearch.action.admin.indices.open.OpenIndexRequest;
import org.elasticsearch.action.admin.indices.open.OpenIndexResponse;
import org.elasticsearch.action.admin.indices.refresh.RefreshRequest;
import org.elasticsearch.action.admin.indices.refresh.RefreshResponse;

import java.io.IOException;
import java.util.Collections;
Expand Down Expand Up @@ -111,4 +113,25 @@ public void openIndexAsync(OpenIndexRequest openIndexRequest, ActionListener<Ope
listener, Collections.emptySet(), headers);
}

/**
* Refresh one or more index using the Refresh API
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* <p>
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-refresh.html">
* Refresh API on elastic.co</a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(Same for the async method too)

*/
public final RefreshResponse refresh(RefreshRequest refreshRequest, Header... headers) throws IOException {
return restHighLevelClient.performRequestAndParseEntity(refreshRequest, Request::refresh, RefreshResponse::fromXContent,
Collections.emptySet(), headers);
}

/**
* Asynchronously 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>
*/
public final void refreshAsync(RefreshRequest refreshRequest, ActionListener<RefreshResponse> listener, Header... headers) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to remove the final modifier from these tests. The class is already final, hence our checkstyle checks will fail with this redundant modifiers. You can check this out by running gradlew check from client/rest-high-level

restHighLevelClient.performRequestAsyncAndParseEntity(refreshRequest, Request::refresh, RefreshResponse::fromXContent,
listener, Collections.emptySet(), headers);
}
}
6 changes: 6 additions & 0 deletions client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.elasticsearch.action.admin.indices.create.CreateIndexRequest;
import org.elasticsearch.action.admin.indices.delete.DeleteIndexRequest;
import org.elasticsearch.action.admin.indices.open.OpenIndexRequest;
import org.elasticsearch.action.admin.indices.refresh.RefreshRequest;
import org.elasticsearch.action.bulk.BulkRequest;
import org.elasticsearch.action.delete.DeleteRequest;
import org.elasticsearch.action.get.GetRequest;
Expand Down Expand Up @@ -152,6 +153,11 @@ static Request openIndex(OpenIndexRequest openIndexRequest) {
return new Request(HttpPost.METHOD_NAME, endpoint, parameters.getParams(), null);
}

static Request refresh(RefreshRequest refreshRequest) {
String endpoint = endpoint(refreshRequest.indices(), Strings.EMPTY_ARRAY, "_refresh");
return new Request(HttpPost.METHOD_NAME, endpoint, Collections.emptyMap(), null);
}

static Request createIndex(CreateIndexRequest createIndexRequest) throws IOException {
String endpoint = endpoint(createIndexRequest.indices(), Strings.EMPTY_ARRAY, "");

Expand Down
2 changes: 0 additions & 2 deletions client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.ActionRequest;
import org.elasticsearch.action.ActionRequestValidationException;
import org.elasticsearch.action.admin.indices.open.OpenIndexRequest;
import org.elasticsearch.action.admin.indices.open.OpenIndexResponse;
import org.elasticsearch.action.bulk.BulkRequest;
import org.elasticsearch.action.bulk.BulkResponse;
import org.elasticsearch.action.delete.DeleteRequest;
Expand Down
38 changes: 32 additions & 6 deletions client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesClientIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,22 +27,22 @@
import org.elasticsearch.action.admin.indices.delete.DeleteIndexResponse;
import org.elasticsearch.action.admin.indices.open.OpenIndexRequest;
import org.elasticsearch.action.admin.indices.open.OpenIndexResponse;
import org.elasticsearch.action.admin.indices.refresh.RefreshRequest;
import org.elasticsearch.action.admin.indices.refresh.RefreshResponse;
import org.elasticsearch.action.support.IndicesOptions;
import org.elasticsearch.rest.RestStatus;

import java.io.IOException;
import java.util.Locale;

import static org.hamcrest.Matchers.equalTo;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.common.xcontent.json.JsonXContent;
import org.elasticsearch.rest.RestStatus;

import java.io.IOException;
import java.util.Locale;
import java.util.Map;

import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_NUMBER_OF_REPLICAS;
import static org.hamcrest.Matchers.equalTo;

public class IndicesClientIT extends ESRestHighLevelClientTestCase {

Expand Down Expand Up @@ -180,6 +180,32 @@ public void testOpenNonExistentIndex() throws IOException {
assertEquals(RestStatus.NOT_FOUND, strictException.status());
}

public void testRefresh() throws IOException {
{
String[] indices = randomIndices(1, 5);
for (String index : indices) {
createIndex(index);
}
RefreshRequest refreshRequest = new RefreshRequest(indices);
RefreshResponse refreshResponse =
execute(refreshRequest, highLevelClient().indices()::refresh, highLevelClient().indices()::refreshAsync);
// 10 shards per index by default
assertThat(refreshResponse.getTotalShards(), equalTo(indices.length * 10));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we also check successful and failed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the explanation, your proposal sounds good to me

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good too

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
RefreshRequest refreshRequest = new RefreshRequest(nonExistentIndices);
ElasticsearchException exception = expectThrows(ElasticsearchException.class,
() -> execute(refreshRequest, highLevelClient().indices()::refresh, highLevelClient().indices()::refreshAsync));
assertEquals(RestStatus.NOT_FOUND, exception.status());
}
}

private static String[] randomIndices(int minIndicesNum, int maxIndicesNum) {
int numIndices = randomIntBetween(minIndicesNum, maxIndicesNum);
String[] indices = new String[numIndices];
Expand Down
24 changes: 23 additions & 1 deletion server/src/main/java/org/elasticsearch/action/admin/indices/refresh/RefreshResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,40 @@

import org.elasticsearch.action.ShardOperationFailedException;
import org.elasticsearch.action.support.broadcast.BroadcastResponse;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.xcontent.ConstructingObjectParser;
import org.elasticsearch.common.xcontent.ToXContentFragment;
import org.elasticsearch.common.xcontent.XContentParser;

import java.util.List;

import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}

RefreshResponse() {
}

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

public static RefreshResponse fromXContent(XContentParser parser) {
return PARSER.apply(parser, null);
}
}
23 changes: 21 additions & 2 deletions server/src/main/java/org/elasticsearch/action/support/broadcast/BroadcastResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,10 @@
import org.elasticsearch.action.ShardOperationFailedException;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.common.xcontent.ToXContentFragment;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.index.shard.ShardNotFoundException;
import org.elasticsearch.rest.RestStatus;

import java.io.IOException;
import java.util.List;
Expand All @@ -34,7 +36,7 @@
/**
* Base class for all broadcast operation based responses.
*/
public class BroadcastResponse extends ActionResponse {
public class BroadcastResponse extends ActionResponse implements ToXContentFragment{
private static final ShardOperationFailedException[] EMPTY = new ShardOperationFailedException[0];
private int totalShards;
private int successfulShards;
Expand Down Expand Up @@ -127,4 +129,21 @@ public void writeTo(StreamOutput out) throws IOException {
exp.writeTo(out);
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

builder.startObject(Fields._SHARDS);
builder.field(Fields.TOTAL, getTotalShards());
builder.field(Fields.SUCCESSFUL, getSuccessfulShards());
builder.field(Fields.FAILED, getFailedShards());
builder.endObject();
return builder;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@PnPie PnPie Feb 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds good!

}

public static final class Fields {
public static final String _SHARDS = "_shards";
public static final String TOTAL = "total";
public static final String SUCCESSFUL = "successful";
public static final String FAILED = "failed";
}
}