Keep track of timestamp_field mapping as part of a data stream by martijnvg · Pull Request #58096 · elastic/elasticsearch

martijnvg · 2020-06-15T09:36:10Z

This change adds logic that keeps track of a data steam's field mapping of the timestamp_field.
When a data stream is created the mapping for the timestamp_field is extracted and stored with the data stream. Each time a rollover happens this field mapping is applied on top of the final mappings when the new backing index is created.

Having this logic is important, because the mapping inside a composable index template can change and so the mapping for the timestamp field can be changed too or removed. With this change the field mapping for the timestamp_field will stay the same for each new backing index.

… the relevant mapping details

elasticmachine · 2020-06-15T09:36:12Z

Pinging @elastic/es-core-features (:Core/Features/Data streams)

danhermann · 2020-06-15T15:04:26Z

server/src/internalClusterTest/java/org/elasticsearch/indices/DataStreamIT.java

+        // Index doc that triggers creation of a data stream
+        IndexRequest indexRequest = new IndexRequest("logs-foobar").source("{}", XContentType.JSON).opType("create");
+        IndexResponse indexResponse = client().index(indexRequest).actionGet();
+        assertThat(indexResponse.getIndex(), equalTo(".ds-logs-foobar-000001"));


Where possible, could we use the DataStream::getDefaultBackingIndexName method so that any changes there are automatically picked up at least in Java classes?

danhermann · 2020-06-15T15:11:26Z

server/src/main/java/org/elasticsearch/cluster/metadata/DataStream.java

        return Objects.hash(name, timeStampField, indices, generation);
    }
+
+    public static final class TimestampField implements Writeable, ToXContentObject {


Do we want to allow this field to contain any mapping properties or just the field's data type? Also, should we require that it contains a data type property of either date or date_nanos?

This class should contain the mapping attributes. Part of the mapping attributes is the type attribute, so that data type is kept track of.

Ok, should we permit not specifying a data type? A data type of either date or date_nanos is required on the composable template, so it seems like maybe it should be required on the data stream, too?

It isn't possible that any other data type than date and date_nanos is being included the fieldMapping map, because no composable index template with a data stream definition can be created with timestamp field mapping that is not of type date or date_nanos.

When creating a data stream the contents of the field mapping is copied and this includes a type attribute, so that we know whether either date or date_nanos is specified.

I can add an assert here that checks that value for the type key either returns date or date_nanos for sanity checking.

henningandersen

I left a comment to consider about where the mapping for timestamp field is specified. Otherwise looking good.

henningandersen · 2020-06-16T09:12:23Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateDataStreamService.java

        new LinkedHashSet<>(List.of(DateFieldMapper.CONTENT_TYPE, DateFieldMapper.DATE_NANOS_CONTENT_TYPE));

    private final ClusterService clusterService;
+    private final IndicesService indicesService;


I think this is unused?

A left over from an iteration that never made it into the pr.

henningandersen · 2020-06-16T09:16:40Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateDataStreamService.java

+        assert firstBackingIndex.mapping() != null : "no mapping found for backing index [" + firstBackingIndexName + "]";
+
+        String fieldName = template.getDataStreamTemplate().getTimestampField();
+        Map<String, Object> mapping = firstBackingIndex.mapping().getSourceAsMap();


I wonder if it was more intuitive to require that the mapping be specified on the timestamp field in the template rather than mixing it in with the index mappings. In particular, it will be more clear that those are special, not modifiable compared to the others which can change on rollover.

I think requiring that timestamp field mapping the be specified inside the timestamp_field object in a composable template makes sense. It helps highlighting that the timestamp_field and its mapping are special.

However I do think it should be made modifiable? So that new data streams being created can use an updated field mapping? This was my motivation for keeping track of the timestamp field mapping on the DataStream instance, so that new backing indices would use the same valid timestamp field mapping, but when new data streams are being created from the same composable template than an updated timestamp field mapping could be used.

++, agreed, I meant unmodifiable in the sense that template changes will not reflect on already created data-streams.

👍 I think we can change the data_stream definition in a composable template to include the mapping for the timestamp_field mapping in a seperate change?

Yes, that can work out too.

…tamp_field

…mapping

henningandersen

LGTM. I left a number of smaller comments to consider.

henningandersen · 2020-06-17T11:12:38Z

rest-api-spec/src/main/resources/rest-api-spec/test/indices.data_stream/10_basic.yml

        name: "*"
  - match: { 0.name: simple-data-stream1 }
-  - match: { 0.timestamp_field: '@timestamp' }
+  - match: { 0.timestamp_field.field_name: '@timestamp' }


I wonder if this should be just timestamp_field.name?

I would like to also see an assert on the mapping being included and having the right content.

henningandersen · 2020-06-17T11:14:42Z

server/src/internalClusterTest/java/org/elasticsearch/indices/DataStreamIT.java

+        assertThat(indexResponse.getIndex(), equalTo(DataStream.getDefaultBackingIndexName("logs-foobar", 2)));
+
+        // Change the template to have a different timestamp field
+        createIndexTemplate("id1", "logs-foo*", "@timestamp2");


I think we should rename createIndexTemplate to putIndexTemplate to signal that it also updates existing.

henningandersen · 2020-06-17T11:21:27Z

server/src/internalClusterTest/java/org/elasticsearch/indices/DataStreamIT.java

    }

