-
Notifications
You must be signed in to change notification settings - Fork 25.2k
HLRC: Add ML delete filter action #35382
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
Conversation
It adds delete ML filter action to the high level rest client. Relates elastic#29827
Pinging @elastic/es-core-infra |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not spotting this before :(
*/ | ||
public class DeleteFilterRequest implements Validatable { | ||
|
||
private final String filter_id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fields and parameters should be camelCase
, so this should be filterId
.
Everything else looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits, ship it.
|
||
static Request deleteFilter(DeleteFilterRequest deleteFilterRequest) { | ||
String endpoint = new EndpointBuilder() | ||
.addPathPartAsIs("_xpack") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this still be [filter_id]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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