HLRC: Deactivate Watch API#34192
Conversation
|
Pinging @elastic/es-core-infra |
hub-cap
left a comment
There was a problem hiding this comment.
Please also add a asciidoc API as well as a Documentation Test for the asciidoc API.
|
|
||
| @Override | ||
| public Optional<ValidationException> validate() { | ||
| ValidationException exception = new ValidationException(); |
There was a problem hiding this comment.
Lets move this validation into the constructor and remove this validate method. Still leave it Validatable, its just a relic :)
| return status; | ||
| } | ||
|
|
||
| private static final ParseField STATUS_FIELD = new ParseField("status"); |
There was a problem hiding this comment.
typically this stuff is all at the top, along with the static block below
| new DeactivateWatchRequest(watchId), RequestOptions.DEFAULT); | ||
| assertThat(response.getStatus().state().isActive(), is(false)); | ||
| } | ||
| // Deactivate a watch that does not exist |
There was a problem hiding this comment.
worth adding a second test case for imho
Conflicts: client/rest-high-level/src/main/java/org/elasticsearch/client/WatcherClient.java client/rest-high-level/src/main/java/org/elasticsearch/client/WatcherRequestConverters.java client/rest-high-level/src/test/java/org/elasticsearch/client/WatcherIT.java client/rest-high-level/src/test/java/org/elasticsearch/client/WatcherRequestConvertersTests.java
Conflicts: client/rest-high-level/src/main/java/org/elasticsearch/client/WatcherClient.java client/rest-high-level/src/main/java/org/elasticsearch/client/WatcherRequestConverters.java client/rest-high-level/src/test/java/org/elasticsearch/client/WatcherIT.java client/rest-high-level/src/test/java/org/elasticsearch/client/WatcherRequestConvertersTests.java client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/WatcherDocumentationIT.java docs/java-rest/high-level/supported-apis.asciidoc
Conflicts: docs/java-rest/high-level/supported-apis.asciidoc
hub-cap
left a comment
There was a problem hiding this comment.
Everything is great, just a minor nit about the validation and then you are good to 
|
|
||
| public DeactivateWatchRequest(String watchId) { | ||
|
|
||
| if (watchId == null) { |
There was a problem hiding this comment.
Sorry I did not explain myself well enuf. This is good, but we dont need the ValidationException here at all, really. You can instead just use Objects.requireNotNull(obj, msg) and it will throw exceptions for you. its more concise.
There was a problem hiding this comment.
so does that mean we don't need to check for white space in the watch id (via PutWatchRequest.isValidId())?
There was a problem hiding this comment.
You should also validate this as well in the constructor.
| return watchId; | ||
| } | ||
|
|
||
| // as per discussion https://github.com/elastic/elasticsearch/pull/34192/files#r221994527, keeping validate method as a no-op relic |
There was a problem hiding this comment.
Feel free to just remove the comment here and the whole method instead of just returning empty since the default does this already.
Conflicts: client/rest-high-level/src/main/java/org/elasticsearch/client/WatcherClient.java client/rest-high-level/src/test/java/org/elasticsearch/client/WatcherIT.java client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/WatcherDocumentationIT.java
|
Sorry about the labeling snafu! |
Relates to elastic#29827 Conflicts: client/rest-high-level/src/test/java/org/elasticsearch/client/WatcherRequestConvertersTests.java
Relates to #29827
There are no doc tests on this PR right now, because when I was writing them, I saw @hub-cap 's comments on #33988 that we're about to merge a new way to do docs & doc tests for HLRC work. Everything else is ready for review.