REST high-level client: add support for Rollover Index API #28698

javanna · 2018-02-15T23:06:37Z

Relates to #27205

…ded given that a CreateIndexRequest is internally created and can then be retrieved to set settings, mappings and aliases to it.

cbuescher · 2018-02-19T10:41:25Z

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

+
+        highLevelClient().index(new IndexRequest("test", "type", "1").source("field", "value"));
+        highLevelClient().index(new IndexRequest("test", "type", "2").source("field", "value")
+                .setRefreshPolicy(WriteRequest.RefreshPolicy.WAIT_UNTIL));


Just curious why we need the refresh policy here. Maybe worth a comment.

The rollover API doesn't see the proper number of docs otherwise. At least that's why I think the test fails without it.

Can you add this explanation as a short comment then? Thanks.

cbuescher

@javanna I did a first round of review, left a couple of questions and small comments, but this looks like it's ready soon

cbuescher · 2018-02-19T11:03:30Z

docs/java-rest/high-level/indices/rollover.asciidoc

+<1> Timeout to wait for the all the nodes to acknowledge the index is opened
+as a `TimeValue`
+<2> Timeout to wait for the all the nodes to acknowledge the index is opened
+as a `String`


Wondering if it makes sense to add a note somewhere that these are alternatives. Maybe not needed though.

good one, we did it this way in every single docs page though, maybe if we want to change this we should do it as part of a different PR. Not sure, but I see why you ask this question.

Doesn't need to be in this PR, it just occured to me when reading this docs page. Probably also not a big deal, for most people it should be obvious that these (and similar cases) contain two options

cbuescher · 2018-02-19T11:13:33Z

server/src/main/java/org/elasticsearch/action/admin/indices/create/CreateIndexResponse.java

@@ -37,9 +37,8 @@
 /**
 * A response for a create index action.
 */
-public class CreateIndexResponse extends AcknowledgedResponse implements ToXContentObject {
+public class CreateIndexResponse extends ShardsAcknowledgedResponse implements ToXContentObject {


I have some compile problems after merging in current master that might be related to this. Can you rebase/merge to check before merging?

I do not see these, let me know where you see problems.

updated my workspace, gone now. Sorry for the noise.

cbuescher · 2018-02-19T11:21:55Z

server/src/main/java/org/elasticsearch/action/admin/indices/create/CreateIndexResponse.java

-        declareAcknowledgedField(objectParser);
-        objectParser.declareField(constructorArg(), (parser, context) -> parser.booleanValue(), SHARDS_ACKNOWLEDGED,
-                ObjectParser.ValueType.BOOLEAN);
+        declareAcknowledgedAndShardsAcknowledgedFields(objectParser);
        objectParser.declareField(constructorArg(), (parser, context) -> parser.text(), INDEX, ObjectParser.ValueType.STRING);


Unrelated to this PR, but can the "index" field be null? Looking at toXContent this can theoretically be the case, not sure if this is guarded against somewhere in CreateIndexResponse, but I couldn't find anything on a quick look.

good one, I didn't see this, it should not happen as we never parse something that was serialized through the wire, and it can only be null before 5.6 where the high-level client didn't even exist yet. But the code allows for this so I am fixing and adding a test for it.

cbuescher · 2018-02-19T11:27:07Z

server/src/main/java/org/elasticsearch/action/admin/indices/rollover/Condition.java

+            return false;
+        }
+        Condition<?> condition = (Condition<?>) o;
+        return Objects.equals(name, condition.name);


Should condition equals() ignore the value? If so, why?

yes. it was not ignored before but it's counter intuitive. Json doesn't allow for duplicated keys with same name, which is the only way you could end up with two conditions with same name. These conditions end up in a set, which is why I think it is important to properly implement equals and hashcode although before it was not.

I see, but it feels a bit strange. Maybe the way it is used in the set is a bit off then? Probably outside the scope of this PR though, but can you add this explanation as a javadoc comment to the equals method?

It is within the scope of this PR as I introduced these methods as part of this PR :) I am trying to think of how to improve this and make it less strange. Maybe having a set would not not be that important if things only came from the REST layer, but given that we are setting it also through transport client and high-level REST client, I think we need to make sure that a condition can only be set once. Would you prefer to have a Map<String, Condition> where the key is the name, rather than a Set and implement equals/hashcode this way?

