From acabee2f9455b2a67a66b5f30547bdb68fcb1438 Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Wed, 3 Jul 2019 16:25:38 -0400 Subject: [PATCH 1/3] Fix Rollup job creation to work with templates 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 --- .../resources/rollup-dynamic-template.json | 46 +++++----- .../elasticsearch/xpack/rollup/Rollup.java | 4 +- .../action/TransportPutRollupJobAction.java | 18 ++-- .../action/PutJobStateMachineTests.java | 4 +- .../rest-api-spec/test/rollup/put_job.yml | 83 +++++++++++++++++++ 5 files changed, 124 insertions(+), 31 deletions(-) diff --git a/x-pack/plugin/core/src/main/resources/rollup-dynamic-template.json b/x-pack/plugin/core/src/main/resources/rollup-dynamic-template.json index 94336c60c4d68..17ffb6de65c5f 100644 --- a/x-pack/plugin/core/src/main/resources/rollup-dynamic-template.json +++ b/x-pack/plugin/core/src/main/resources/rollup-dynamic-template.json @@ -1,26 +1,28 @@ { - "_meta":{ - "_rollup": { - "ROLLUP_METADATA_PLACEHOLDER":"ROLLUP_METADATA_PLACEHOLDER" - }, - "rollup-version": "${rollup.dynamic_template.version}" - }, - "dynamic_templates": [ - { - "strings": { - "match_mapping_type": "string", - "mapping": { - "type": "keyword" + "mappings": { + "_doc": { + "_meta": { + "_rollup": {}, + "rollup-version": "${rollup.dynamic_template.version}" + }, + "dynamic_templates": [ + { + "strings": { + "match_mapping_type": "string", + "mapping": { + "type": "keyword" + } + } + }, + { + "date_histograms": { + "path_match": "*.date_histogram.timestamp", + "mapping": { + "type": "date" + } + } } - } - }, - { - "date_histograms": { - "path_match": "*.date_histogram.timestamp", - "mapping": { - "type": "date" - } - } + ] } - ] + } } \ No newline at end of file diff --git a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/Rollup.java b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/Rollup.java index 20c8fb4502891..e9c7b13090b3f 100644 --- a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/Rollup.java +++ b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/Rollup.java @@ -89,15 +89,15 @@ public class Rollup extends Plugin implements ActionPlugin, PersistentTaskPlugin public static final String TASK_THREAD_POOL_NAME = RollupField.NAME + "_indexing"; public static final String SCHEDULE_THREAD_POOL_NAME = RollupField.NAME + "_scheduler"; - public static final String MAPPING_METADATA_PLACEHOLDER = "\"ROLLUP_METADATA_PLACEHOLDER\":\"ROLLUP_METADATA_PLACEHOLDER\""; public static final String ROLLUP_TEMPLATE_VERSION_FIELD = "rollup-version"; - public static final String ROLLUP_TEMPLATE_VERSION_PATTERN = + private static final String ROLLUP_TEMPLATE_VERSION_PATTERN = Pattern.quote("${rollup.dynamic_template.version}"); private static final String ROLLUP_TEMPLATE_NAME = "/rollup-dynamic-template.json"; public static final String DYNAMIC_MAPPING_TEMPLATE = TemplateUtils.loadTemplate(ROLLUP_TEMPLATE_NAME, Version.CURRENT.toString(), Rollup.ROLLUP_TEMPLATE_VERSION_PATTERN); + // list of headers that will be stored when a job is created public static final Set HEADER_FILTERS = new HashSet<>(Arrays.asList("es-security-runas-user", "_xpack_security_authentication")); diff --git a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java index 571d37e9652ba..98044ce5bb161 100644 --- a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java +++ b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java @@ -37,7 +37,9 @@ import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.time.DateUtils; import org.elasticsearch.common.unit.TimeValue; -import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; +import org.elasticsearch.common.xcontent.XContentHelper; +import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.license.LicenseUtils; import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.persistent.PersistentTasksCustomMetaData; @@ -147,16 +149,18 @@ private static RollupJob createRollupJob(RollupJobConfig config, ThreadPool thre return new RollupJob(config, filteredHeaders); } + @SuppressWarnings("unchecked") static void createIndex(RollupJob job, ActionListener listener, PersistentTasksService persistentTasksService, Client client, Logger logger) { - String jobMetadata = "\"" + job.getConfig().getId() + "\":" + job.getConfig().toJSONString(); + Map jobMetaData = toMap(job.getConfig().toJSONString()); + Map mapping = toMap(Rollup.DYNAMIC_MAPPING_TEMPLATE); - String mapping = Rollup.DYNAMIC_MAPPING_TEMPLATE - .replace(Rollup.MAPPING_METADATA_PLACEHOLDER, jobMetadata); + ((Map)((Map) ((Map) ((Map) mapping.get("mappings")) + .get("_doc")).get("_meta")).get("_rollup")).put(job.getConfig().getId(), jobMetaData); CreateIndexRequest request = new CreateIndexRequest(job.getConfig().getRollupIndex()); - request.mapping(RollupField.TYPE_NAME, mapping, XContentType.JSON); + request.source(mapping, LoggingDeprecationHandler.INSTANCE); client.execute(CreateIndexAction.INSTANCE, request, ActionListener.wrap(createIndexResponse -> startPersistentTask(job, listener, persistentTasksService), e -> { @@ -171,6 +175,10 @@ static void createIndex(RollupJob job, ActionListener list })); } + private static Map toMap(String s) { + return XContentHelper.convertToMap(JsonXContent.jsonXContent, s, false); + } + @SuppressWarnings("unchecked") static void updateMapping(RollupJob job, ActionListener listener, PersistentTasksService persistentTasksService, Client client, Logger logger) { diff --git a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/action/PutJobStateMachineTests.java b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/action/PutJobStateMachineTests.java index 19f241440c438..9f5359c9424de 100644 --- a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/action/PutJobStateMachineTests.java +++ b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/action/PutJobStateMachineTests.java @@ -132,8 +132,8 @@ public void testIndexMetaData() throws InterruptedException { String mapping = requestCaptor.getValue().mappings().get("_doc"); // Make sure the version is present, and we have our date template (the most important aspects) - assertThat(mapping, containsString("\"rollup-version\": \"" + Version.CURRENT.toString() + "\"")); - assertThat(mapping, containsString("\"path_match\": \"*.date_histogram.timestamp\"")); + assertThat(mapping, containsString("\"rollup-version\":\"" + Version.CURRENT.toString() + "\"")); + assertThat(mapping, containsString("\"path_match\":\"*.date_histogram.timestamp\"")); listenerCaptor.getValue().onFailure(new ResourceAlreadyExistsException(job.getConfig().getRollupIndex())); latch.countDown(); diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/put_job.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/put_job.yml index 0964c88be2451..deb53a9d05d71 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/put_job.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/put_job.yml @@ -256,4 +256,87 @@ setup: ] } +--- +"Test put job with templates": + + - do: + indices.put_template: + name: test + body: + index_patterns: foo_* + mappings: + properties: + field: + type: keyword + + - do: + headers: + Authorization: "Basic eF9wYWNrX3Jlc3RfdXNlcjp4LXBhY2stdGVzdC1wYXNzd29yZA==" # run as x_pack_rest_user, i.e. the test setup superuser + rollup.put_job: + id: foo + body: > + { + "index_pattern": "foo", + "rollup_index": "foo_rollup", + "cron": "*/30 * * * * ?", + "page_size" :10, + "groups" : { + "date_histogram": { + "field": "the_field", + "calendar_interval": "1h" + } + }, + "metrics": [ + { + "field": "value_field", + "metrics": ["min", "max", "sum"] + } + ] + } + - is_true: acknowledged + + - do: + indices.get_mapping: + index: foo_rollup + + - set: {foo_rollup.mappings._meta.rollup-version: version} + + - match: + foo_rollup: + mappings: + _meta: + _rollup: + foo: + id: "foo" + index_pattern: "foo" + rollup_index: "foo_rollup" + cron: "*/30 * * * * ?" + page_size: 10 + groups : + date_histogram: + calendar_interval: "1h" + field: "the_field" + time_zone: "UTC" + metrics: + - field: "value_field" + metrics: + - "min" + - "max" + - "sum" + timeout: "20s" + rollup-version: $version + dynamic_templates: + - strings: + match_mapping_type: "string" + mapping: + type: "keyword" + - date_histograms: + path_match: "*.date_histogram.timestamp" + mapping: + type: "date" + properties: + field: + type: "keyword" + + From 22675bdadfd31bdc2886a84f3d528979751601cc Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Thu, 1 Aug 2019 14:26:40 -0400 Subject: [PATCH 2/3] Use XContentBuilder for template, remove json template --- .../resources/rollup-dynamic-template.json | 28 ---------- .../elasticsearch/xpack/rollup/Rollup.java | 8 --- .../action/TransportPutRollupJobAction.java | 54 +++++++++++++++---- 3 files changed, 43 insertions(+), 47 deletions(-) delete mode 100644 x-pack/plugin/core/src/main/resources/rollup-dynamic-template.json diff --git a/x-pack/plugin/core/src/main/resources/rollup-dynamic-template.json b/x-pack/plugin/core/src/main/resources/rollup-dynamic-template.json deleted file mode 100644 index 17ffb6de65c5f..0000000000000 --- a/x-pack/plugin/core/src/main/resources/rollup-dynamic-template.json +++ /dev/null @@ -1,28 +0,0 @@ -{ - "mappings": { - "_doc": { - "_meta": { - "_rollup": {}, - "rollup-version": "${rollup.dynamic_template.version}" - }, - "dynamic_templates": [ - { - "strings": { - "match_mapping_type": "string", - "mapping": { - "type": "keyword" - } - } - }, - { - "date_histograms": { - "path_match": "*.date_histogram.timestamp", - "mapping": { - "type": "date" - } - } - } - ] - } - } -} \ No newline at end of file diff --git a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/Rollup.java b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/Rollup.java index e9c7b13090b3f..5d53aa6d53b17 100644 --- a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/Rollup.java +++ b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/Rollup.java @@ -90,19 +90,11 @@ public class Rollup extends Plugin implements ActionPlugin, PersistentTaskPlugin public static final String SCHEDULE_THREAD_POOL_NAME = RollupField.NAME + "_scheduler"; public static final String ROLLUP_TEMPLATE_VERSION_FIELD = "rollup-version"; - private static final String ROLLUP_TEMPLATE_VERSION_PATTERN = - Pattern.quote("${rollup.dynamic_template.version}"); - - private static final String ROLLUP_TEMPLATE_NAME = "/rollup-dynamic-template.json"; - public static final String DYNAMIC_MAPPING_TEMPLATE = TemplateUtils.loadTemplate(ROLLUP_TEMPLATE_NAME, - Version.CURRENT.toString(), Rollup.ROLLUP_TEMPLATE_VERSION_PATTERN); - // list of headers that will be stored when a job is created public static final Set HEADER_FILTERS = new HashSet<>(Arrays.asList("es-security-runas-user", "_xpack_security_authentication")); - private final SetOnce schedulerEngine = new SetOnce<>(); private final Settings settings; private final boolean enabled; diff --git a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java index 98044ce5bb161..f68e563378c74 100644 --- a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java +++ b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java @@ -10,6 +10,7 @@ import org.elasticsearch.ElasticsearchException; import org.elasticsearch.ElasticsearchStatusException; import org.elasticsearch.ResourceAlreadyExistsException; +import org.elasticsearch.Version; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.admin.indices.create.CreateIndexAction; @@ -38,7 +39,9 @@ import org.elasticsearch.common.time.DateUtils; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; +import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentHelper; +import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.license.LicenseUtils; import org.elasticsearch.license.XPackLicenseState; @@ -112,7 +115,7 @@ protected void masterOperation(Task task, PutRollupJobAction.Request request, Cl .fields(request.getConfig().getAllFields().toArray(new String[0])); fieldCapsRequest.setParentTask(clusterService.localNode().getId(), task.getId()); - client.fieldCaps(fieldCapsRequest, new ActionListener() { + client.fieldCaps(fieldCapsRequest, new ActionListener<>() { @Override public void onResponse(FieldCapabilitiesResponse fieldCapabilitiesResponse) { ActionRequestValidationException validationException = request.validateMappings(fieldCapabilitiesResponse.get()); @@ -149,18 +152,17 @@ private static RollupJob createRollupJob(RollupJobConfig config, ThreadPool thre return new RollupJob(config, filteredHeaders); } - @SuppressWarnings("unchecked") static void createIndex(RollupJob job, ActionListener listener, PersistentTasksService persistentTasksService, Client client, Logger logger) { - Map jobMetaData = toMap(job.getConfig().toJSONString()); - Map mapping = toMap(Rollup.DYNAMIC_MAPPING_TEMPLATE); - - ((Map)((Map) ((Map) ((Map) mapping.get("mappings")) - .get("_doc")).get("_meta")).get("_rollup")).put(job.getConfig().getId(), jobMetaData); - CreateIndexRequest request = new CreateIndexRequest(job.getConfig().getRollupIndex()); - request.source(mapping, LoggingDeprecationHandler.INSTANCE); + try { + XContentBuilder mapping = createMappings(job.getConfig()); + request.source(mapping); + } catch (IOException e) { + listener.onFailure(e); + return; + } client.execute(CreateIndexAction.INSTANCE, request, ActionListener.wrap(createIndexResponse -> startPersistentTask(job, listener, persistentTasksService), e -> { @@ -175,8 +177,38 @@ static void createIndex(RollupJob job, ActionListener list })); } - private static Map toMap(String s) { - return XContentHelper.convertToMap(JsonXContent.jsonXContent, s, false); + private static XContentBuilder createMappings(RollupJobConfig config) throws IOException { + return XContentBuilder.builder(XContentType.JSON.xContent()) + .startObject() + .startObject("mappings") + .startObject("_doc") + .startObject("_meta") + .field(Rollup.ROLLUP_TEMPLATE_VERSION_FIELD, Version.CURRENT.toString()) + .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(); } @SuppressWarnings("unchecked") From d64d9953233b4c3bfbbbec081efbc32568d2c572 Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Fri, 2 Aug 2019 11:23:26 -0400 Subject: [PATCH 3/3] checkstyle --- .../src/main/java/org/elasticsearch/xpack/rollup/Rollup.java | 3 --- .../xpack/rollup/action/TransportPutRollupJobAction.java | 3 --- 2 files changed, 6 deletions(-) diff --git a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/Rollup.java b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/Rollup.java index 5d53aa6d53b17..3f7180e43bb7a 100644 --- a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/Rollup.java +++ b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/Rollup.java @@ -6,7 +6,6 @@ package org.elasticsearch.xpack.rollup; import org.apache.lucene.util.SetOnce; -import org.elasticsearch.Version; import org.elasticsearch.action.ActionRequest; import org.elasticsearch.action.ActionResponse; import org.elasticsearch.client.Client; @@ -46,7 +45,6 @@ import org.elasticsearch.xpack.core.rollup.action.StartRollupJobAction; import org.elasticsearch.xpack.core.rollup.action.StopRollupJobAction; import org.elasticsearch.xpack.core.scheduler.SchedulerEngine; -import org.elasticsearch.xpack.core.template.TemplateUtils; import org.elasticsearch.xpack.rollup.action.TransportDeleteRollupJobAction; import org.elasticsearch.xpack.rollup.action.TransportGetRollupCapsAction; import org.elasticsearch.xpack.rollup.action.TransportGetRollupIndexCapsAction; @@ -73,7 +71,6 @@ import java.util.List; import java.util.Set; import java.util.function.Supplier; -import java.util.regex.Pattern; import static java.util.Collections.emptyList; diff --git a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java index f68e563378c74..12fce698a173b 100644 --- a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java +++ b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java @@ -38,11 +38,8 @@ import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.time.DateUtils; import org.elasticsearch.common.unit.TimeValue; -import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.common.xcontent.XContentType; -import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.license.LicenseUtils; import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.persistent.PersistentTasksCustomMetaData;