HLRC: Add get rollup job by nik9000 · Pull Request #33921 · elastic/elasticsearch

nik9000 · 2018-09-20T19:53:27Z

Adds support for the get rollup job to the High Level REST Client. I had
to do two interesting and unexpected things:

I ported the rollup state wiping code into the high level client
tests. I'll move this into the test framework in a followup and remove
the x-pack version.
The timeout in the rollup config was serialized using the
toString representation of TimeValue which produces fractional time
values which are more human readable but aren't supported by parsing. So
I switched it to getStringRep.

Adds support for the get rollup job to the High Level REST Client. I had to do two interesting and unexpected things: 1. I ported the rollup state wiping code into the high level client tests. I'll move this into the test framework in a followup and remove the x-pack version. 2. The `timeout` in the rollup config was serialized using the `toString` representation of `TimeValue` which produces fractional time values which are more human readable but aren't supported by parsing. So I switched it to `getStringRep`.

elasticmachine · 2018-09-20T19:53:29Z

Pinging @elastic/es-core-infra

hub-cap

Just the little bit about the test and toXContent.

hub-cap · 2018-09-21T18:46:03Z

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

+     * @param jobId id of the job to return or {@code _all} to return all jobs
+     */
+    public GetRollupJobRequest(final String jobId) {
+        this.jobId = Objects.requireNonNull(jobId, "jobId is required");


<3 validation here

The server-side request (and REST endpoint) converts null, empty or * into _all. So we should probably do the same here, or remove that from the server-side request and add the logic to the transport action.

Otherwise an asterisk from the HLRC will go unconverted and the server will try to look for a job literally named * :)

It looks like REST doesn't support null but the transport client does. Or maybe I'm reading it wrong. I'm not sure we need all of that because we have a "normal" way to query for all.

We could totally do a default constructor that sets it to _all and ensure this one does not do that behavior w the null check

hub-cap · 2018-09-21T18:47:30Z

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

+     * is also persistent when changing from started/stopped in case the allocated
+     * task is restarted elsewhere.
+     */
+    public enum IndexerState {


is this only valid in the context of a rollup job response? if not, can u make it so others can use it by making its own class.

I believe it is only valid for this, yes.

++ This is correct, atm only exposed to the user via the GetJob API

hub-cap · 2018-09-21T18:49:46Z

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

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


hey, these toXContent are not needed in responses. They help with the tests but you can manually create a parser for now, as I have to think about how to better do these tests without putting the toXContent logic in the classes themselves.

KK. I'll pull.

Doesn't this make testing a lot harder than just implementing a manual parser? E.g. we can't use AbstractXContentTestCase anymore, which does all the xcontent shuffling serializing/reparsing/equivalence checks?

Requires a refactor to AbstractXContentTestCase

Also pulls in the rollup cleaning from elastic#33921

polyfractal · 2018-09-26T19:31:14Z

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

+        assertEquals(putRollupJobRequest.getConfig(), job.getJob());
+        assertThat(job.getStats().getNumPages(), lessThan(10L));
+        assertEquals(numDocs, job.getStats().getNumDocuments());
+        assertThat(job.getStats().getNumInvocations(), lessThan(10L));


Hmm, i wonder if this is potentially flaky? E.g. the job is configured to trigger every second, so if it takes longer than 10 seconds to get to this point for whatever reason, the assertion might fail.

I think the other assertions are fine since they should be deterministic (number of docs indexed, number of pages, etc)

Got it. I hadn't realized it'd count time when it didn't do any work. I'll just assert >0.

Yeah, it's not necessarily time... but it increments each time the indexer is "triggered", even if nothing is done after the trigger. Handy way to help debug if the indexer is "doing things" even if no documents are being emitted for some reason.

polyfractal · 2018-09-26T19:32:04Z

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

+        assertEquals(numDocs, job.getStats().getNumDocuments());
+        assertThat(job.getStats().getNumInvocations(), lessThan(10L));
+        assertEquals(1, job.getStats().getOutputDocuments());
+        assertEquals(IndexerState.STARTED, job.getStatus().getState());


Probably need to check for either STARTED or INDEXING since you could catch the job right after it triggers but before it realizes there's no more data to index

polyfractal · 2018-09-26T19:40:32Z

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

+
+    public void testGetJob() {
+        boolean getAll = randomBoolean();
+        String job = getAll ? "_all" : RequestConvertersTests.randomIndicesNames(1, 1)[0];


Related to above comment about null/empty/asterisk... if that changes we should probably include those as options here too

polyfractal · 2018-09-26T19:44:09Z

Left a few minor comments/questions about the rollup'y bits :)

hub-cap

ty, i think i will start showing the response test to people as the proper way to do it :)

nik9000 · 2018-09-28T19:05:15Z

ty, i think i will start showing the response test to people as the proper way to do it :)

❤️

Maybe we can build something simpler to use once we have a few examples using it?

Adds support for the get rollup job to the High Level REST Client. I had to do three interesting and unexpected things: 1. I ported the rollup state wiping code into the high level client tests. I'll move this into the test framework in a followup and remove the x-pack version. 2. The `timeout` in the rollup config was serialized using the `toString` representation of `TimeValue` which produces fractional time values which are more human readable but aren't supported by parsing. So I switched it to `getStringRep`. 3. Refactor the xcontent round trip testing utilities so we can test parsing of classes that don't implements `ToXContent`.

nik9000 added 2 commits September 20, 2018 14:01

Add test for missing and converter

08fa5ef

nik9000 added >enhancement v7.0.0 :Core/Java High Level REST Client v6.5.0 labels Sep 20, 2018

nik9000 requested a review from polyfractal September 20, 2018 19:53

nik9000 requested a review from hub-cap September 20, 2018 19:53

elasticmachine mentioned this pull request Sep 20, 2018

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

Closed

98 tasks

hub-cap suggested changes Sep 21, 2018

View reviewed changes

nik9000 added 2 commits September 24, 2018 10:53

Merge branch 'master' into hlrc_rollup_get

81015df

Move toXContent to tests

0418a81

Requires a refactor to AbstractXContentTestCase

polyfractal pushed a commit to polyfractal/elasticsearch that referenced this pull request Sep 24, 2018

Fix toXContent serialization of request

7754803

Also pulls in the rollup cleaning from elastic#33921

$polyfractal$

polyfractal reviewed Sep 26, 2018

View reviewed changes

nik9000 added 3 commits September 27, 2018 17:28

Merge branch 'master' into hlrc_rollup_get

baef527

Fix

d542da4

Merge branch 'master' into hlrc_rollup_get

eb90d8a

hub-cap approved these changes Sep 28, 2018

View reviewed changes

hub-cap mentioned this pull request Sep 28, 2018

HLRC: add support for get license basic/trial status API #33176

Merged

Fixup

a92d354

nik9000 added 2 commits October 1, 2018 10:07

Merge branch 'master' into hlrc_rollup_get

fdfa4c4

No _all

55bfb19

nik9000 merged commit f904c41 into elastic:master Oct 2, 2018

nik9000 added the backport pending label Oct 2, 2018

nik9000 removed the backport pending label Oct 2, 2018

hub-cap mentioned this pull request Oct 3, 2018

Add document _count API support to Rest High Level Client. #34267

Merged

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

Conversation

nik9000 commented Sep 20, 2018

Uh oh!

elasticmachine commented Sep 20, 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!

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!

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!

polyfractal commented Sep 26, 2018

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Sep 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants