HLRC: Add activate watch action #33988

iverase · 2018-09-24T10:23:12Z

Adds activate watch action to the high level rest client.

Relates #29827

Relates elastic#29827

elasticmachine · 2018-09-24T10:38:31Z

Pinging @elastic/es-core-infra

pcsanwald

A few small naming suggestions and one test suggestion, but I don't want to block merging on re-review.

pcsanwald · 2018-09-28T15:12:55Z

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

+    }
+
+    @Override
+    public String toString() {


do we need to override toString here? seems relatively harmless, but wasn't sure if it's necessary.

pcsanwald · 2018-09-28T15:18:07Z

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

+
+        }
+
+        // Activate watch that does not exist


I'd split this into a separate test case, testActivateWatchThatExists and testActivateWatchThatDoesNotExist

pcsanwald · 2018-09-28T15:21:44Z

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

@@ -75,4 +76,14 @@ public void testDeleteWatch() {
        assertEquals("/_xpack/watcher/watch/" + watchId, request.getEndpoint());
        assertThat(request.getEntity(), nullValue());
    }
+
+    public void testActivateWatch() {


I think testActivateWatchRequestConversion is a clearer name.

hub-cap

Wow im super sorry, i had done a review and forgot to click submit :/

Update to today, and we will very soon have a better way to test responses (you do not have a response test) and to generate docs, in the next day or two. We should hold off until then to merge. Feel free to fix up my nits tho.

hub-cap · 2018-09-25T16:17:03Z

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

+            .addPathPartAsIs("_xpack")
+            .addPathPartAsIs("watcher")
+            .addPathPartAsIs("watch")
+            .addPathPartAsIs(activateWatchRequest.getWatchId())


.addPathPart(activateWatchRequest...) .addPathPartAsIs("_activate")

Looks like you swapped the two of them.

hub-cap · 2018-09-25T16:18:03Z

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

+
+        ValidationException exception = new ValidationException();
+
+        if (watchId == null) {


You should just put these in the constructor (the null check already is) and throw from there instead of here and just not override it

hub-cap · 2018-09-25T16:19:42Z

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

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


Node needed and it really doesnt even do anything, so we can remove it and ToXContent interface

hub-cap · 2018-09-25T16:19:56Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/watcher/ActivateWatchResponse.java

+        return Objects.hash(status);
+    }
+
+


extra space

hub-cap · 2018-09-28T17:03:37Z

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

+
+        }
+
+        // Activate watch that does not exist


hub-cap · 2018-09-28T17:04:32Z

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

@@ -132,4 +135,59 @@ public void onFailure(Exception e) {
        }
    }

