Create a WatchStatus class for the high-level REST client. #33527

jtibshirani · 2018-09-07T18:15:13Z

This class will be used in a few of the watcher responses ('get watch', 'ack watch', etc.), so it's being introduced first in its own PR.

To create this PR, I first copied over all WatchStatus and its dependencies without modification, then iteratively cleaned them up. It's likely easiest to review by going through the commits separately.

elasticmachine · 2018-09-07T18:15:15Z

Pinging @elastic/es-core-infra

jtibshirani · 2018-09-08T00:40:52Z

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

+
+import java.io.IOException;
+
+public final class WatchStatusDateParser {


Somewhat confusingly, the date parsing logic used in WatchStatus is different from that in ActionStatus. I've opted to maintain the logic while refactoring until we have more complete test coverage (hopefully against a live server) and can better understand the behavior.

jtibshirani · 2018-09-08T00:45:43Z

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

+
+public class WatchStatusTests extends ESTestCase {
+
+ public void testBasicParsing() throws IOException {


In order to sanity check this PR, I manually added some parsing tests. One other option would be to keep the toXContent logic, and create a AbstractXContentTestCase. I decided against that approach because there was quite a bit of logic associated with toXContent, and this isn't the test direction we're ultimately aiming for.

I think you can avoid that by overriding AbstractXContentTestCase#assertToXContentEquivalence? I think it's worth using AbstractXContentTestCase here, it's going to be much more thorough than hand-rolling parsing tests.

we need to come to consensus on that, since we wont actually have the toXContent on a lot of classes, and i dont know if i want to add it solely for completing a test. Also, testing that the class can ser/deser the bytes is one thing, but testing that a server side response ser's into something that the client can deser is much more valuable, since thats the way we know the client actually works. After support summit ill have a few brain cells to devote to this as Id like to get an answer out sooner than later.

Thanks everyone for taking a look. I purposefully removed the toXContent method and stream serialization methods (in the commit “Remove logic related to writing xContent and stream serialization”) because in the high-level REST client, response classes are only ever deserialized from xContent. As @hub-cap mentioned, we expect this to be the case for most of the response classes.

In tests like AbstractXContentTestCase, we test parsing by creating an object, generating xContent, then parsing it back to an object and checking equality. So because we no longer have a toXContent method, this testing strategy doesn’t apply. My understanding was that we did not want to keep around toXContent just to help with testing, and would prefer to move in a direction where we’re testing against a live server (or some kind of API spec?)

@hub-cap do you have thoughts about the right testing approach for this PR? It would be nice make progress on getting this merged (even if the testing approach is temporary), so others can continue their watcher HLRC work.

I need to sit down to make a approach, ill finally have time early next week. Id prefer the manual parsing in order to test the fromXContents, for now.

I clarified with @hub-cap and he thinks we should stick with the current testing approach for this PR + proceed with the code review (or as he says, :) )

There are already a couple of actions implemented. For example this one which is the delete watcher action:

#32337

Responses extends the abstract class ActionResponse which seems to require serialisation/deserialisation. The responses classes implement ToXContentObject as well so no sure if for consistency we should do the same.

Right, but looking at a more recent xpack HLRC PR (#32332), the responses no longer contain the extraneous serialization/ deserialization logic (see PutUserResponse). I assumed that this is the preferred way going forward, especially now that we are moving the client classes into a separate lightweight module. From the above comments, it sounds like @hub-cap is okay with this for now.

@jtibshirani is correct

This other approach looks fine to me as well. Thanks for the clarification!

romseygeek

This looks a great start @jtibshirani, I left a couple of comments.

romseygeek · 2018-09-08T08:49:01Z

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

+
+public class WatchStatusTests extends ESTestCase {
+
+ public void testBasicParsing() throws IOException {


I think you can avoid that by overriding AbstractXContentTestCase#assertToXContentEquivalence? I think it's worth using AbstractXContentTestCase here, it's going to be much more thorough than hand-rolling parsing tests.

romseygeek · 2018-09-08T08:50:42Z

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

+ }
+
+ public static ActionStatus parse(String actionId, XContentParser parser) throws IOException {
+ AckStatus ackStatus = null;


I think using ConstructingObjectParser would make all these parse methods much clearer

Thanks @romseygeek -- I agree, but I opted not to make major refactors to the shared parsing logic to keep this PR focused. Maybe I could do a follow-up to clean up ActionStatus (both the HLRC version, and the core watcher version)?

iverase

Thanks Julie for this super effort. I am wondering if this classes should implement {toXContent} for Json serialisation and potentially {Writable} as well.

romseygeek · 2018-09-19T15:28:22Z

I think this is ready to merge now? If there's consensus on the tests, then I'm +1

jtibshirani · 2018-09-19T16:16:53Z

👍 I'll rebase and make sure the tests are green. It would be great to get a PR approval, if everyone is on board.

…output.

Also fix the following issues it uncovered: * Make sure that parsing is robust to unknown fields. * Add the missing method WatchStatus#lastMetCondition. * Remove the headers parameter, as headers should not be visible in the REST API.

This will simplify the parsing of responses, especially in the case of acks where the watch ID is not available.

iverase

So here is my thumb up.

We agreed that implementation of request/response will follow the implementation of #32332 and therefore no need of serialisation/deserialisation of WatchStatus.
LGTM

hub-cap · 2018-09-19T18:09:49Z

This class will be used in a few of the watcher responses ('get watch', 'ack watch', etc.), so it's being introduced first in its own PR.

jtibshirani added WIP v7.0.0 :Core/Java High Level REST Client :Data Management/Watcher v6.5.0 labels Sep 7, 2018

jtibshirani force-pushed the watch-status-parsing branch 2 times, most recently from 8ec1fe1 to a72fa01 Compare September 7, 2018 23:20

jtibshirani removed the WIP label Sep 8, 2018

jtibshirani commented Sep 8, 2018

View reviewed changes

romseygeek reviewed Sep 8, 2018

View reviewed changes

iverase reviewed Sep 10, 2018

View reviewed changes

jtibshirani added 11 commits September 19, 2018 09:23

Duplicate WatchStatus and its dependencies.

b4a5a49

Remove WatcherXContentParser, as it isn't necessary for parsing REST …

7982286

…output.

Remove logic related to writing xContent and stream serialization.

96a9469

Remove logic related to updating watches.

725366f

Remove the Exceptions utility class.

0f483bb

Make instance variables private and final where possible.

8d18836

Small simplification in ActionStatus.AckStatus.State.

241c8a2

Add basic tests around WatchStatus parsing.

22aaf2f

Also fix the following issues it uncovered: * Make sure that parsing is robust to unknown fields. * Add the missing method WatchStatus#lastMetCondition. * Remove the headers parameter, as headers should not be visible in the REST API.

Clean up date parsing.

b015647

Use the apache license for client classes.

78a2174

Avoid requiring the watch ID during parsing.

5cae92f

This will simplify the parsing of responses, especially in the case of acks where the watch ID is not available.

jtibshirani force-pushed the watch-status-parsing branch from a72fa01 to 5cae92f Compare September 19, 2018 16:23

iverase self-requested a review September 19, 2018 16:34

iverase approved these changes Sep 19, 2018

View reviewed changes

jtibshirani merged commit c764012 into elastic:master Sep 19, 2018

jtibshirani deleted the watch-status-parsing branch September 19, 2018 19:17

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

Create a WatchStatus class for the high-level REST client. (#33527)

83e30ee

This class will be used in a few of the watcher responses ('get watch', 'ack watch', etc.), so it's being introduced first in its own PR.

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

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

Create a WatchStatus class for the high-level REST client. #33527

Create a WatchStatus class for the high-level REST client. #33527

jtibshirani commented Sep 7, 2018 •

edited

Loading

elasticmachine commented Sep 7, 2018

jtibshirani Sep 8, 2018 •

edited

Loading

jtibshirani Sep 8, 2018 •

edited

Loading

romseygeek Sep 8, 2018

hub-cap Sep 8, 2018

jtibshirani Sep 13, 2018

hub-cap Sep 14, 2018

jtibshirani Sep 14, 2018 •

edited

Loading

iverase Sep 17, 2018

jtibshirani Sep 17, 2018 •

edited

Loading

hub-cap Sep 17, 2018

iverase Sep 18, 2018

romseygeek left a comment

romseygeek Sep 8, 2018

romseygeek Sep 8, 2018

jtibshirani Sep 13, 2018

iverase left a comment

romseygeek commented Sep 19, 2018

jtibshirani commented Sep 19, 2018

iverase left a comment

hub-cap commented Sep 19, 2018


		import java.io.IOException;

		public final class WatchStatusDateParser {


		public class WatchStatusTests extends ESTestCase {

		public void testBasicParsing() throws IOException {

Create a WatchStatus class for the high-level REST client. #33527

Create a WatchStatus class for the high-level REST client. #33527

Conversation

jtibshirani commented Sep 7, 2018 • edited Loading

elasticmachine commented Sep 7, 2018

jtibshirani Sep 8, 2018 • edited Loading

Choose a reason for hiding this comment

jtibshirani Sep 8, 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 Sep 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani Sep 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romseygeek 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

iverase left a comment

Choose a reason for hiding this comment

romseygeek commented Sep 19, 2018

jtibshirani commented Sep 19, 2018

iverase left a comment

Choose a reason for hiding this comment

hub-cap commented Sep 19, 2018

jtibshirani commented Sep 7, 2018 •

edited

Loading

jtibshirani Sep 8, 2018 •

edited

Loading

jtibshirani Sep 8, 2018 •

edited

Loading

jtibshirani Sep 14, 2018 •

edited

Loading

jtibshirani Sep 17, 2018 •

edited

Loading