Introduce mapping version to index metadata by jasontedor · Pull Request #33147 · elastic/elasticsearch

jasontedor · 2018-08-25T13:25:14Z

This commit introduces mapping version to index metadata. This value is monotonically increasing and is updated on mapping updates. This will be useful in cross-cluster replication so that we can request mapping updates from the leader only when there is a mapping update as opposed to the strategy we employ today which is to request a mapping update any time there is an index metadata update. As index metadata updates can occur for many reasons other than mapping updates, this leads to some unnecessary requests and work in cross-cluster replication.

elasticmachine · 2018-08-25T13:25:15Z

Pinging @elastic/es-core-infra

s1monw

I left 2 comments and LGTM. I do wonder if we can add an assertion somewhere in our code that ensures that when we update a mapping and the version is different it's source is different too? Same goes for if the version doesn't change ie in MapperService#updateMapping()` ?

s1monw · 2018-08-25T18:59:19Z

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


        this.index = index;
        this.version = version;
+        this.mappingVersion = mappingVersion;


can we assert that mappingVersion is positive here?

I pushed ad6a048.

s1monw · 2018-08-25T19:01:10Z

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

                    } else if (KEY_VERSION.equals(currentFieldName)) {
                        builder.version(parser.longValue());
+                    } else if (KEY_MAPPING_VERSION.equals(currentFieldName)) {
+                        builder.mappingVersion(parser.longValue());


can we somehow make sure that if the index is 7.0 that the version is present? I really want to make sure we are not missing it.

I pushed e1c6fe6.

Let me know what you think of that one @s1monw.

martijnvg

This looks good. I left one question about where to maintain the mapping version.

martijnvg · 2018-08-26T05:59:11Z

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

            index = in.readString();
            routingNumShards = in.readInt();
            version = in.readLong();
+            if (in.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) {


I assume when this is backported the version will be changed to 6.5.0?

If we set the version to 6.5.0 now then the BWC tests will fail.

martijnvg · 2018-08-26T06:07:19Z

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

    private final Index index;
    private final long version;
+
+    private final long mappingVersion;


I wonder whether MappingMetaData would be a better place to maintain the mapping version. The reason it feels like a better place to me is that this version is about an instance of MappingMetaData.

I considered this but it ends up that we construct mapping metadata in quite a few places where having the mapping version is not natural. We do not hold on to the mapping metadata in the mapping service, so in some of these places it means we would have to hold on to the mapping version in the mapper service. But then we would have multiple places in the system carrying the mapping version. Because of this I considered the index metadata to be better. Additionally, like that we have the builder for the index metadata. Let me know if you feel strongly about this, or see a good way to have it on the mapping metadata.

I don't feel strongly about this. I was just wondering about this. Your explanation is a good reason why to add the version to IndexMetadata instead of MappingMetaData.

* master: Fix a mappings update test (elastic#33146) Reload Secure Settings REST specs & docs (elastic#32990) Refactor CachingUsernamePassword realm (elastic#32646) Add proxy support to RemoteClusterConnection (elastic#33062)

jasontedor · 2018-08-26T17:16:09Z

I do wonder if we can add an assertion somewhere in our code that ensures that when we update a mapping and the version is different it's source is different too? Same goes for if the version doesn't change ie in MapperService#updateMapping()` ?

@s1monw I pushed 0a6f7c9 for this. Let me know what you think. Note that this assertion will fail without #33153, the act of adding the assertion uncovered that bug so I will need that PR integrated and merged into this branch before I can get a green build here.

* master: Do not lose default mapper on metadata updates (elastic#33153)

martijnvg

LGTM

This reverts commit 4ae4bd2.

s1monw

LGTM thanks for the extra iterations.

dnhatn

LGTM

This commit introduces mapping version to index metadata. This value is monotonically increasing and is updated on mapping updates. This will be useful in cross-cluster replication so that we can request mapping updates from the leader only when there is a mapping update as opposed to the strategy we employ today which is to request a mapping update any time there is an index metadata update. As index metadata updates can occur for many reasons other than mapping updates, this leads to some unnecessary requests and work in cross-cluster replication.

* master: Adjust BWC version on mapping version Token API supports the client_credentials grant (#33106) Build: forked compiler max memory matches jvmArgs (#33138) Introduce mapping version to index metadata (#33147) SQL: Enable aggregations to create a separate bucket for missing values (#32832) Fix grammar in contributing docs SECURITY: Fix Compile Error in ReservedRealmTests (#33166) APM server monitoring (#32515) Support only string `format` in date, root object & date range (#28117) [Rollup] Move toBuilders() methods out of rollup config objects (#32585) Fix forbiddenapis on java 11 (#33116) Apply publishing to genreate pom (#33094) Have circuit breaker succeed on unknown mem usage Do not lose default mapper on metadata updates (#33153) Fix a mappings update test (#33146) Reload Secure Settings REST specs & docs (#32990) Refactor CachingUsernamePassword realm (#32646)

* 6.x: Introduce mapping version to index metadata (#33147) Move non duplicated actions back into xpack core (#32952) HLRC: Create server agnostic request and response (#32912) Build: forked compiler max memory matches jvmArgs (#33138) * Added breaking change section for GROUP BY behavior: now it considers null or empty values as a separate group/bucket. Previously, they were ignored. * This is part of backporting of #32832 SQL: Enable aggregations to create a separate bucket for missing values (#32832) [TEST] version guard for reload rest-api-spec Fix grammar in contributing docs APM server monitoring (#32515) Support only string `format` in date, root object & date range (#28117) [Rollup] Move toBuilders() methods out of rollup config objects (#32585) Accept Gradle build scan agreement (#30645) Fix forbiddenapis on java 11 (#33116) Run forbidden api checks with runtimeJavaVersion (#32947) Apply publishing to genreate pom (#33094) Fix a mappings update test (#33146) Reload Secure Settings REST specs & docs (#32990) Refactor CachingUsernamePassword realm (#32646)

jasontedor added >enhancement review :Data Management/Indices APIs DO NOT USE. Use ":Distributed/Indices APIs" or ":StorageEngine/Templates" instead. v7.0.0 v6.5.0 labels Aug 25, 2018

jasontedor requested review from dnhatn, martijnvg and s1monw August 25, 2018 13:25

Remove import

f9e3f56

s1monw approved these changes Aug 25, 2018

View reviewed changes

jasontedor added 4 commits August 25, 2018 15:09

Add assertion

ad6a048

Add missing serialization

755d4a5

Assert mapping version present

e1c6fe6

Fix toString

e7cdfd2

martijnvg reviewed Aug 26, 2018

View reviewed changes

jasontedor added 2 commits August 26, 2018 09:46

Merge branch 'master' into mapping-version

41260ef

* master: Fix a mappings update test (elastic#33146) Reload Secure Settings REST specs & docs (elastic#32990) Refactor CachingUsernamePassword realm (elastic#32646) Add proxy support to RemoteClusterConnection (elastic#33062)

Add assertion

0a6f7c9

Merge branch 'master' into mapping-version

e69ffe5

* master: Do not lose default mapper on metadata updates (elastic#33153)

martijnvg approved these changes Aug 27, 2018

View reviewed changes

jasontedor added 4 commits August 26, 2018 21:30

Tracking mapping updates per index

7aabd21

Disable assertions sometimes

4ae4bd2

Revert "Disable assertions sometimes"

e763004

This reverts commit 4ae4bd2.

Disable assertions based on index version

b577264

s1monw approved these changes Aug 27, 2018

View reviewed changes

dnhatn approved these changes Aug 27, 2018

View reviewed changes

jasontedor merged commit 2aef7e0 into elastic:master Aug 27, 2018

jasontedor added the backport pending label Aug 27, 2018

jasontedor deleted the mapping-version branch August 27, 2018 18:02

jasontedor removed the backport pending label Aug 27, 2018

jasontedor mentioned this pull request Aug 27, 2018

Only fetch mapping updates when necessary #33182

Merged

Mpdreamz mentioned this pull request Dec 13, 2018

[meta] 6.5.0 Release elastic/elasticsearch-net#3457

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

martijnvg mentioned this pull request Sep 3, 2019

ES: Failed to read local state, exiting... #46252

Closed

Conversation

jasontedor commented Aug 25, 2018

Uh oh!

elasticmachine commented Aug 25, 2018

Uh oh!

s1monw 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!

martijnvg 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!

jasontedor commented Aug 26, 2018

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

dnhatn left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants