Add GetRollupCaps API to high level rest client by polyfractal · Pull Request #32880 · elastic/elasticsearch

polyfractal · 2018-08-15T17:34:12Z

This is based on @tlrx's branch in #32703, so a review should probably hold off until that PR is merged so that this diff is a little cleaner.

The GetRollupIndexCaps API uses basically the same serialization of objects with a slightly different endpoint, but this PR was getting big enough already so I'll do that one in a followup.

Related #29827

…//github.com/tlrx/elasticsearch into tlrx-add-create-rollup-job-api-to-high-level-client

Related elastic#29827

hub-cap · 2018-08-27T18:08:18Z

hi @polyfractal, We decided to split the request and response from the ones used in server/x-pack, so they do not overlap. Please update your PR such that you create new request implements Validatable & generic response class in the high level rest client's org.elasticsearch.client.nameOfYourClient. Example: o.e.client.watcher, o.e.client.cluster. Please revert any work you did to modify the existing request/response in x-pack or server.

tlrx · 2018-09-21T07:00:32Z

@polyfractal Is it ready to review?

polyfractal · 2018-09-21T11:30:21Z

@tlrx sorry, not quite yet. After pushing I realized the client versions of the clases still extend Writeable when they don't need too. Removed those and need to refactor a few tests because of it.

I'm off today but will finish it up first thing on Monday :)

No need to extend ActionRequest, Writeable, etc

Also pulls in the rollup cleaning from elastic#33921

polyfractal · 2018-09-25T13:18:09Z

@tlrx Ok, think this is ready for a look now. Sorry for the delay, was fighting tests :)

tlrx

I left a first bunch of comments, mostly cosmetics

tlrx · 2018-09-26T12:33:16Z

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

+    }
+
+    /**
+     * Asynchronously put a rollup job into the cluster


Nit: doc not up to date

tlrx · 2018-09-26T12:34:03Z

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

+import java.util.Optional;
+
+public class GetRollupCapsRequest implements Validatable, ToXContentObject {
+    private String indexPattern;


Can be final too

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

tlrx · 2018-09-26T12:35:43Z

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

+    }
+
+    @Override
+    public Optional<ValidationException> validate() {


You don't need this, this is the default implementation

tlrx · 2018-09-26T12:36:48Z

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

+
+public class GetRollupCapsResponse implements ToXContentObject {
+
+    private Map<String, RollableIndexCaps> jobs = Collections.emptyMap();


Could be final

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

tlrx · 2018-09-26T12:54:38Z

...t-high-level/src/test/java/org/elasticsearch/client/documentation/RollupDocumentationIT.java

+    public void testGetRollupCaps() throws Exception {
+        RestHighLevelClient client = highLevelClient();
+
+


...t-high-level/src/test/java/org/elasticsearch/client/documentation/RollupDocumentationIT.java

tlrx · 2018-09-26T12:58:37Z

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

+
+}
+
+


extra lines

polyfractal · 2018-09-27T14:17:36Z

Tidied up the various style issues, and applied similar corrections to some of the other classes :)

hub-cap · 2018-10-02T14:37:46Z

this PR is way too big for me to review. if @tlrx is happy, then im happy. Im just going to check the high level rest items and skip all the pojos. next time it would be rad to separate all the pojos from the hlrc work.

hub-cap

based on the size of this, it might make sense to break it up into > 1 PR. I cant approve it as is, because its too much to review. If @tlrx is happy, then thats good enuf for me!

hub-cap · 2018-10-02T14:38:44Z

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

+            RollupRequestConverters::getRollupCaps,
+            options,
+            GetRollupCapsResponse::fromXContent,
+            Collections.emptySet());


any reason these two methods have drastically different indentation? one of them is a newline every param and the Async below is just 2 lines.

hub-cap · 2018-10-02T14:40:18Z

docs/java-rest/high-level/x-pack/rollup/put_job.asciidoc

@@ -0,0 +1,172 @@
+[[java-rest-high-x-pack-rollup-put-job]]


whats up this file? is it meant to be added?

hub-cap

based on the size of this, it might make sense to break it up into > 1 PR. I cant approve it as is, because its too much to review. If @tlrx is happy, then thats good enuf for me!

tlrx

LGTM - I left some minor comments that can be addressed without another review

There still two important things: the Put Rollup documentation that does not belong to this PR and a doc change to make it compatible with #34125

tlrx · 2018-10-05T07:43:15Z

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

        return request;
    }
+
+    static Request getRollupCaps(GetRollupCapsRequest getRollupCapsRequest) throws IOException {


getRollupCapsRequest can be final

tlrx · 2018-10-05T07:47:03Z

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

+            return false;
+        }
+        GetRollupCapsResponse other = (GetRollupCapsResponse) obj;
+        return Objects.equals(jobs, other.jobs);


Oh right sorry

tlrx · 2018-10-05T07:48:57Z

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

+
+    RollableIndexCaps(final String indexName, final List<RollupJobCaps> caps) {
+        this.indexName = indexName;
+        this.jobCaps = Collections.unmodifiableList(Objects.requireNonNull(caps)


Is it sorted for XContent rendering purpose? If so maybe it could be done in the toXContent() method instead of the constructor?

It is... but isn't it better to sort once in the ctor instead of each time toXContent is called? Since jobcaps is final and unmodifiable, we know it will only happen once and won't ever become unsorted.

(also note iirc it's mainly there to make testing from yaml rest tests easier)

Ok, let's keep the sorting in the ctor. I wanted to avoid the sort to be done each time the object is instantiated but this is a HLRC specific object so it will be done just one time

tlrx · 2018-10-05T07:51:27Z

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

+                }
+            }
+        }
+        return new GetRollupCapsResponse(Collections.unmodifiableMap(jobs));


I don't think you need to make it unmodifiable here if it's done in the constructor

...t-high-level/src/test/java/org/elasticsearch/client/documentation/RollupDocumentationIT.java

tlrx · 2018-10-05T07:54:49Z

...t-high-level/src/test/java/org/elasticsearch/client/documentation/RollupDocumentationIT.java

+        deleteRollupJobs();
+        waitForPendingRollupTasks();
+    }
+    private void deleteRollupJobs() throws Exception {


nit: missing new line

tlrx · 2018-10-05T07:57:39Z

docs/java-rest/high-level/rollup/get_rollup_caps.asciidoc

+
+["source","java",subs="attributes,callouts,macros"]
+--------------------------------------------------
+include-tagged::{doc-tests}/RollupDocumentationIT.java[x-pack-rollup-get-rollup-caps-request]


You'll have to use Nik's improvement here - it avoids to copy paste the execution snippets for the HLRC doc.

tlrx · 2018-10-05T07:59:26Z

.../plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/action/RollableIndexCaps.java

+    public RollableIndexCaps(String indexName, List<RollupJobCaps> caps) {
        this.indexName = indexName;
-        this.jobCaps = new ArrayList<>();
+        this.jobCaps = Collections.unmodifiableList(Objects.requireNonNull(caps)


Do we need to sort here or maybe it can be done in the ToXContent() method?

polyfractal · 2018-10-15T15:40:23Z

Will continue the style fixes, and see if it can be split into >1 PR... I suspect that's going to be hard.

The current structure of the client means we have to do a lot of semi-duplication, and a lot of moving things around, just to make the client bits work. E.g. all the request/response objects need to be sorta duplicated, minus the bits that are only used internal to the server. In the case of the Caps API, that's a lot of objects since it's a deeply-nested response object. Plus their associated response parsers, which aren't used on the Server side.

There are some smaller changes I could probably make to Rollup in isolation (some changes to make a few objects immutable) but honestly I don't think that will help much.

Re: docs... I meant to move the PutRollupJob docs, not sure why it copied instead. The organization of the docs appears to be under /java-rest/high-level/<plugin>/. But it appears that PutJob is under /java-rest/high-level/x-pack/rollup/... I'm assuming that was from before the recent HLRC restructuring. In any case, it seems like the x-pack version should be deleted and moved to the other location to follow how other plugins are doing it (ML, etc)

hub-cap · 2018-10-15T18:57:52Z

RE docs: it should not have an x-pack folder

RE splitting, dont worry about it. if @tlrx is happy, so am I. The high points of the HLRC are ok, and assuming you fix the docs as he pointed out,

tlrx · 2018-10-16T13:44:07Z

@polyfractal Once docs and the few style issues fixed I'll be happy to review this one more time

polyfractal · 2018-10-16T19:28:13Z

Jenkins test this please

tlrx

LGTM, left minor comments. If CI is happy I'm happy

tlrx · 2018-10-17T07:52:32Z

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

+            }
+
+            RollupFieldCaps that = (RollupFieldCaps) other;
+            return Objects.deepEquals(this.aggs, that.aggs);


You can use Objects.equals() here

tlrx · 2018-10-17T07:52:56Z

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

+        }
+
+        RollableIndexCaps that = (RollableIndexCaps) other;
+        return Objects.deepEquals(this.jobCaps, that.jobCaps)


You can use Objects.equals() here

tlrx · 2018-10-17T07:55:50Z

.../plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/action/RollableIndexCaps.java


-        return Objects.equals(this.jobCaps, that.jobCaps)
-                && Objects.equals(this.indexName, that.indexName);
+        return Objects.deepEquals(this.jobCaps, that.jobCaps)


You can use Objects.equals() here

tlrx · 2018-10-19T07:07:38Z

Thanks @polyfractal 🎉

Adds GetRollupCaps API to the HLRC, and tweaks some of the Caps objects to be immutable. Also various style tweaks

tlrx and others added 5 commits August 13, 2018 19:28

Add Create Rollup Job API to the high level rest client

38fca66

Fix license

7667ed9

Add Cron validation

2f340dd

Merge branch 'add-create-rollup-job-api-to-high-level-client' of git:…

dfb0b14

…//github.com/tlrx/elasticsearch into tlrx-add-create-rollup-job-api-to-high-level-client

Add GetRollupCaps API to the High Level Rest Client

310e690

Related elastic#29827

$@polyfractal$ polyfractal added >enhancement review v7.0.0 v6.5.0 labels Aug 15, 2018

$@polyfractal$ polyfractal requested review from hub-cap and tlrx August 15, 2018 17:34

elasticmachine mentioned this pull request Aug 15, 2018

Java high-level REST client completeness for xpack APIs #29827

Closed

98 tasks

Zachary Tong added 2 commits September 19, 2018 13:32

Merge remote-tracking branch 'origin/master' into hlrc_rollup_get_caps

ff85234

HLRC Refactor

a38379a

Zachary Tong added 3 commits September 24, 2018 10:56

Tweak request/response objects

19cfc2c

No need to extend ActionRequest, Writeable, etc

Fix toXContent serialization of request

7754803

Also pulls in the rollup cleaning from elastic#33921

Merge remote-tracking branch 'origin/master' into hlrc_rollup_get_caps

89ab8a9

tlrx reviewed Sep 26, 2018

View reviewed changes

Zachary Tong added 3 commits September 26, 2018 15:06

Review cleanup

157db4a

Empty lines

6dd3fd0

checkstyle

dda7039

hub-cap reviewed Oct 2, 2018

View reviewed changes

tlrx approved these changes Oct 5, 2018

View reviewed changes

rjernst removed the review label Oct 10, 2018

Merge remote-tracking branch 'origin/master' into hlrc_rollup_get_caps

5edd406

Zachary Tong added 3 commits October 16, 2018 13:59

Review Cleanup

0dac0d0

Merge remote-tracking branch 'origin/master' into hlrc_rollup_get_caps

98b0585

Doc tweaks

39fb7dd

tlrx approved these changes Oct 17, 2018

View reviewed changes

Zachary Tong added 3 commits October 17, 2018 11:29

Merge remote-tracking branch 'origin/master' into hlrc_rollup_get_caps

9613d19

Unecessary deepEquals

0716c29

Merge remote-tracking branch 'origin/master' into hlrc_rollup_get_caps

a74dc3e

$@polyfractal$ polyfractal merged commit 45546e7 into elastic:master Oct 18, 2018

polyfractal pushed a commit to polyfractal/elasticsearch that referenced this pull request Oct 19, 2018

Add GetRollupCaps API to high level rest client (elastic#32880)

17b78be

Adds GetRollupCaps API to the HLRC, and tweaks some of the Caps objects to be immutable. Also various style tweaks

$@polyfractal$ polyfractal mentioned this pull request Oct 19, 2018

Add GetRollupCaps to HLRC (6.x Backport) #34647

Merged

polyfractal pushed a commit that referenced this pull request Oct 19, 2018

Add GetRollupCaps API to high level rest client (#32880) (#34647)

c6cd502

Adds GetRollupCaps API to the HLRC, and tweaks some of the Caps objects to be immutable. Also various style tweaks

kcm pushed a commit that referenced this pull request Oct 30, 2018

Add GetRollupCaps API to high level rest client (#32880)

4176ae7

Adds GetRollupCaps API to the HLRC, and tweaks some of the Caps objects to be immutable. Also various style tweaks

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


		public class GetRollupCapsResponse implements ToXContentObject {

		private Map<String, RollableIndexCaps> jobs = Collections.emptyMap();

		public void testGetRollupCaps() throws Exception {
		RestHighLevelClient client = highLevelClient();

Conversation

polyfractal commented Aug 15, 2018

Uh oh!

hub-cap commented Aug 27, 2018

Uh oh!

tlrx commented Sep 21, 2018

Uh oh!

polyfractal commented Sep 21, 2018

Uh oh!

polyfractal commented Sep 25, 2018

Uh oh!

tlrx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polyfractal commented Sep 27, 2018

Uh oh!

hub-cap commented Oct 2, 2018

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

tlrx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polyfractal commented Oct 15, 2018

Uh oh!

hub-cap commented Oct 15, 2018

Uh oh!

tlrx commented Oct 16, 2018

Uh oh!

polyfractal commented Oct 16, 2018

Uh oh!

tlrx left a comment

Choose a reason for hiding this comment

Uh oh!

$@polyfractal$ polyfractal commented Aug 15, 2018