HLRC: Add ML delete filter action#35382
Conversation
It adds delete ML filter action to the high level rest client. Relates elastic#29827
|
Pinging @elastic/es-core-infra |
benwtrent
left a comment
There was a problem hiding this comment.
Thanks a bunch for knocking this out :D.
Just a couple of comments.
client/rest-high-level/src/main/java/org/elasticsearch/client/ml/DeleteFilterRequest.java
Outdated
Show resolved
Hide resolved
client/rest-high-level/src/main/java/org/elasticsearch/client/MLRequestConverters.java
Outdated
Show resolved
Hide resolved
client/rest-high-level/src/test/java/org/elasticsearch/client/MachineLearningIT.java
Show resolved
Hide resolved
benwtrent
left a comment
There was a problem hiding this comment.
Sorry for not spotting this before :(
| */ | ||
| public class DeleteFilterRequest implements Validatable { | ||
|
|
||
| private final String filter_id; |
There was a problem hiding this comment.
Fields and parameters should be camelCase, so this should be filterId.
Everything else looks good to me.
|
|
||
| static Request deleteFilter(DeleteFilterRequest deleteFilterRequest) { | ||
| String endpoint = new EndpointBuilder() | ||
| .addPathPartAsIs("_xpack") |
There was a problem hiding this comment.
addPathPartAsIs("_xpack", "ml", "filters")
client/rest-high-level/src/test/java/org/elasticsearch/client/MLRequestConvertersTests.java
Show resolved
Hide resolved
|
@elasticmachine test this please |
|
@elasticmachine test this please |
2 similar comments
|
@elasticmachine test this please |
|
@elasticmachine test this please |
hub-cap
left a comment
There was a problem hiding this comment.
Hmm, 2 deletes, 2 diff responses in your PRs... I think we should consider the differences between the two of these and how to return a single, proper ACK. I wont let this hold up the review tho.
| private final String filterId; | ||
|
|
||
| public DeleteFilterRequest(String filterId) { | ||
| this.filterId = Objects.requireNonNull(filterId, "[filter_id] is required"); |
There was a problem hiding this comment.
should this still be [filter_id] ?
There was a problem hiding this comment.
@hub-cap yes, that holds with other ML null checks.
e.g.
|
Can we double check that this does or does not throw a 404? IMHO a resource thats not found should throw a 404 here. This goes back to the other DELETE you did work on @iverase |
|
Cool, so then keeping this one with no ignores=404 is a good thing. ty for verifying @benwtrent |
|
@elasticmachine test this please |
* HLRC: Add ML delete filter action It adds delete ML filter action to the high level rest client. Relates #29827
It adds delete ML filter action to the high level rest client.
Relates #29827