Skip to content

Conversation

@jdconrad
Copy link
Contributor

@jdconrad jdconrad commented Oct 5, 2018

This adds both the start water service and stop watcher service apis to the HLRC. Includes new request objects, tests, and documentation.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Contributor

@hub-cap hub-cap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easy review to do. Thanks muchos. Please just update the docs and consider (nit) taking out those constructors. Im going to approve this, assuming you will be DRY'ing the docs pages and running the docs build locally as well. Ping me if you want a re-review.


public class StartWatchServiceRequest implements Validatable {

public StartWatchServiceRequest() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need default constructors here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.


public class StopWatchServiceRequest implements Validatable {

public StopWatchServiceRequest() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

@@ -0,0 +1,53 @@
[[java-rest-high-watcher-start-watch-service]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These have a new tidier format. See #34203 for a much more concise version of the docs. Pls do this for both of the asciidoc pages you have added

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@jdconrad
Copy link
Contributor Author

jdconrad commented Oct 18, 2018

@hub-cap Thanks for the review! Will commit once CI passes.

@jdconrad jdconrad merged commit 1100942 into elastic:master Oct 19, 2018
kcm pushed a commit that referenced this pull request Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants