REST high-level client: add synced flush API #29189

sohaibiftikhar · 2018-03-21T10:55:17Z

Relates to #27205
Moved to #30650

WIP: Need clarifications

elasticmachine · 2018-03-21T10:55:19Z

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?

elasticmachine · 2018-03-21T10:55:19Z

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?

sohaibiftikhar · 2018-03-21T10:56:54Z

@javanna This turned out to be a bit more complex than I initially thought it to be. It seems a bit tricky to reconstruct the SyncedFlushResponse object back from its JSON/XContent. Could you please have a look at SyncedFlushResponse::fromXContent to see if this is at least going in the right direction?

elasticmachine · 2018-03-21T10:56:55Z

Pinging @elastic/es-core-infra

— Fixed issues with parser for null values — Cleaned up the response parsing for SyncedFlush — TODO: Run complete test suite and check style cleanup

nik9000

Oh boy!

I wonder if in this case we shouldn't parse the normal response object and make a new one that is more like what is returned over the REST API. This is just so different and you have to go through so many hoops to make so many thing parseable I wonder if it is worth breaking from tradition here.

@javanna, I know you were reviewing this too, what do you think?

nik9000 · 2018-03-26T13:52:04Z

server/src/main/java/org/elasticsearch/action/admin/indices/flush/SyncedFlushResponse.java

