-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Add delete rollup job support to HL REST Client #34066
Add delete rollup job support to HL REST Client #34066
Conversation
Pinging @elastic/es-core-infra |
...nt/rest-high-level/src/main/java/org/elasticsearch/client/rollup/DeleteRollupJobRequest.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.
In general looks good. Maybe we should add this action to the IT tests:
org.elasticsearch.client.RollupIT
WDYT?
@iverase good call, I'll add a test in |
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.
didn't add support for a synchronous call intentionally, from digging around there is no way to block on this
Can you expand on what is the problem exactly? Thanks!
Otherwise I left some minor comments, it looks great
@@ -73,4 +75,38 @@ public void putRollupJobAsync(PutRollupJobRequest request, RequestOptions option | |||
PutRollupJobResponse::fromXContent, | |||
listener, Collections.emptySet()); | |||
} | |||
|
|||
/** | |||
* delete a rollup job from the cluster |
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.
Nit: delete -> Delete
|
||
private final String id; | ||
|
||
private static final ParseField ID_FIELD = new ParseField("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.
Nit: We usually place static constants at the beginning of the class, then class members
private static final ParseField ID_FIELD = new ParseField("id"); | ||
|
||
public DeleteRollupJobRequest(String id) { | ||
this.id = Objects.requireNonNull(id,"id parameter must not be null"); |
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.
Nit: missing space after ,
return PARSER.parse(parser, null); | ||
} | ||
|
||
private static final ConstructingObjectParser<DeleteRollupJobResponse, Void> PARSER |
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.
I think we could move the declaration of the PARSER into the AcknowledgedResponse
class so that we don't have to repeat the parsing logic in DeleteRollupJobResponse and PutRollupJobResponse?
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.
originally I tried to do this, but it's a static field and I couldn't work out a way to pass the concrete type without using some kind of builder thing, which felt like overkill for a small amount of duplication. Am I missing an easy way to do this?
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.
I was thinking of something like declareInnerHitsParseFields
or declareAggregationFields
(in ParsedAggregation). But I agree this is a bit overkill :)
} catch (Exception e) { | ||
// Swallow any exception, this test does not test actually cancelling. | ||
} | ||
|
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.
extra spaces
...t-high-level/src/test/java/org/elasticsearch/client/documentation/RollupDocumentationIT.java
Show resolved
Hide resolved
[[java-rest-high-x-pack-rollup-delete-job]] | ||
=== Delete Rollup Job API | ||
|
||
The Delete Rollup Job API can be used to create a new Rollup job |
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.
create a new -> delete an existing
|
||
The Delete Rollup Job API can be used to create a new Rollup job | ||
in the cluster. The API accepts a `PutRollupJobRequest` object | ||
as a request and returns a `PutRollupJobResponse`. |
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.
We now have a mecanism to avoid repeating this section, can you use it? (with DeleteRollupJobRequest instead of PutRollupJobRequest)
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.
I'd like to do this in a follow up PR: because there is a lot of re-use of RollupDocumentationIT
across these, I'd like to just move (get|put|delete) job all at once, and adding that stuff to this PR doesn't feel right to me. I'll open the PR as soon as this is merged. seem reasonable?
DeleteRollupJobResponse::fromXContent, | ||
Collections.emptySet()); | ||
} | ||
/** |
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.
nit: need a space
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.
LGTM, just a minor comment
Add support for delete rollup job to HL REST Client.
hey this needs backporting to 6.x |
backported in #35045 |
Related to #29827, add support for deletion of a job to HLRC. A few notes on the PR:
AcknowledgedResponse
to remove a small bit of duplication