+1, that would probably make things clearer. The set might not have been the best choice given we only want to assure each condition type is set once, as far as I understand it.

cbuescher · 2018-02-19T11:30:04Z

server/src/main/java/org/elasticsearch/action/admin/indices/rollover/Condition.java

+                return false;
+            }
+            Result result = (Result) o;
+            return Objects.equals(condition, result.condition);


Should "matched" be ignored in equal()? If so, why?

Yes I think it should, whether a condition matched or not does not matter to identify which condition it is?

Same here, I see now why it doesn't matter for how we use the object, but if just looking at the object alone one could think it is part of the object. I think it would also be good to add this explanation as a javadoc comment to the equals method if we leave it like this.

cbuescher · 2018-02-19T12:14:52Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestShrinkIndexAction.java

-        }
-        if (request.param("index") == null) {
-            throw new IllegalArgumentException("no source index");
-        }


Is this some sort of cleanup?

yes that can't happen already as this is validated at the response level , no need to do it at the rest action level too.

cbuescher · 2018-02-19T12:28:20Z

server/src/test/java/org/elasticsearch/action/admin/indices/rollover/RolloverRequestTests.java

+            rolloverRequest.getCreateIndexRequest().settings(RandomCreateIndexGenerator.randomIndexSettings());
+        }
+        int numConditions = randomIntBetween(0, 3);
+        Set<Consumer<RolloverRequest>> consumers = new HashSet<>();


While adding the random conditions here seems to work, it took me a while to understand the setup. Maybe this can be simlified a bit by e.g. sampling a random subset from the three options and then using a switch or something? Maybe less elegant but more readable?

right, I could have used randomSubsetOf and made things much simpler. Will do that.

cbuescher · 2018-02-19T12:34:37Z

server/src/test/java/org/elasticsearch/action/admin/indices/rollover/RolloverRequestTests.java