+ Map<String, ShardCounts> shardsCountsPerIndex = new HashMap<>();
+ Map<String, List<ShardsSyncedFlushResult>> shardsResultPerIndex = new HashMap<>();
+ // If it is an object we try to parse it for Fields._SHARD or for an index entry
+ for (Token curToken = parser.currentToken(); curToken != Token.END_OBJECT; curToken = parser.nextToken()) {


I agree that we need to parse the top level object by hand because of the unexpected names. ObjectParser doesn't support that sort of thing.

nik9000 · 2018-03-26T13:53:07Z

server/src/main/java/org/elasticsearch/action/admin/indices/flush/SyncedFlushResponse.java

+ Integer successfulShards = null;
+ Integer failedShards = null;
+ Map<ShardId, List<FailureContainer>> failures = new HashMap<>();
+ if (curToken == Token.START_OBJECT) { // Start parsing for _shard or for index


Compared to our other parsing code this is a little weird because it doesn't know what field it is parsing up front. I get why you do this, but it is weird.

Also it is weird because we don't serialize all that much information. You get almost nothing if there isn't an error.

~~This is indeed true. Which is why I agree with your initial idea. It would be much better to have a separate response object altogether as this whole effort felt more like a workaround to me.~~ Sorry, I misunderstood initially. We could probably have something like

{ "_shards" : ... "indexes" : { "index1" : { ... }, "index2" : { ... }, ... } }

And then parse the index information like a Map. But I am not sure about the repercussions that changing the JSON structure might have.

we can't change the format of the response at this time. We can at some point, but for now we just have to parse what we have, and figure out what we should do to make things better for the future, meaning potentially breaking changes etc.

nik9000 · 2018-03-26T14:18:21Z

server/src/main/java/org/elasticsearch/cluster/routing/ShardRouting.java

- .field("relocating_node", relocatingNodeId())
- .field("shard", id())
- .field("index", getIndexName());
+ .field(Fields.STATE, state())


We've stopped making these Fields objects in a past year or so and just started to use string constants or even quoted strings.

javanna

I left some initial comments, thanks a lot for your efforts @sohaibiftikhar I definitely didn't realize how complicated this API was going to be, I changed its difficulty in the meta issue to "medium". I definitely didn't see that it was printing out ShardRouting hence a lot of other stuff that needs to be parsed back. On alternative proposals, I am open to ideas, but if this is what we return through REST today and what we print out, I don't follow how using a different object would make things simpler. Maybe changing the format of the REST response would, but that is a different matter. @nik9000 could you clarify?

javanna · 2018-03-22T12:44:47Z

client/rest-high-level/src/main/java/org/elasticsearch/client/IndicesClient.java

+ * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-synced-flush.html">
+ * Synced flush API on elastic.co</a>
+ */
+ public void syncedFlushAsync(SyncedFlushRequest syncedFlushRequest, ActionListener<SyncedFlushResponse> listener, Header... headers) {


as odd as this sounds, could you rename the methods to flushSynced as that's how this API is referred to in our SPEC ? request and response can and should stay the same.

javanna · 2018-03-22T12:46:22Z

client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java

@@ -233,6 +234,15 @@ static Request flush(FlushRequest flushRequest) {
 return new Request(HttpPost.METHOD_NAME, endpoint, parameters.getParams(), null);
 }

+ static Request syncedFlush(SyncedFlushRequest syncedFlushRequest) {
+ String[] indices = syncedFlushRequest.indices() == null ? Strings.EMPTY_ARRAY : syncedFlushRequest.indices();
+ String endpoint = endpoint(indices, "_flush", "synced");


_flush/synced can now be provided as a single argument, it also won't be encoded this way which is fine as we know that it doesn't need to be encoded.

javanna · 2018-03-22T12:48:36Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java

+ }
+ Map<String, String> expectedParams = new HashMap<>();
+ setRandomIndicesOptions(syncedFlushRequest::indicesOptions, syncedFlushRequest::indicesOptions, expectedParams);
+


can you remove one of these empty lines please?

javanna · 2018-03-22T12:49:02Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java

+ endpoint.add(String.join(",", indices));
+ }
+ endpoint.add("_flush");
+ endpoint.add("synced");


endpoint.add("_flush/synced") ?

javanna · 2018-03-26T12:59:57Z

client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java

+ String[] indices = syncedFlushRequest.indices() == null ? Strings.EMPTY_ARRAY : syncedFlushRequest.indices();
+ String endpoint = endpoint(indices, "_flush", "synced");
+ Params syncedFlushparameters = Params.builder();
+ // This request takes no other parameters other than the indices.


I would remove this comment

javanna · 2018-03-26T13:20:46Z

server/src/main/java/org/elasticsearch/action/admin/indices/flush/SyncedFlushResponse.java

+ if (curToken == Token.FIELD_NAME) {
+ String level2FieldName = parser.currentName();
+ curToken = parser.nextToken();
+ switch (level2FieldName) {


we usually have a switch based on the token found. For instance here we would have one for start_array, and we would do something if the current field name is failures otherwise ignore. And another if token.isValue() for total, successful and failed. You can find something like this for instance in SearchResponse. This makes it easier to reason about what we are parsing I think.

javanna · 2018-03-26T14:09:52Z

server/src/main/java/org/elasticsearch/action/admin/indices/flush/SyncedFlushResponse.java

+ if (failureReason != null &&
+ shardId != null &&
+ totalCopies != null &&
+ successfulCopies != null) {


I think that given our responses, this is always true?

Yes. Same reason as above. I have a general tendency of always expecting illegal JSONs I think. This would mean that if someone changed the toXContent (removing some fields) method tests would start to fail. I will remove it if you think it is not required.

javanna · 2018-03-26T14:57:22Z

server/src/main/java/org/elasticsearch/action/admin/indices/flush/SyncedFlushResponse.java

+ new FailureContainer(shardId, failureReason, totalCopies, successfulCopies, routing)
+ );
+ } else {
+ throw new ParsingException(startLocation, "Unable to construct ShardsSyncedFlushResult");


we try not to throw exception when parsing responses as they may become a problem when it comes to forward compatibility. The client should be able to speak to future versions that have added fields, arrays or objects, by just reading what it knows and ignoring the rest.

Adding fields will not throw this exception. Removing will. IMHO it should because otherwise the response object would get constructed with nulls.

javanna · 2018-03-26T14:58:05Z

server/src/main/java/org/elasticsearch/action/admin/indices/flush/SyncedFlushResponse.java

+ } else {
+ parser.skipChildren();
+ }
+ }


maybe for readability we could split this into a couple of methods for each section / object?

javanna · 2018-03-26T14:58:40Z

server/src/main/java/org/elasticsearch/action/admin/indices/flush/SyncedFlushResponse.java

+ }
+ if (totalShards != null &&
+ successfulShards != null &&
+ failedShards != null) {


when can it happen that some of these is null?

Same reason as above. Just precaution against removing keys from the XContent string in the future. Will remove it if you want.

sohaibiftikhar · 2018-03-26T16:24:38Z

@javanna

On alternative proposals, I am open to ideas, but if this is what we return through REST today and what we print out, I don't follow how using a different object would make things simpler.

I am not sure about simpler. But what it would do is make it logically correct I think. For example consider the method SyncedFlushResponse::getShardsResultPerIndex. Currently this returns all responses successful or not. Since we discard successful responses upon serialization (conversion to XContent String) when we reconstruct this it would only contain responses for failed shards. Hence if you call ::toXContent on a SyncedFlushResponse object and then ::fromXContent on the result obtained it would not give back an equivalent object.

— Split the `toXContent` method into multiple methods

…o synced_flush

— fixed document:integTest

sohaibiftikhar · 2018-05-02T13:31:33Z

@javanna Sorry for the long delay as I was confused on how to proceed. I added the rest of the tests and merged with the latest master. gradle check completed successfully except some xpack stuff that seems unrelated. An issue was created for this already. Could you take a look now?

nik9000 · 2018-05-08T12:45:26Z

OK! So, I vanished for a while because I was at the beach. And then a weekend. And then we had to get a few people together to talk about an approach.

So, there are a bunch of APIs like this: the Response objects just don't match the http response that we make. We use the Response objects elsewhere so we can't change them. The simplest way to actually integrate this into the high level rest client is to make a response object that parses the response from the server. Could you do that? Make a new response object that parses the response from the server and put it in the high level rest client.

I'm really sorry to throw away so much work that you've already done. I suppose it isn't a waste because your work was super important in making it clear that we can't share all of the responses even though we'd really like to.

sohaibiftikhar · 2018-05-08T13:23:03Z

@nik9000 Thanks for the feedback. Since I always get confused with naming could you suggest a name for this new response object?

From my understanding, we would still need some part of what we are doing here (ShardRouting would still need to be parsed). The stuff that would change is that we would not be reconstructing the list of ShardsSyncedFlushResult thing. I will make the required changes once we can decide on a name.

javanna · 2018-05-08T13:32:46Z

@sohaibiftikhar the idea would be to have something that resembles more the json response, and get away from the current response and the heavy object that it holds. SyncedFlushResponse is still good I think, it is going to be in an another package anyway, and renaming it is always possible later if we wish to do so.

nik9000 · 2018-05-08T14:51:53Z

I think I'd make an object to hold whatever we return instead of reusing ShardRouting. I see that we call toXContent on shard routing when we render the response but I'm don't think it is worth trying to rebuild the object in the client. It feels like it should be internal data that we should have never made part of the client in the first place.

sohaibiftikhar · 2018-05-08T15:16:12Z

I see that we call toXContent on shard routing when we render the response but I'm don't think it is worth trying to rebuild the object in the client

@nik9000 I get your point. But we pretty much serialize everything in ShardRouting. And it is not even flat. It serializes the AllocationId and RecoverySource and UnassignedInfo. The only part it leaves out is the SnapshotId::uuid when we have a SnapshotRecoverySource. So I don't understand how discarding deserialization of ShardRouting would help. We would need to replicate all the nested objects. With the only difference being the uuid that I mentioned above.

nik9000 · 2018-05-08T22:04:08Z

I talked to @sohaibiftikhar over another channel and convinced him that we shouldn't reuse ShardRouting in the high level rest client. My argument was mostly that the class and the things that it touches feel internal and we don't really want to make guarantees that we won't change their shape in the future.

sohaibiftikhar · 2018-05-09T08:24:33Z

Since the required changes are more or less entirely different is it okay if I move this to a separate PR?

nik9000 · 2018-05-09T14:42:04Z

Since the required changes are more or less entirely different is it okay if I move this to a separate PR?

Either way is fine with me!

sohaibiftikhar · 2018-05-16T12:39:46Z

Moved to #30650

REST high-level client: add synced flush API

5922216

WIP: Need clarifications

javanna mentioned this pull request Mar 21, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

javanna self-assigned this Mar 21, 2018

javanna added the :Core/Java High Level REST Client label Mar 21, 2018

javanna added the >enhancement label Mar 21, 2018

sohaibiftikhar added 2 commits March 22, 2018 02:32

Added Unit Tests

76bf15b

— Fixed issues with parser for null values — Cleaned up the response parsing for SyncedFlush — TODO: Run complete test suite and check style cleanup

Merge remote-tracking branch 'elastic/master' into synced_flush

1a77355

javanna self-requested a review March 22, 2018 16:52

Merged with master and fixed conflicts

b301ace

nik9000 reviewed Mar 26, 2018

View reviewed changes

javanna reviewed Mar 26, 2018

View reviewed changes

Added integration tests

723300b

— Split the `toXContent` method into multiple methods

sohaibiftikhar force-pushed the synced_flush branch from 349f7b3 to 723300b Compare April 30, 2018 21:11

Merge branch 'master' of https:/elastic/elasticsearch int…

3d7f978

…o synced_flush

sohaibiftikhar force-pushed the synced_flush branch from 85bdde4 to 0543779 Compare May 1, 2018 22:43

Fixes after merging with master

0bf9dcf

— fixed document:integTest

sohaibiftikhar force-pushed the synced_flush branch from 0543779 to 0bf9dcf Compare May 2, 2018 12:35

sohaibiftikhar changed the title ~~[WIP] REST high-level client: add synced flush API~~ REST high-level client: add synced flush API May 2, 2018

sohaibiftikhar mentioned this pull request May 16, 2018

REST high-level client: add synced flush API (2) #30650

Merged

sohaibiftikhar closed this May 16, 2018

sohaibiftikhar deleted the synced_flush branch June 1, 2018 09:15

REST high-level client: add synced flush API #29189

REST high-level client: add synced flush API #29189

Conversation

sohaibiftikhar commented Mar 21, 2018 • edited Loading

elasticmachine commented Mar 21, 2018

elasticmachine commented Mar 21, 2018

sohaibiftikhar commented Mar 21, 2018 • edited Loading

elasticmachine commented Mar 21, 2018

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohaibiftikhar Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohaibiftikhar Mar 26, 2018 • edited Loading

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

sohaibiftikhar commented Mar 26, 2018 • edited Loading

sohaibiftikhar commented May 2, 2018 • edited Loading

nik9000 commented May 8, 2018

sohaibiftikhar commented May 8, 2018

javanna commented May 8, 2018

nik9000 commented May 8, 2018

sohaibiftikhar commented May 8, 2018 • edited Loading

nik9000 commented May 8, 2018

sohaibiftikhar commented May 9, 2018

nik9000 commented May 9, 2018

sohaibiftikhar commented May 16, 2018

sohaibiftikhar commented Mar 21, 2018 •

edited

Loading

sohaibiftikhar commented Mar 21, 2018 •

edited

Loading

sohaibiftikhar Mar 26, 2018 •

edited

Loading

sohaibiftikhar Mar 26, 2018 •

edited

Loading

sohaibiftikhar commented Mar 26, 2018 •

edited

Loading

sohaibiftikhar commented May 2, 2018 •

edited

Loading

sohaibiftikhar commented May 8, 2018 •

edited

Loading