Fix Rollup job creation to work with templates #43943
Conversation
The PutJob API accidentally used an "expert" API of CreateIndexRequest. That API is semi-lenient to syntax; a type could be omitted and the request would work as expected. But if a type was omitted it would not merge with templates correctly, leading to index creation that only has the template and not the requested mappings in the request. This commit refactors the PutJob API to: - Include the type name - Use a less "expert" API in an attempt to future proof against errors - Uses maps instead of string parsing, which is easier to deal with and more robust
polyfractal
added
>bug
:StorageEngine/Rollup
|
Pinging @elastic/es-analytics-geo |
|
@elasticmachine update branch |
|
@elasticmachine run elasticsearch-ci/2 |
|
|
||
| String mapping = Rollup.DYNAMIC_MAPPING_TEMPLATE | ||
| .replace(Rollup.MAPPING_METADATA_PLACEHOLDER, jobMetadata); | ||
| ((Map<String, Object>)((Map<String, Object>) ((Map<String, Object>) ((Map<String, Object>) mapping.get("mappings")) |
There was a problem hiding this comment.
This is...uhh... not ideal, but seemed less hacky than doing a string replace. I could be convinced otherwise, open to alternatives.
There was a problem hiding this comment.
I wonder if we should use an XContentBuilder directly to simplify the understanding of this code. Something like:
private static XContentBuilder createMappings(RollupJobConfig config) throws IOException {
return XContentBuilder.builder(XContentType.JSON.xContent())
.startObject()
.startObject("mappings")
.startObject("_doc")
.startObject("_meta")
.field("rollup-version", Version.CURRENT)
.startObject("_rollup")
.field(config.getId(), config)
.endObject()
.endObject()
.startArray("dynamic_templates")
.startObject()
.startObject("strings")
.field("match_mapping_type", "string")
.startObject("mapping")
.field("type", "keyword")
.endObject()
.endObject()
.endObject()
.startObject()
.startObject("date_histograms")
.field("path_match", "*.date_histogram.timestamp")
.startObject("mapping")
.field("type", "date")
.endObject()
.endObject()
.endObject()
.endArray()
.endObject()
.endObject()
.endObject();
}
... seems easier to read imo. We could also retrieve the dynamic_templates from a file like we do today but we have only 2 rules so I am not too concerned by the inlining here. What do you think ?
There was a problem hiding this comment.
This looks a hundred times better :) Will make the change
There was a problem hiding this comment.
Sorry for the late review @polyfractal . I left one comment.
|
|
||
| String mapping = Rollup.DYNAMIC_MAPPING_TEMPLATE | ||
| .replace(Rollup.MAPPING_METADATA_PLACEHOLDER, jobMetadata); | ||
| ((Map<String, Object>)((Map<String, Object>) ((Map<String, Object>) ((Map<String, Object>) mapping.get("mappings")) |
There was a problem hiding this comment.
I wonder if we should use an XContentBuilder directly to simplify the understanding of this code. Something like:
private static XContentBuilder createMappings(RollupJobConfig config) throws IOException {
return XContentBuilder.builder(XContentType.JSON.xContent())
.startObject()
.startObject("mappings")
.startObject("_doc")
.startObject("_meta")
.field("rollup-version", Version.CURRENT)
.startObject("_rollup")
.field(config.getId(), config)
.endObject()
.endObject()
.startArray("dynamic_templates")
.startObject()
.startObject("strings")
.field("match_mapping_type", "string")
.startObject("mapping")
.field("type", "keyword")
.endObject()
.endObject()
.endObject()
.startObject()
.startObject("date_histograms")
.field("path_match", "*.date_histogram.timestamp")
.startObject("mapping")
.field("type", "date")
.endObject()
.endObject()
.endObject()
.endArray()
.endObject()
.endObject()
.endObject();
}
... seems easier to read imo. We could also retrieve the dynamic_templates from a file like we do today but we have only 2 rules so I am not too concerned by the inlining here. What do you think ?
|
@elasticmachine update branch |
The PutJob API accidentally used an "expert" API of CreateIndexRequest. That API is semi-lenient to syntax; a type could be omitted and the request would work as expected. But if a type was omitted it would not merge with templates correctly, leading to index creation that only has the template and not the requested mappings in the request. This commit refactors the PutJob API to: - Include the type name - Use a less "expert" API in an attempt to future proof against errors - Uses an XContentBuilder instead of string replacing, removes json template
|
It's come to my attention that the impact of this bug is wider than I thought. My original thinking was that only index patterns targeting Rollups were problematic, and should be relatively rare. But since this is affected by any template, common templates like We might want to look into backporting this further. I'll take a look and raise this for discussion with the team |
The PutJob API accidentally used an "expert" API of CreateIndexRequest. That API is semi-lenient to syntax; a type could be omitted and the request would work as expected. But if a type was omitted it would not merge with templates correctly, leading to index creation that only has the template and not the requested mappings in the request. This semi-lenient behavior is why it went unnoticed until now (no tests that also use unrelate templates)
This commit refactors the PutJob API to:
and more robust