+    public void testActivateWatch() throws Exception {


We will have a new ay to deal with the docs+test, we should wait to merge until it is merged.

@hub-cap which is the issue that it is introducing this change?

#34125 and it looks like its merged!

iverase · 2018-10-03T10:17:17Z

@hub-cap Added response test and changed the docs. I think it is getting closer.

Is this change required in 6.x?

hub-cap · 2018-10-03T18:02:30Z

Yea we backport all of the HLRC APIs to the current 6.x. Some of them landed in 6.4, most will land in 6.5

hub-cap

minor nits, then

hub-cap · 2018-10-03T18:03:13Z

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

+/**
+ * A request to explicitly activate a watch.
+ */
+public final class ActivateWatchRequest implements Validatable, Closeable {


Any reason this is Closeable?

You may also have a look at TimedRequest, assuming this needs the timeouts (I dont know if it does or not, you would have to research the transport layer action)

hub-cap · 2018-10-03T18:05:37Z

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

+
+    public ActivateWatchRequest(String watchId) {
+        if (watchId == null) {
+            throw new IllegalArgumentException("Watch identifier is required");


there is a Objects.requireNonNull helper you can use here

hub-cap · 2018-10-03T18:05:58Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/watcher/ActivateWatchResponse.java

+        return Objects.hash(status);
+    }
+
+    private static final ParseField STATUS_FIELD = new ParseField("status");


This is typically on teh top of these classes.

hub-cap · 2018-10-03T18:06:13Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/watcher/ActivateWatchResponse.java

+        new ConstructingObjectParser<>("activate_watch_response", true,
+            a -> new ActivateWatchResponse((WatchStatus) a[0]));
+
+    static {


this too is on top

iverase · 2018-10-04T05:18:59Z

@elasticmachine test this please

iverase · 2018-10-04T06:29:16Z

@elasticmachine test this please

iverase · 2018-10-04T10:52:13Z

@hub-cap The commit cannot be back ported as WatchStatus has not been back ported. If necessary I can coordinate with @jtibshirani to do so.

jtibshirani · 2018-10-04T12:01:26Z

Just backported it to 6.x in 83e30ee, sorry for the delay!

* HLRC: Add activate watcher action Adds activate watch action to the high level rest client. Relates #29827

* master: (25 commits) HLRC: ML Adding get datafeed stats API (elastic#34271) Small fixes to the HLRC watcher documentation. (elastic#34306) Tasks: Document that status is not semvered (elastic#34270) HLRC: ML Add preview datafeed api (elastic#34284) [CI] Fix bogus ScheduleWithFixedDelayTests.testRunnableRunsAtMostOnceAfterCancellation Fix error in documentation for activete watch SCRIPTING: Terms set query expression (elastic#33856) Logging: Drop remaining Settings log ctor (elastic#34149) [ML] Remove unused last_data_time member from Job (elastic#34262) Docs: Allow skipping response assertions (elastic#34240) HLRC: Add activate watch action (elastic#33988) [Security] Multi Index Expression alias wildcard exclusion (elastic#34144) [ML] Label anomalies with multi_bucket_impact (elastic#34233) Document smtp.ssl.trust configuration option (elastic#34275) Support PKCS#11 tokens as keystores and truststores (elastic#34063) Fix sporadic failure in NestedObjectMapperTests [Authz] Allow update settings action for system user (elastic#34030) Replace version with reader cache key in IndicesRequestCache (elastic#34189) [TESTS] Set SO_LINGER and SO_REUSEADDR on the mock socket (elastic#34211) Security: upgrade unboundid ldapsdk to 4.0.8 (elastic#34247) ...

* rename-ccr-stats: (25 commits) HLRC: ML Adding get datafeed stats API (elastic#34271) Small fixes to the HLRC watcher documentation. (elastic#34306) Tasks: Document that status is not semvered (elastic#34270) HLRC: ML Add preview datafeed api (elastic#34284) [CI] Fix bogus ScheduleWithFixedDelayTests.testRunnableRunsAtMostOnceAfterCancellation Fix error in documentation for activete watch SCRIPTING: Terms set query expression (elastic#33856) Logging: Drop remaining Settings log ctor (elastic#34149) [ML] Remove unused last_data_time member from Job (elastic#34262) Docs: Allow skipping response assertions (elastic#34240) HLRC: Add activate watch action (elastic#33988) [Security] Multi Index Expression alias wildcard exclusion (elastic#34144) [ML] Label anomalies with multi_bucket_impact (elastic#34233) Document smtp.ssl.trust configuration option (elastic#34275) Support PKCS#11 tokens as keystores and truststores (elastic#34063) Fix sporadic failure in NestedObjectMapperTests [Authz] Allow update settings action for system user (elastic#34030) Replace version with reader cache key in IndicesRequestCache (elastic#34189) [TESTS] Set SO_LINGER and SO_REUSEADDR on the mock socket (elastic#34211) Security: upgrade unboundid ldapsdk to 4.0.8 (elastic#34247) ...

* HLRC: Add activate watcher action Adds activate watch action to the high level rest client. Relates #29827

HLRC: Add activate watcher action

8c4487d

Relates elastic#29827

iverase requested a review from hub-cap September 24, 2018 10:23

elasticmachine mentioned this pull request Sep 24, 2018

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

Closed

98 tasks

iverase changed the title ~~HLRC: Add activate watcher action~~ HLRC: Add activate watch action Sep 24, 2018

remove unused imports

8533941

iverase added :Core/Java High Level REST Client :Data Management/Watcher labels Sep 24, 2018

iverase added v7.0.0 v6.5.0 labels Sep 24, 2018

iverase self-assigned this Sep 24, 2018

Merge branch 'master' into activateWatcher

0a27785

pcsanwald approved these changes Sep 28, 2018

View reviewed changes

hub-cap suggested changes Sep 28, 2018

View reviewed changes

not-napoleon mentioned this pull request Oct 1, 2018

HLRC: Deactivate Watch API #34192

Merged

iverase added 7 commits October 2, 2018 16:43

Implemented first round of PR comments

95c874a

rename test

cdd2579

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

70dcf64

Update docs and cos test using new approach

4a3db30

merge with head and resolve conflicts

b1bd977

remove extra space

04ed79a

add response test

78f88d0

hub-cap approved these changes Oct 3, 2018

View reviewed changes

iverase added 2 commits October 3, 2018 21:39

clean up request and response classes

7799655

remove unused import

d602424

Merge branch 'master' into activateWatcher

17705f2

iverase merged commit 3ccb7af into elastic:master Oct 4, 2018

iverase added a commit that referenced this pull request Oct 4, 2018

HLRC: Add activate watch action (#33988)

d967725

* HLRC: Add activate watcher action Adds activate watch action to the high level rest client. Relates #29827

iverase deleted the activateWatcher branch October 4, 2018 12:47

colings86 added >enhancement and removed :Data Management/Watcher labels Oct 25, 2018

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

HLRC: Add activate watch action (#33988)

0112631

* HLRC: Add activate watcher action Adds activate watch action to the high level rest client. Relates #29827

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HLRC: Add activate watch action #33988

HLRC: Add activate watch action #33988

iverase commented Sep 24, 2018 •

edited

Loading

elasticmachine commented Sep 24, 2018

pcsanwald left a comment

pcsanwald Sep 28, 2018

pcsanwald Sep 28, 2018

hub-cap Sep 28, 2018

pcsanwald Sep 28, 2018

hub-cap left a comment

hub-cap Sep 25, 2018

hub-cap Sep 25, 2018

hub-cap Sep 25, 2018

hub-cap Sep 25, 2018

hub-cap Sep 28, 2018

hub-cap Sep 28, 2018

iverase Oct 2, 2018

hub-cap Oct 2, 2018

iverase commented Oct 3, 2018 •

edited

Loading

hub-cap commented Oct 3, 2018

hub-cap left a comment

hub-cap Oct 3, 2018

hub-cap Oct 3, 2018

hub-cap Oct 3, 2018

hub-cap Oct 3, 2018

hub-cap Oct 3, 2018

iverase commented Oct 4, 2018

iverase commented Oct 4, 2018

iverase commented Oct 4, 2018

jtibshirani commented Oct 4, 2018


		ValidationException exception = new ValidationException();

		if (watchId == null) {

HLRC: Add activate watch action #33988

HLRC: Add activate watch action #33988

Conversation

iverase commented Sep 24, 2018 • edited Loading

elasticmachine commented Sep 24, 2018

pcsanwald 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

hub-cap 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

iverase commented Oct 3, 2018 • edited Loading

hub-cap commented Oct 3, 2018

hub-cap 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

iverase commented Oct 4, 2018

iverase commented Oct 4, 2018

iverase commented Oct 4, 2018

jtibshirani commented Oct 4, 2018

iverase commented Sep 24, 2018 •

edited

Loading

iverase commented Oct 3, 2018 •

edited

Loading