+        expectThrows(ParsingException.class, () -> request.fromXContent(createParser(xContentType.xContent(), mutated)));
+    }
+
+    public void testAddConditions() {


Maybe add a comment that this test is about failing when calling addXYZ twice, or rename the test method to reflect that better

I am renaming to testSameConditionCanOnlyBeAddedOnce

cbuescher · 2018-02-19T12:35:35Z

server/src/test/java/org/elasticsearch/action/admin/indices/rollover/RolloverRequestTests.java

+        RolloverRequest rolloverRequest = new RolloverRequest();
+        assertNotNull(rolloverRequest.getCreateIndexRequest());
+        ActionRequestValidationException validationException = rolloverRequest.validate();
+        assertNotNull(validationException);


Maybe add a message explaining why this should not be null

isn't it self-explanatory given that we retrieve the expected error in the following lines?

Agreed, one can read it like that.

cbuescher · 2018-02-19T12:38:38Z

server/src/test/java/org/elasticsearch/action/admin/indices/rollover/RolloverResponseTests.java

+import java.util.HashSet;
+import java.util.Set;
+
+public class RolloverResponseTests extends ESTestCase {


Should we also add serialization roundtrip tests to this new test class?

yes it would be nice but I think it's besides the scope of this PR. I can add it but I must admit we didn't do it before for other responses and adding that may require adding equals/hashcode methods plus testing them etc. which becomes too time-consuming. There are enough tests added around the high-level REST client stuff already I think, and the main goal here is adding support for new APIs to the high-level REST client. If some important test is missing we should open separate issues for them and address them separately. What do you think?

Np, was also more of a "nice to have" since we're introducing this new test here. Would you open a separate issue to improbe this at some point though?

I couldn't help it, I didn't want to leave yet another issue behind, not knowing who and when would take care of it. I may go back and also look at some other response tests, given that implement a different base test class gives us almost all we need already.

javanna · 2018-02-19T14:15:18Z

@cbuescher I addressed your comments / replied to your questions.

cbuescher

@javanna thanks for the changes, I'm also agreeing with most of your comments. Left a few small requests to add comments or follow up PRs but other than that LGTM

…which saves some lines of code

cbuescher · 2018-02-20T08:51:17Z

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

@@ -463,6 +463,7 @@ public void testRollover() throws IOException {
        highLevelClient().index(new IndexRequest("test", "type", "1").source("field", "value"));
        highLevelClient().index(new IndexRequest("test", "type", "2").source("field", "value")
                .setRefreshPolicy(WriteRequest.RefreshPolicy.WAIT_UNTIL));
+        //without the refresh the rollover may not happen as the number of docs seen may be off


cbuescher · 2018-02-20T09:13:34Z

server/src/test/java/org/elasticsearch/action/admin/indices/rollover/RolloverResponseTests.java

+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+public class RolloverResponseTests extends AbstractStreamableXContentTestCase<RolloverResponse> {


Probably it would be worth implementing AbstractStreamableTestCase#getMutateFunction in this case, so that the equals/hashCode tests are more complete. I think without it a part of them is skipped.

cbuescher · 2018-02-20T09:16:28Z

@javanna I left a few more comments regarding the latest changes, nothing to hold back the PR though.

javanna · 2018-02-20T10:11:11Z

thanks a lot for your review @cbuescher

Relates to #27205

* es/6.x: (140 commits) Remove AcknowledgedRestListener in favour of RestToXContentListener (#28724) Migrate some *ResponseTests to AbstractStreamableXContentTestCase (#28749) Revert "Disable BWC tests for build issues" Skip some plugins service tests on Windows Build: Consolidate archives and packages configuration (#28760) Ensure that azure stream has socket privileges (#28773) Disable BWC tests for build issues Pass InputStream when creating XContent parser (#28754) [DOCS] Fixed broken link. [DOCS] Changed to use transient setting to reenabled allocation. Closes #27677 Delay path expansion on Windows [TEST] replace randomAsciiAlphanumOfLengthBetween with randomAsciiLettersOfLengthBetween REST high-level client: add support for Rollover Index API (#28698) [Tests] Extract the testing logic for Google Cloud Storage (#28576) Moved Grok helper code to a separate Gradle module and let ingest-common module depend on it. version set in ingest pipeline (#27573) [Docs] Update links to java9 docs (#28750) Revert "Add startup logging for standalone tests" Tests: don't wait for completion while trying to get completed task Add 5.6.9 snapshot version ...

…8698) Relates to elastic#27205

REST high-level client: add support for Rollover Index API

c25f268

Relates to elastic#27205

javanna added >enhancement v7.0.0 :Core/Java High Level REST Client v6.3.0 labels Feb 15, 2018

javanna requested a review from tlrx February 15, 2018 23:06

javanna mentioned this pull request Feb 15, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

javanna requested a review from cbuescher February 16, 2018 10:03

javanna added 2 commits February 16, 2018 11:41

fix test failure

5704be0

remove the setCreateIndexrRequest from RolloverRequest. It is not nee…

58513c2

…ded given that a CreateIndexRequest is internally created and can then be retrieved to set settings, mappings and aliases to it.

cbuescher self-assigned this Feb 16, 2018

fix test failure

955a6cf

cbuescher reviewed Feb 19, 2018

View reviewed changes

cbuescher requested changes Feb 19, 2018

View reviewed changes

javanna added 4 commits February 19, 2018 14:01

Merge branch 'master' into enhancement/rest_hl_client_rollover

6b66c81

fix create index response parsing and add test

eb4931d

address review comments

07a6fe7

address review comments

07a6361

cbuescher approved these changes Feb 19, 2018

View reviewed changes

javanna added 4 commits February 19, 2018 17:29

add serialization tests for RolloverResponse, also change base class …

7a55875

…which saves some lines of code

adapt Condition equals/hashcode to take value into account

97f7f10

Merge branch 'master' into enhancement/rest_hl_client_rollover

5e81283

shorten serialization code

d188e54

cbuescher reviewed Feb 20, 2018

View reviewed changes

shorten serialization code

ee0141e

cbuescher reviewed Feb 20, 2018

View reviewed changes

add mutate function

4e74af3

adjust rollover response tests

9b7a4de

javanna merged commit 8bbb3c9 into elastic:master Feb 20, 2018

javanna added the backport pending label Feb 20, 2018

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

REST high-level client: add support for Rollover Index API (#28698)

967d88f

Relates to #27205

javanna removed the backport pending label Feb 21, 2018

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

REST high-level client: add support for Rollover Index API (elastic#2…

93790a0

…8698) Relates to elastic#27205

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

REST high-level client: add support for Rollover Index API #28698

REST high-level client: add support for Rollover Index API #28698

Conversation

javanna commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher 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

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

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

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

javanna commented Feb 19, 2018

cbuescher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher commented Feb 20, 2018

javanna commented Feb 20, 2018