+    public void testChangeTimestampFieldInComposableTemplatePriorToRollOver() throws Exception {
+        createIndexTemplate("id1", "logs-foo*", "@timestamp");


Should we also include tests of any mapping specialization/params? AFAICS, the implementation could keep only the mapping type now and all tests would succeed.

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java

henningandersen · 2020-06-17T11:32:40Z

server/src/main/java/org/elasticsearch/cluster/metadata/DataStream.java

+
+        public TimestampField(String fieldName, Map<String, Object> fieldMapping) {
+            this.fieldName = fieldName;
+            this.fieldMapping = fieldMapping;


Should we assert that the fieldMapping contains ("type")?

I added that assert here: https://github.com/elastic/elasticsearch/pull/58096/files#diff-82f348b800c60fabd3a34ff574e248a5R159

The reason is that in production, this is where new data stream instances are initiated.
Ideally this assertion would be added here, but then all tests need to be changes to supply: Map.of("type, "date") whereas currently tests supply: Map.of(). Maybe it makes sense to add an new constructor that just sets fieldMapping to Map.of("type, "date") and then delegates to this constructor where we can assert whether type key contains a valid value?

Could we not just supply DataStreamTestHelper.defaultMapping() (new method) instead? A few more bytes of source code, but I would prefer asserting this here to avoid something creating this in a wrong way somewhere or under some condition.

…ests

If the timestamp field path consisted out of object fields and if the final mapping did not contain the parent field then an error occurred, because the prior logic assumed that the object field existed.

…tamp_field

…ic#58096) Backporting elastic#58096 to 7.x branch. Relates to elastic#53100 * use mapping source direcly instead of using mapper service to extract the relevant mapping details * moved assertion to TimestampField class and added helper method for tests * Improved logic that inserts timestamp field mapping into an mapping. If the timestamp field path consisted out of object fields and if the final mapping did not contain the parent field then an error occurred, because the prior logic assumed that the object field existed.

Backporting #58096 to 7.x branch. Relates to #53100 * use mapping source direcly instead of using mapper service to extract the relevant mapping details * moved assertion to TimestampField class and added helper method for tests * Improved logic that inserts timestamp field mapping into an mapping. If the timestamp field path consisted out of object fields and if the final mapping did not contain the parent field then an error occurred, because the prior logic assumed that the object field existed.

…58444) With #58096, data streams now track the timestamp field mapping outside of the template associated with the stream. This means you can no longer update the timestamp field mapping using template changes. This updates the associated data stream docs.

…58444) (#58517) With #58096, data streams now track the timestamp field mapping outside of the template associated with the stream. This means you can no longer update the timestamp field mapping using template changes. This updates the associated data stream docs.

martijnvg added 3 commits June 15, 2020 11:28

Keep track of timestamp field mapping as part of a data stream.

57e5161

use mapping source direcly instead of using mapper service to extract…

b10cb35

… the relevant mapping details

fixed rest tests

ca450ca

martijnvg added >non-issue v8.0.0 :StorageEngine/Data streams Data streams and their lifecycles v7.9.0 labels Jun 15, 2020

martijnvg requested review from danhermann and henningandersen June 15, 2020 09:36

elasticmachine added the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label Jun 15, 2020

fixed test compile errors

07bb5f8

danhermann reviewed Jun 15, 2020

View reviewed changes

henningandersen reviewed Jun 16, 2020

View reviewed changes

martijnvg added 6 commits June 16, 2020 11:35

Merge remote-tracking branch 'es/master' into keep_track__of_ds_times…

f810431

…tamp_field

removed unused field

a1eee83

Merge remote-tracking branch 'es/master' into keep_track__of_ds_times…

c6c9bfd

…tamp_field

iter

b783267

fix versioning in serialization

4aa4280

added assertions for sanity checking the type of the timestamp_field …

6e0e847

…mapping

martijnvg requested review from danhermann and henningandersen June 17, 2020 09:41

martijnvg added 3 commits June 17, 2020 12:10

fixed test compile and checkstyle errors

f256b02

fixed hlrc serialization (only field name needs to get parsed)

ea50930

made field final

43098bd

henningandersen approved these changes Jun 17, 2020

View reviewed changes

martijnvg added 4 commits June 17, 2020 13:36

fixed test compile errors

dfbadbe

rename field

6124deb

also assert mapping of timestamp_field

3dfa7aa

renamed method

bee3763

martijnvg added 11 commits June 17, 2020 16:23

Added integ test

951bb1d

moved assertion to TimestampField class and added helper method for t…

9d99ca9

…ests

removed unused imports

5104eff

fixed test

31110c1

Improved logic that inserts timestamp field mapping into an mapping.

85da2ab

If the timestamp field path consisted out of object fields and if the final mapping did not contain the parent field then an error occurred, because the prior logic assumed that the object field existed.

Merge remote-tracking branch 'es/master' into keep_track__of_ds_times…

b617446

…tamp_field

removed unused imports

1c0742e

fixed test compile error

99a247e

Merge remote-tracking branch 'es/master' into keep_track__of_ds_times…

c2ea7a6

…tamp_field

fixed test compile errors after merge

6234cc7

Merge remote-tracking branch 'es/master' into keep_track__of_ds_times…

6452090

…tamp_field

martijnvg merged commit 085ba99 into elastic:master Jun 22, 2020

martijnvg mentioned this pull request Jun 22, 2020

Keep track of timestamp_field mapping as part of a data stream #58400

Merged

jloleysens mentioned this pull request Jun 23, 2020

[Index Management] Fix API Integration Test and use of timestamp_field elastic/kibana#69666

Merged

jrodewig mentioned this pull request Jun 23, 2020

[DOCS] Note that DS timestamp field mapping changes require reindex #58444

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Conversation

martijnvg commented Jun 15, 2020

Uh oh!

elasticmachine commented Jun 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants