Add create rollup job api to high level rest client #32703
Conversation
|
Pinging @elastic/es-core-infra |
| */ | ||
| package org.elasticsearch.xpack.core.rollup; | ||
| package org.elasticsearch.protocol.xpack.rollup; |
There was a problem hiding this comment.
Note: I moved RollupField along with the rollup configuration objects as it is used in their toBuilders() and getMetadata() methods. I created #32585 and #32579 to remove these two methods from the config objects but we didn't reach a consensus on it (see @polyfractal #32585 (comment)) so any opinion on this is welcome. I don't think it's blocking the PR either.
There was a problem hiding this comment.
Still mulling this over, but after seeing how it integrates in HLRC (namely, being moved to protocol package) I'm thinking it may be better to separate from the config object despite my earlier objections.
The HLRC will require yet another package to keep updated (xpack.rollup, xpack.core, xpack.protocol and the HLRC itself). So probably best to just move toBuilders() and getMetadata() into the rollup package. The logic will still be spread out, but won't require hunting across multiple packages at least.
Thoughts?
I agree it's not blocking though
| @@ -119,7 +131,7 @@ public RollupJobConfig(final String id, | |||
| // Cron doesn't have a parse helper method to see if the cron is valid, | |||
| // so just construct a temporary cron object and if the cron is bad, it'll | |||
| // throw an exception | |||
| Cron testCron = new Cron(cron); | |||
| //Cron testCron = new Cron(cron); | |||
There was a problem hiding this comment.
Note: here Cron is used to validate the cron expression when instanciating the RollupJobConfig. Maybe this validation could be done on the server side (transport action)? Or should we bring Cron in the common lib?
There was a problem hiding this comment.
For ML we had the same problem only much, much worse: to reproduce all our server-side config validation in the HLRC would mean moving several classes you'd expect to be server-side only into the protocol library. We decided not to do complex validation client-side to avoid moving such classes.
There was a problem hiding this comment.
++ to moving this "validation" to the transport action, and adding a TODO to make an actual validation method at some point instead of this hack.
There was a problem hiding this comment.
Thanks, I moved the validation to the transport action
|
@hub-cap I left two comments I'd like to have your opinion on. |
| this.config = config; | ||
| } | ||
|
|
||
| public PutRollupJobRequest() { |
There was a problem hiding this comment.
Do we need to worry about this empty ctor now? E.g. if the user uses this instead of the other ctor we'll be throwing NPEs everywhere?
There was a problem hiding this comment.
Sadly this public ctor is required because of the Streamable nature of PutRollupJobRequest and different packages used for the rollup request and the transport action. Maybe I should have used different requests/response objects for the HLRC since the beginning. It seems that ML did this in the PutJobRequest, but I had the assumption that we were trying to reuse the same objects if possible. For this PutRollupJobRequest I added a check for the configuration in the validate() method, just in case :)
I think PutWatchRequest/DeleteWatchRequest have a similar issue, providing a public ctor but not preventing any resulting NPEs. We may want to change that but I'd defer to Nik/Baz about this.
|
Left a few comments but the rollup'y side LGTM |
tlrx
force-pushed
the
add-create-rollup-job-api-to-high-level-client
branch
from
bb329b5
to
2f340dd
Compare
|
@elasticmachine test this please |
|
Hi @tlrx. We decided to split the request and response from the ones used in server/x-pack, so they do not overlap. Please update your PR such that you create new |
|
Superseded by #33521 |
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
This pull request adds the Create Rollup Job API to the high level REST client.
Related #29827