Add start rollup job support to HL REST Client #34623
Conversation
This change adds support for starting a rollup job to High Level REST Client. Relates to elastic#29827
cbuescher
added
>enhancement
v7.0.0
:Core/Java High Level REST Client
:StorageEngine/Rollup
|
Pinging @elastic/es-core-infra |
|
Pinging @elastic/es-search-aggs |
cbuescher
force-pushed
the
HLClient-addStartJob
branch
from
64177dd
to
de4b2a9
Compare
|
@elasticmachine test this please |
| @@ -48,6 +50,18 @@ static Request putJob(final PutRollupJobRequest putRollupJobRequest) throws IOEx | |||
| return request; | |||
| } | |||
|
|
|||
| 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")....
| @@ -58,7 +60,7 @@ public int hashCode() { | |||
| public XContentBuilder toXContent(XContentBuilder builder, ToXContent.Params params) throws IOException { | |||
| 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? |
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. |
| public DeleteRollupJobResponse(boolean acknowledged) { | ||
| 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
jpountz
removed
the
:StorageEngine/Rollup
This change adds support for starting a rollup job to High Level REST Client.
Relates to #29827