Skip to content

Fix time series timestamp meta missing#80695

Merged
nik9000 merged 11 commits intoelastic:masterfrom
weizijun:fix-timeseries-timestamp-badcase
Nov 22, 2021
Merged

Fix time series timestamp meta missing#80695
nik9000 merged 11 commits intoelastic:masterfrom
weizijun:fix-timeseries-timestamp-badcase

Conversation

@weizijun
Copy link
Copy Markdown
Contributor

@weizijun weizijun commented Nov 15, 2021

I find a time series timestamp meta missing case.

here is the reproduce steps:

  1. create a time_series index, and set the timestamp field some meta.
  2. index a doc with a new field that is not in mappings, it will call mappings marge.
  3. then the timestamp field meta is missing.

the reason that meta is missing is when a new field comes, MappingParser.parse will build a new mapping with new fields. And merge the new mappings with exist mapping.
the new mapping have no timestamp field, so it will auto add timestamp field, the timestamp is without user's meta info.
And merge method build a new timestamp field to override the user's timestamp field.
It cause the timestamp meta missing.

I fixed the case, by move timestamp logic from MappingParser.parse to create index logic.
And move the tests to a new IT test.
I add a test to test case, TimeSeriesModeIT.testAddTimeStampMeta will fail in the pre-commit.

@elasticsearchmachine elasticsearchmachine added v8.1.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Nov 15, 2021
@nik9000 nik9000 added the :StorageEngine/TSDB You know, for Metrics label Nov 15, 2021
@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Nov 15, 2021
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Nov 15, 2021

Pinging @elastic/es-analytics-geo (Team:Analytics)

@nik9000 nik9000 added >bug and removed Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Nov 15, 2021
@weizijun
Copy link
Copy Markdown
Contributor Author

weizijun commented Nov 16, 2021

@elasticmachine update branch

@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Nov 16, 2021

For my own education I reproduced this:

curl -uelastic:password -HContent-Type:application/json -XPUT localhost:9200/foo?pretty -d'{
  "settings": {
    "index": {
      "mode": "time_series",
      "routing_path": "dim"
    }
  },
  "mappings": {
    "properties": {
      "@timestamp": {
        "type": "date",
        "meta": {
          "m": "words words words"
        }
      },
      "dim": {
        "type": "keyword",
        "time_series_dimension": true
      }
    }
  }
}'


curl -uelastic:password localhost:9200/foo/_mappings?pretty
# meta is present

curl -uelastic:password -HContent-Type:application/json -XPOST localhost:9200/foo/_doc?pretty -d'{
  "@timestamp": "2021-11-16T09:48:20Z",
  "dim": "a",
  "cat": "mouse"
}'

curl -uelastic:password localhost:9200/foo/_mappings?pretty
# meta is absent

nik9000
nik9000 reviewed Nov 16, 2021
View reviewed changes
server/src/internalClusterTest/java/org/elasticsearch/index/TimeSeriesModeIT.java
assertThat(getMappingsResponse.getMappings().get(index).source().string(), equalTo(Strings.toString(expect)));
}

}
Copy link
Copy Markdown
Member

@nik9000 nik9000 Nov 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd tend to want to do these as yaml rest tests instead of ESIntegTestCase. The yaml tests are much more copy and paste-ish, we run them during rolling upgrade testing and during backwards compatibility testing.

I'm not sure that it's worth rewriting it now though.

Copy link
Copy Markdown
Contributor Author

@weizijun weizijun Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine, I move the test from TimeSeriesModeTests to TimeSeriesModeIT, some tests are the same with yaml tests. Maybe later, we can remove these tests.

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java Outdated
for (Map<String, Object> mapping : mappings) {
List<Map<String, Object>> mergedMappings = new ArrayList<>();
boolean isTimeSeriesMode = indexService.getIndexSettings().getMode() != null
&& indexService.getIndexSettings().getMode() == IndexMode.TIME_SERIES;
Copy link
Copy Markdown
Member

@nik9000 nik9000 Nov 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think IndexSettings.getMode can't be null - it'd default to STANDARD.

I think I'd prefer to move this merge helping code to IndexMode too, if we can. If every behavior change in different index modes is a method on IndexMode then folks reading it can scan it for the kinds of behavior that it influences. Hard checks for a particular mode like this make that much harder.

Copy link
Copy Markdown
Contributor Author

@weizijun weizijun Nov 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine, I'm trying to move these code into IndexMode

Copy link
Copy Markdown
Contributor Author

@weizijun weizijun Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I change the code, is it ok?

nik9000
nik9000 approved these changes Nov 18, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me.

@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Nov 18, 2021

@elasticmachine, test this please

@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Nov 18, 2021

@elasticmachine update branch

@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Nov 18, 2021

@elasticmachine test this please

@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Nov 18, 2021

@weizijun the errors in the build look related to your change.

@weizijun
Copy link
Copy Markdown
Contributor Author

weizijun commented Nov 19, 2021

@weizijun the errors in the build look related to your change.

ok, I will fixed the tests

@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Nov 19, 2021

@elasticmachine, test this please

@nik9000 nik9000 requested a review from martijnvg November 19, 2021 12:30
martijnvg
martijnvg approved these changes Nov 22, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@nik9000 nik9000 merged commit 2136af2 into elastic:master Nov 22, 2021
@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Nov 22, 2021

Thanks for your patience on this one @weizijun !

@weizijun
Copy link
Copy Markdown
Contributor Author

weizijun commented Nov 22, 2021

Thank you, @nik9000 , @martijnvg !

@mark-vieira mark-vieira mentioned this pull request Nov 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nik9000 nik9000 nik9000 approved these changes

@martijnvg martijnvg martijnvg approved these changes

Assignees

No one assigned

Labels

>bug external-contributor Pull request authored by a developer outside the Elasticsearch team :StorageEngine/TSDB You know, for Metrics v8.1.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@weizijun @elasticmachine @nik9000 @martijnvg @elasticsearchmachine