Add start rollup job support to HL REST Client#34623
Add start rollup job support to HL REST Client#34623cbuescher merged 11 commits intoelastic:masterfrom
Conversation
This change adds support for starting a rollup job to High Level REST Client. Relates to elastic#29827
|
Pinging @elastic/es-core-infra |
|
Pinging @elastic/es-search-aggs |
64177dd to
de4b2a9
Compare
|
@elasticmachine test this please |
hub-cap
left a comment
There was a problem hiding this comment.
One nit and one larger comment that I think is worth discussing.
|
|
||
| static Request startJob(final StartRollupJobRequest startRollupJobRequest) throws IOException { | ||
| String endpoint = new RequestConverters.EndpointBuilder() | ||
| .addPathPartAsIs("_xpack") |
There was a problem hiding this comment.
this is now a variadic, so u can .addPathPartAsIs("_xpack", "rollup", "job")....
| builder.startObject(); | ||
| { | ||
| builder.field("acknowledged", isAcknowledged()); | ||
| builder.field(field, isAcknowledged()); |
There was a problem hiding this comment.
Im not sure i like changing this, because it would be nice if we had a more generic AcknowledgedResponse. I understand that yall need to override the "acknowledged" field, but maybe its best to have builder.field(getField(), isAcknowledged()); and override that in the child classes that are in this review. I wonder if the ConstructingObjectParser can also be put in here with the same abstraction.
|
@hub-cap thanks for the review, I updated the PR. Can you take a second look? |
hub-cap
left a comment
There was a problem hiding this comment.
Hey, awesome work. The only thing change is to remove the abstract from getFieldName, and just have the default AcknowledgeResponse impl of this method be "acknowledged", since thats the generic case that other APIs can and will use as well.
I think it might be worth also moving the parser in. You can feel free to shoot this down, but i basically did the following to a sample response i had sitting in my current branch. Mind you, the tests started failing cuz i changed the parse field name, just to make sure it was working :)
public class DeleteRollupJobResponse extends AcknowledgedResponse {
private static final String PARSE_FIELD_NAME = "some_other_ack";
private static final ConstructingObjectParser<DeleteRollupJobResponse, Void> PARSER =
AcknowledgedResponse.generateParser("delete_rollup_job_response", DeleteRollupJobResponse::new, PARSE_FIELD_NAME);
public DeleteRollupJobResponse(boolean acknowledged) {
super(acknowledged);
}
public static DeleteRollupJobResponse fromXContent(final XContentParser parser) throws IOException {
return PARSER.parse(parser, null);
}
@Override
String getFieldName() {
return PARSE_FIELD_NAME;
}
}
and added the following to AcknowledgedResponse
public static <T> ConstructingObjectParser generateParser(String name, Function<Boolean, T> ctor, String parseField) {
ConstructingObjectParser p = new ConstructingObjectParser<>(name, true, args -> ctor.apply((boolean) args[0]));
p.declareBoolean(constructorArg(), new ParseField(parseField));
return p;
}
|
@hub-cap I think I got what you mean. Please take a look at the last commit if thats the change you intended. |
hub-cap
left a comment
There was a problem hiding this comment.
yea this is real nice now.
| super(acknowledged); | ||
| } | ||
|
|
||
| private static final ConstructingObjectParser<DeleteRollupJobResponse, Void> PARSER = AcknowledgedResponse |
This change adds support for starting a rollup job to High Level REST Client. Relates to #29827
This change adds support for starting a rollup job to High Level REST Client. Relates to #29827
This change adds support for starting a rollup job to High Level REST Client.
Relates to #29827