HLRC: Add get watch API #35531

jimczi · 2018-11-14T09:43:17Z

This changes adds the support for the get watch API in the high level rest client.

Relates #29827

This changes adds the support for the get watch API in the high level rest client.

elasticmachine · 2018-11-14T09:43:19Z

Pinging @elastic/es-core-infra

jimczi · 2018-11-16T18:30:29Z

run gradle build tests

jtibshirani

It looks good to me overall, just left a few comments.

jtibshirani · 2018-11-16T17:39:51Z

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

+            .addPathPart(getWatchRequest.getId())
+            .build();
+
+        Request request = new Request(HttpGet.METHOD_NAME, endpoint);


Super small comment: can just be return new Request(...);.

jtibshirani · 2018-11-19T23:01:52Z

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

+    private final BytesReference source;
+    private final XContentType xContentType;
+
+    public GetWatchResponse(String id) {


Small comment: it would be nice to add a comment to this constructor explaining that it is for a 'missing watch', as opposed to the one below.

jtibshirani · 2018-11-19T23:28:53Z

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

+        return source;
+    }
+
+    public XContentType getContentType() {


I wonder if it would make sense to always return JSON content as we do in GetFieldMappingsResponse. In that class, it also looks like we have a sourceAsMap() for convenience.

jtibshirani · 2018-11-19T23:32:11Z

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

+                try (XContentBuilder builder = XContentBuilder.builder(parser.contentType().xContent())) {
+                    builder.copyCurrentStructure(parser);
+                    BytesReference ref = BytesReference.bytes(builder);
+                    return new XContentSource(ref, parser.contentType());


Could we return the builder here instead of creating a new class XContentSource?

jtibshirani · 2018-11-19T23:42:16Z

docs/java-rest/high-level/watcher/get-watch.asciidoc

@@ -0,0 +1,56 @@
+[[java-rest-high-x-pack-watcher-get-watch]]
+=== Get Watch API


There were a couple changes made to the HLRC docs set-up:

It's now common to define and use variables like :api: instead of hard-coding names.

With these variables defined, you can use include::../execution.asciidoc[] to automatically include the 'asynchronous execution' section.

Here's an example that has been updated to the new format: https://raw.githubusercontent.com/elastic/elasticsearch/master/docs/java-rest/high-level/watcher/ack-watch.asciidoc

thanks I adapted the docs

jtibshirani · 2018-11-19T23:50:36Z

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

+
+import static org.elasticsearch.index.mapper.DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER;
+
+public class GetWatchResponseTests extends AbstractXContentTestCase<GetWatchResponseTests.ResponseWrapper> {


I think the new standard is to extend AbstractHlrcStreamableXContentTestCase. @vladimirdolzhenko introduced this testing option, and had linked to 'put license' as an example: https://github.com/elastic/elasticsearch/pull/34547/files#diff-1ed9d64125ce162773bfc2a4b02f5783

for this to work we'd need to implement toXContent on the HLRC GetWatchResponse and by extension to WatchStatus. I can add it but I thought that we've decided not to add this if it's solely used in tests ?

This new testing approach wasn't available when we had that discussion (which I think was part of the 'ack watch' PR). The test works by depending on both the HLRC and transport classes. I think it creates random instances of the transport GetWatchResponse, calls toXContent on them, and then checks that the HLRC GetWatchResponse can correctly parse the output. It shouldn't be necessary to implement toXContent on the HLRC objects.

One slight issue is that the transport GetWatchResponse doesn't currently implement the interface ToXContent. But I think this wouldn't be too hard to refactor, since most of the complexity is in WatchStatus, which does implement ToXContentObject.

I don't know why I was under the impression that fromXContent was also needed on the transport GetWatchResponse to implement AbstractHlrcStreamableXContentTestCase but that's not the case. I switched to the new testing approach in 22c0c7a, thanks.

jimczi · 2018-11-28T19:27:32Z

@jtibshirani I think I addressed all your comments. Can you take another look ?

jtibshirani

Thanks @jimczi, I had a few more questions:

I wonder if the choice to only return the watch in JSON format led to more complications than it's worth. To me it seems cleaner to stick with your original approach. Sorry about that, I didn't foresee how it would interact with the new testing strategy, where both the transport + HLRC objects are used.
Why did we add a headers field to the HLRC WatchStatus (and make some other changes involving headers)?

jtibshirani · 2018-11-28T20:15:16Z

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

+     * Returns the source as a map
+     */
+    public Map<String, Object> getSourceAsMap() throws IOException {
+        if (sourceAsMap == null && source != null) {


A couple questions:

Sorry if I'm missing something, but why don't we use XContentHelper#convertToMap here?

For me it'd be better to avoid introducing the cached sourceAsMap variable, and just do the conversion every time as we do in GetFieldMappingsResponse and IndexRequest. That way this object can remain immutable, and we don't need to reason about thread safety at all.

+1, I changed it to return XContentHelper#convertToMap all the time

jtibshirani · 2018-11-28T20:33:04Z

...lugin/core/src/test/java/org/elasticsearch/protocol/xpack/watcher/GetWatchResponseTests.java

+
+    @Override
+    protected boolean supportsUnknownFields() {
+        return false;


I'm wondering why this is false. Enabling this is useful, as it helps test that clients can handle unforeseen changes in the responses. Should we instead be using randomFieldsExcludeFilter to only omit watch, as we do for shuffled fields?

Right thanks

jtibshirani · 2018-11-28T20:55:13Z

...lugin/core/src/test/java/org/elasticsearch/protocol/xpack/watcher/GetWatchResponseTests.java

+        return new GetWatchResponse(id, version, status, new XContentSource(source, XContentType.JSON));
+    }
+
+    private static BytesReference emptyWatch() {


It would be nice to return a simple, non-empty structure here so that we test that aspect of the response parsing.

jimczi

@jtibshirani I pushed more changes to address your comments. I reintroduced the xContentType and finished the support for headers in the response. Can you take another look ?

jtibshirani

It looks good to me! Thanks for the fixes to WatchStatus as well.

This changes adds the support for the get watch API in the high level rest client.

HLRC: Add get watch API

4a74bfa

This changes adds the support for the get watch API in the high level rest client.

jimczi added >enhancement v7.0.0 :Core/Java High Level REST Client v6.6.0 labels Nov 14, 2018

Merge branch 'master' into hlrc_get_watch

6c23b72

jimczi requested a review from jtibshirani November 16, 2018 17:23

elasticmachine mentioned this pull request Nov 19, 2018

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

Closed

98 tasks

jtibshirani reviewed Nov 19, 2018

View reviewed changes

jimczi added 9 commits November 27, 2018 20:08

Merge branch 'master' into hlrc_get_watch

05dcebe

iter

af60cec

Merge branch 'master' into hlrc_get_watch

425e95f

address review

22c0c7a

fix checkstyle

569aefa

add a method to get the watch as a map

0228652

fix map conversion

1b27b2f

checkstyle

a046649

remove deprecation handler that uses log4j

a23bbc2

jtibshirani reviewed Nov 28, 2018

View reviewed changes

jimczi added 3 commits November 28, 2018 23:37

Merge branch 'master' into hlrc_get_watch

78dab6e

iter

c9c9cb5

cleanup

d10e976

jimczi commented Nov 29, 2018

View reviewed changes

jtibshirani approved these changes Nov 29, 2018

View reviewed changes

jimczi merged commit 5c7b2c5 into elastic:master Nov 30, 2018

jimczi deleted the hlrc_get_watch branch November 30, 2018 10:02

jimczi added a commit that referenced this pull request Nov 30, 2018

HLRC: Add get watch API (#35531)

d8789af

This changes adds the support for the get watch API in the high level rest client.

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 get watch API #35531

HLRC: Add get watch API #35531

jimczi commented Nov 14, 2018

elasticmachine commented Nov 14, 2018

jimczi commented Nov 16, 2018

jtibshirani left a comment

jtibshirani Nov 16, 2018

jtibshirani Nov 19, 2018

jtibshirani Nov 19, 2018

jtibshirani Nov 19, 2018 •

edited

Loading

jtibshirani Nov 19, 2018

jimczi Nov 28, 2018

jtibshirani Nov 19, 2018

jimczi Nov 27, 2018

jtibshirani Nov 28, 2018 •

edited

Loading

jimczi Nov 28, 2018

jimczi commented Nov 28, 2018

jtibshirani left a comment

jtibshirani Nov 28, 2018

jimczi Nov 29, 2018

jtibshirani Nov 28, 2018 •

edited

Loading

jimczi Nov 29, 2018

jtibshirani Nov 28, 2018 •

edited

Loading

jimczi Nov 29, 2018

jimczi left a comment

jtibshirani left a comment

		@@ -0,0 +1,56 @@
		[[java-rest-high-x-pack-watcher-get-watch]]
		=== Get Watch API


		import static org.elasticsearch.index.mapper.DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER;

		public class GetWatchResponseTests extends AbstractXContentTestCase<GetWatchResponseTests.ResponseWrapper> {

HLRC: Add get watch API #35531

HLRC: Add get watch API #35531

Conversation

jimczi commented Nov 14, 2018

elasticmachine commented Nov 14, 2018

jimczi commented Nov 16, 2018

jtibshirani 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

jtibshirani Nov 19, 2018 • edited Loading

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

jtibshirani Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimczi commented Nov 28, 2018

jtibshirani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimczi left a comment

Choose a reason for hiding this comment

jtibshirani left a comment

Choose a reason for hiding this comment

jtibshirani Nov 19, 2018 •

edited

Loading

jtibshirani Nov 28, 2018 •

edited

Loading

jtibshirani Nov 28, 2018 •

edited

Loading

jtibshirani Nov 28, 2018 •

edited

Loading