New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix a mappings update test #33146

Merged

jasontedor merged 1 commit into elastic:master from jasontedor:fix-mappings-test

Aug 26, 2018

Member

jasontedor commented Aug 25, 2018

This commit fixes a mappings update test. The test is broken in the sense that it passes, but for the wrong reason. The test here is testing that if we make a mapping update but do not commit that mapping update then the mapper service still maintains the previous document mapper. This was not the case long, long ago when a mapping update would update the in-memory state before the cluster state update was committed. This test was passing, but it was passing because the mapping update was never even updated. It was never even updated because it was encountering a null pointer exception. Of course the in-memory state is not going to be updated in that case, we are simply going to end up with a failed cluster state update. Fixing that leads to another issue which is that the mapping source does not even parse so again we would, of course, end up with the in-memory state not being modified. We fix these issues, assert that the result cluster state task completed successfully, and finally that the in-memory state was not updated since we never committed the resulting cluster state.


          Fix a mappings update test

88029c0

This commit fixes a mappings update test. The test is broken in the
sense that it passes, but for the wrong reason. The test here is testing
that if we make a mapping update but do not commit that mapping update
then the mapper service still maintains the previous document
mapper. This was not the case long, long ago when a mapping update would
update the in-memory state before the cluster state update was
committed. This test was passing, but it was passing because the mapping
update was never even updated. It was never even updated because it was
encountering a null pointer exception. Of course the in-memory state is
not going to be updated in that case, we are simply going to end up with
a failed cluster state update. Fixing that leads to another issue which
is that the mapping source does not even parse so again we would, of
course, end up with the in-memory state not being modified. We fix these
issues, assert that the result cluster state task completed
successfully, and finally that the in-memory state was not updated since
we never committed the resulting cluster state.

jasontedor added >test review v7.0.0 :Distributed/Distributed v6.5.0 labels

Collaborator

elasticmachine commented Aug 25, 2018

Pinging @elastic/es-distributed

jasontedor requested review from martijnvg and s1monw

August 25, 2018 12:38

Member Author

jasontedor commented Aug 25, 2018

I discovered this while adding mapping versions to index metadata.

jasontedor commented

View reviewed changes

server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataMappingServiceTests.java

                       final ClusterService clusterService = getInstanceFromNode(ClusterService.class);
                       // TODO - it will be nice to get a random mapping generator
                       final PutMappingClusterStateUpdateRequest request = new PutMappingClusterStateUpdateRequest().type("type");
-                      request.source("{ \"properties\" { \"field\": { \"type\": \"text\" }}}");
-                      mappingService.putMappingExecutor.execute(clusterService.state(), Collections.singletonList(request));
+                      request.indices(new Index[] {indexService.index()});

Member Author

jasontedor Aug 25, 2018

This line prevents a null pointer exception when handling the request.

jasontedor commented

View reviewed changes

server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataMappingServiceTests.java

-                      request.source("{ \"properties\" { \"field\": { \"type\": \"text\" }}}");
-                      mappingService.putMappingExecutor.execute(clusterService.state(), Collections.singletonList(request));
+                      request.indices(new Index[] {indexService.index()});
+                      request.source("{ \"properties\": { \"field\": { \"type\": \"text\" }}}");

Member Author

jasontedor Aug 25, 2018 •

edited

Loading

This line adds a : after \"properties\" so that the mapping source parses properly.

Contributor

s1monw Aug 25, 2018

💥

jasontedor commented

View reviewed changes

server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataMappingServiceTests.java

-                      mappingService.putMappingExecutor.execute(clusterService.state(), Collections.singletonList(request));
+                      request.indices(new Index[] {indexService.index()});
+                      request.source("{ \"properties\": { \"field\": { \"type\": \"text\" }}}");
+                      final ClusterStateTaskExecutor.ClusterTasksResult<PutMappingClusterStateUpdateRequest> result =

Member Author

jasontedor Aug 25, 2018

This line is unchanged except now we capture the result of the cluster state update task.

jasontedor commented

View reviewed changes

server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataMappingServiceTests.java

+                      // the task really was a mapping update
+                      assertThat(
+                              indexService.mapperService().documentMapper("type").mappingSource(),
+                              not(equalTo(result.resultingState.metaData().index("test").mapping("type").source())));

Member Author

jasontedor Aug 25, 2018

The above lines assert that the cluster state update task completed successfully and that if the result cluster state were committed then the mapping for the type would change.

jasontedor commented

View reviewed changes

server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataMappingServiceTests.java

+                      assertThat(
+                              indexService.mapperService().documentMapper("type").mappingSource(),
+                              not(equalTo(result.resultingState.metaData().index("test").mapping("type").source())));
+                      // since we never committed the cluster state update, the in-memory state is unchanged
                       assertThat(indexService.mapperService().documentMapper("type").mappingSource(), equalTo(currentMapping));

Member Author

jasontedor Aug 25, 2018

We finally end with the existing assertion that the in-memory state of the mapping was not changed as a result of processing the mapping update. This is expected since the mapping update (the resulting cluster state) was never committed.

s1monw approved these changes

View reviewed changes

Contributor

s1monw left a comment

LGTM

server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataMappingServiceTests.java

-                      request.source("{ \"properties\" { \"field\": { \"type\": \"text\" }}}");
-                      mappingService.putMappingExecutor.execute(clusterService.state(), Collections.singletonList(request));
+                      request.indices(new Index[] {indexService.index()});
+                      request.source("{ \"properties\": { \"field\": { \"type\": \"text\" }}}");

Contributor

s1monw Aug 25, 2018

💥

Member Author

jasontedor commented Aug 25, 2018

@elasticmachine test this please

martijnvg approved these changes

View reviewed changes

Member

martijnvg left a comment

LGTM

Thanks for fixing and making this test better!

jasontedor merged commit f8b07a0 into elastic:master

jasontedor added a commit that referenced this pull request


          Fix a mappings update test (#33146)

73cc92d

This commit fixes a mappings update test. The test is broken in the
sense that it passes, but for the wrong reason. The test here is testing
that if we make a mapping update but do not commit that mapping update
then the mapper service still maintains the previous document
mapper. This was not the case long, long ago when a mapping update would
update the in-memory state before the cluster state update was
committed. This test was passing, but it was passing because the mapping
update was never even updated. It was never even updated because it was
encountering a null pointer exception. Of course the in-memory state is
not going to be updated in that case, we are simply going to end up with
a failed cluster state update. Fixing that leads to another issue which
is that the mapping source does not even parse so again we would, of
course, end up with the in-memory state not being modified. We fix these
issues, assert that the result cluster state task completed
successfully, and finally that the in-memory state was not updated since
we never committed the resulting cluster state.

jasontedor deleted the fix-mappings-test branch

August 26, 2018 13:42

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request


          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)

jasontedor added a commit that referenced this pull request


          Merge branch 'master' into ccr

0e5d42c

* 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)

jasontedor added a commit that referenced this pull request


          Merge branch '6.x' into ccr-6.x

6d4b637

* 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)

colings86 added v7.0.0-beta1 and removed v7.0.0 labels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed/Distributed >test v6.5.0 v7.0.0-beta1