Add create rollup job api to high level rest client #33521
Conversation
|
Pinging @elastic/es-core-infra |
|
@polyfractal I'm sorry to create a new pull request for this but that was really easier as I created new classes for request, response and configurations objects. I kept the parsing logic even though it's not strictly required for this API as it simplifies the unit tests and prepare the field for other Rollup APIs. |
This pull request adds the Create Rollup Job API to the high level REST client. It supersedes elastic#32703 and adds dedicated request/response objects so that it does not depend on server side components. Related elastic#29827
tlrx
force-pushed
the
add-create-rollup-job-api-to-high-level-client-two
branch
from
1c55364
to
f98ecf4
Compare
| import static org.elasticsearch.client.RequestConverters.REQUEST_BODY_CONTENT_TYPE; | ||
| import static org.elasticsearch.client.RequestConverters.createEntity; | ||
|
|
||
| final class RollupRequestConverters { |
There was a problem hiding this comment.
++ to a dedicated class for these Rollup converters
There was a problem hiding this comment.
Thanks @tlrx! Sorry for the delay on getting this reviewed, busy week :)
There was a problem hiding this comment.
LGTM, but its huge. I cant really do any proper reviewing on the sheer number of classes moving over, so I have to trust that its sane. Even breaking it up will only get u about 2k for the POJOs and like 1k for the HLRC stuff. Still huge.
Great work on the validatable stuff tho.
| * the accumulating validation errors | ||
| * @param exception the {@link ValidationException} to add errors from | ||
| */ | ||
| public final void addValidationErrors(final @Nullable ValidationException exception) { |
|
|
||
| private final boolean acknowledged; | ||
|
|
||
| public PutRollupJobResponse(final boolean acknowledged) { |
There was a problem hiding this comment.
Im ok with making a common response in client for the Ack Responses. We have a lot of them.
There was a problem hiding this comment.
I agree, but the PR was already large enough. I think it must be done once another ack response is added to the HLRC.
|
Thanks @polyfractal and @hub-cap! I apologize for the size of the pull request; I tried to split it into multiple chunks (like I did to clean up the config objects) but did not succeed. |
| final String cron = "*/1 * * * * ?"; | ||
| final int pageSize = randomIntBetween(numDocs, numDocs * 10); | ||
| // TODO expand this to also test with histogram and terms? | ||
| final GroupConfig groups = new GroupConfig(new DateHistogramGroupConfig("date", DateHistogramInterval.DAY)); |
There was a problem hiding this comment.
@polyfractal Do you think you could improve this test? I think I borrowed it from another place and I noticed that it did not cover everything.
Another attempt. Introduced in #33521
Another attempt. Introduced in #33521
This pull request adds the Create Rollup Job API to the high level REST client. It supersedes #32703 and adds dedicated request/response objects so that it does not depend on server side components.
Related #29827