Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HLRC: Deactivate Watch API #34192

Merged

Conversation

@not-napoleon
Copy link
Contributor

commented Oct 1, 2018

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.

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Oct 1, 2018

@hub-cap
Copy link
Contributor

left a 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();

This comment has been minimized.

Copy link
@hub-cap

hub-cap Oct 2, 2018

Contributor

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");

This comment has been minimized.

Copy link
@hub-cap

hub-cap Oct 2, 2018

Contributor

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

This comment has been minimized.

Copy link
@hub-cap

hub-cap Oct 2, 2018

Contributor

worth adding a second test case for imho

not-napoleon added 6 commits Oct 2, 2018
Merge branch 'master' into feature/hlrc-deactivate-watch
 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
Merge branch 'master' into feature/hlrc-deactivate-watch
 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
Merge branch 'master' into feature/hlrc-deactivate-watch
 Conflicts:
	docs/java-rest/high-level/supported-apis.asciidoc
@hub-cap
Copy link
Contributor

left a comment

Everything is great, just a minor nit about the validation and then you are good to :shipit:

@@ -28,25 +28,27 @@
private final String watchId;

public DeactivateWatchRequest(String watchId) {

if (watchId == null) {

This comment has been minimized.

Copy link
@hub-cap

hub-cap Oct 15, 2018

Contributor

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.

This comment has been minimized.

Copy link
@not-napoleon

not-napoleon Oct 22, 2018

Author Contributor

so does that mean we don't need to check for white space in the watch id (via PutWatchRequest.isValidId())?

This comment has been minimized.

Copy link
@hub-cap

hub-cap Oct 22, 2018

Contributor

You should also validate this as well in the constructor.

this.watchId = watchId;
}

public String getWatchId() {
return watchId;
}

// as per discussion https://github.com/elastic/elasticsearch/pull/34192/files#r221994527, keeping validate method as a no-op relic

This comment has been minimized.

Copy link
@hub-cap

hub-cap Oct 15, 2018

Contributor

Feel free to just remove the comment here and the whole method instead of just returning empty since the default does this already.

Merge branch 'master' into feature/hlrc-deactivate-watch
 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

@hub-cap hub-cap added v6.5.0 and removed v5.6.5 labels Oct 22, 2018

@hub-cap

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2018

Sorry about the labeling snafu!

@not-napoleon not-napoleon merged commit d94406a into elastic:master Oct 24, 2018

4 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details
not-napoleon added a commit to not-napoleon/elasticsearch that referenced this pull request Oct 24, 2018
HLRC: Deactivate Watch API (elastic#34192)
Relates to elastic#29827
 Conflicts:
	client/rest-high-level/src/test/java/org/elasticsearch/client/WatcherRequestConvertersTests.java
not-napoleon added a commit that referenced this pull request Oct 29, 2018
kcm added a commit that referenced this pull request Oct 30, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.