Add delete rollup job support to HL REST Client by pcsanwald · Pull Request #34066 · elastic/elasticsearch

pcsanwald · 2018-09-25T22:41:18Z

Related to #29827, add support for deletion of a job to HLRC. A few notes on the PR:

refactored to use AcknowledgedResponse to remove a small bit of duplication

elasticmachine · 2018-09-25T22:41:19Z

Pinging @elastic/es-core-infra

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

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

iverase

In general looks good. Maybe we should add this action to the IT tests:

org.elasticsearch.client.RollupIT

WDYT?

pcsanwald · 2018-09-28T16:34:29Z

@iverase good call, I'll add a test in RollupIT

tlrx

didn't add support for a synchronous call intentionally, from digging around there is no way to block on this

Can you expand on what is the problem exactly? Thanks!

Otherwise I left some minor comments, it looks great

tlrx · 2018-10-05T10:02:17Z

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

    }
+
+    /**
+     * delete a rollup job from the cluster


Nit: delete -> Delete

tlrx · 2018-10-05T10:03:45Z

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

+
+    private final String id;
+
+    private static final ParseField ID_FIELD = new ParseField("id");


Nit: We usually place static constants at the beginning of the class, then class members

tlrx · 2018-10-05T10:04:33Z

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

+    private static final ParseField ID_FIELD = new ParseField("id");
+
+    public DeleteRollupJobRequest(String id) {
+        this.id = Objects.requireNonNull(id,"id parameter must not be null");


Nit: missing space after ,

tlrx · 2018-10-05T10:08:56Z

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

+        return PARSER.parse(parser, null);
+    }
+
+    private static final ConstructingObjectParser<DeleteRollupJobResponse, Void> PARSER


I think we could move the declaration of the PARSER into the AcknowledgedResponse class so that we don't have to repeat the parsing logic in DeleteRollupJobResponse and PutRollupJobResponse?

originally I tried to do this, but it's a static field and I couldn't work out a way to pass the concrete type without using some kind of builder thing, which felt like overkill for a small amount of duplication. Am I missing an easy way to do this?

I was thinking of something like declareInnerHitsParseFields or declareAggregationFields (in ParsedAggregation). But I agree this is a bit overkill :)

tlrx · 2018-10-05T10:09:47Z

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

+        } catch (Exception e) {
+            // Swallow any exception, this test does not test actually cancelling.
+        }
+


extra spaces

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

tlrx · 2018-10-05T10:11:05Z

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

+[[java-rest-high-x-pack-rollup-delete-job]]
+=== Delete Rollup Job API
+
+The Delete Rollup Job API can be used to create a new Rollup job


create a new -> delete an existing

tlrx · 2018-10-05T10:12:10Z

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

+
+The Delete Rollup Job API can be used to create a new Rollup job
+in the cluster. The API accepts a `PutRollupJobRequest` object
+as a request and returns a `PutRollupJobResponse`.


We now have a mecanism to avoid repeating this section, can you use it? (with DeleteRollupJobRequest instead of PutRollupJobRequest)

I'd like to do this in a follow up PR: because there is a lot of re-use of RollupDocumentationIT across these, I'd like to just move (get|put|delete) job all at once, and adding that stuff to this PR doesn't feel right to me. I'll open the PR as soon as this is merged. seem reasonable?

pcsanwald · 2018-10-12T11:26:54Z

@tlrx @iverase this is ready for re-review, I believe I have addressed your comments. My apologies on the long delay between your initial comments and me making updates, it's been a busy couple weeks :), thank you very much for reviewing.

iverase · 2018-10-16T11:33:00Z

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

+            DeleteRollupJobResponse::fromXContent,
+            Collections.emptySet());
+    }
+    /**


nit: need a space

iverase

LGTM, just a minor comment

Add support for delete rollup job to HL REST Client.

hub-cap · 2018-12-07T19:44:18Z

hey this needs backporting to 6.x

pcsanwald · 2018-12-10T20:20:18Z

backported in #35045

Paul Sanwald added 3 commits September 25, 2018 18:30

Add support for delete rollup job to HL REST Client.

86a9e1f

remove accidentally adding newline

f430758

add correct headers to new files

4649915

pcsanwald added v7.0.0 :Core/Java High Level REST Client labels Sep 25, 2018

pcsanwald requested review from polyfractal and tlrx September 25, 2018 22:41

Paul Sanwald added 3 commits September 26, 2018 12:05

fix checkstyle

c3cf6fe

fix a bug to use correct http verb. add synchronous method

db61b82

Merge branch 'master' into add-delete-rollup-job-hlrc

0e2afb8

elasticmachine mentioned this pull request Sep 28, 2018

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

Closed

98 tasks

iverase reviewed Sep 28, 2018

View reviewed changes

...nt/rest-high-level/src/main/java/org/elasticsearch/client/rollup/DeleteRollupJobRequest.java Show resolved Hide resolved

iverase reviewed Sep 28, 2018

View reviewed changes

docs/java-rest/high-level/rollup/delete_job.asciidoc Show resolved Hide resolved

iverase reviewed Sep 28, 2018

View reviewed changes

tlrx reviewed Oct 5, 2018

View reviewed changes

Paul Sanwald added 5 commits October 11, 2018 18:40

add test for delete to RollupIT

b46ac17

merge get rollup changes

ebce1ef

address stylistic code review comments

14eb004

add test for deleting a job that doesn't exist. minor docs change

5905e1a

remove extraneous spaces

b9df762

Paul Sanwald added 3 commits October 12, 2018 11:27

fix checkstyle

660e56b

use new execution include and api callouts for docs

eb99685

get callouts aligned correctly

1cb57e9

iverase reviewed Oct 16, 2018

View reviewed changes

iverase self-requested a review October 16, 2018 11:33

iverase approved these changes Oct 16, 2018

View reviewed changes

pcsanwald merged commit 936faba into elastic:master Oct 16, 2018

pcsanwald deleted the add-delete-rollup-job-hlrc branch October 16, 2018 13:02

pcsanwald added the backport pending label Oct 16, 2018

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

Add delete rollup job support to HL REST Client (#34066)

71f1b10

Add support for delete rollup job to HL REST Client.

hub-cap added the v6.6.0 label Dec 7, 2018

pcsanwald removed the backport pending label Dec 10, 2018

tomcallahan added the >enhancement label Dec 18, 2018

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


		private final String id;

		private static final ParseField ID_FIELD = new ParseField("id");

Conversation

pcsanwald commented Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 25, 2018

Uh oh!

Uh oh!

Uh oh!

iverase left a comment

Choose a reason for hiding this comment

Uh oh!

pcsanwald commented Sep 28, 2018

Uh oh!

tlrx left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

pcsanwald commented Oct 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iverase left a comment

Choose a reason for hiding this comment

Uh oh!

hub-cap commented Dec 7, 2018

Uh oh!

pcsanwald commented Dec 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pcsanwald commented Sep 25, 2018 •

edited

Loading

tlrx left a comment •

edited

Loading