Disallow new rollup jobs in clusters with no rollup usage. #108624
Conversation
This change will add logic to the put rollup api that fails if no rollup job is active and no rollup index exists in the cluster. The logic first check whether there is an active rollup persistent task if there are no active rollup persistent tasks, then it checks whether any rollup index exists. The latter check is an expensive check, but assuming that it only runs as part of the pu rollup job api and only when there are no rollup jobs, this should be ok. All tests that invoke the pu rollup job api will need to be adjusted to create a dummy index that has rollup mapping metadata. Otherwise, tests can't create a rollup job. Closes elastic#108381
martijnvg
added
>breaking
:StorageEngine/Rollup
|
Hi @martijnvg, I've created a changelog YAML for you. Note that since this PR is labelled |
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
| listener.onFailure( | ||
| new IllegalArgumentException( | ||
| "rollup has been deprecated and will be removed, therefor starting " | ||
| + "from 8.15.0, creating new rollup jobs is no longer allowed in clusters that don't have any rollup usage." |
There was a problem hiding this comment.
therefore*
I'd just say: "new rollup jobs are not allowed in clusters that don't have any rollup usage, since rollup has been deprecated". The details should be included in rollup documentation.
| if (parser.nextToken() == XContentParser.Token.START_OBJECT) { | ||
| if ("_meta".equals(parser.nextFieldName())) { | ||
| if (parser.nextToken() == XContentParser.Token.START_OBJECT) { | ||
| if ("_rollup".equals(parser.nextFieldName())) { |
There was a problem hiding this comment.
This looks pretty fragile, what if there's another field before _rollup?
Since this is fairly infrequent, I'd go with using sourceAsMap.
There was a problem hiding this comment.
This looks pretty fragile, what if there's another field before _rollup?
I don't there will be another field. The logic in TransportPutRollupJobAction#createMappings(...) always add a _doc._meta._rollup field and the PARSER_CONFIGURATION constant in TransportPutRollupJobAction only allows one field to be present. Let me add some unit tests for this.
I'd go with using sourceAsMap.
Mappings can be pretty large and I like to avoid converting large mappings in map of maps in the jvm heap space.
There was a problem hiding this comment.
In this case, it should throw if there's another field?
Sorry, did you add a unittest this that I missed?
There was a problem hiding this comment.
There was a problem hiding this comment.
Right, I don't see a case with multiple fields under _doc._meta in this file?
There was a problem hiding this comment.
Ah, I see. I added that now too: 3b2b145
This change will add logic to the put rollup api that fails if no rollup job is active and no rollup index exists in the cluster.
The logic first check whether there is an active rollup persistent task if there are no active rollup persistent tasks, then it checks whether any rollup index exists. The latter check is an expensive check, but assuming that it only runs as part of the pu rollup job api and only when there are no rollup jobs, this should be ok.
All tests that invoke the pu rollup job api will need to be adjusted to create a dummy index that has rollup mapping metadata. Otherwise, tests can't create a rollup job.
Closes #108381