Keep parameters in mapping for _timestamp and _size even if disabled
#7475
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -488,6 +492,7 @@ public ClusterState execute(final ClusterState currentState) throws Exception { | |||
| // only add the current relevant mapping (if exists) | |||
| if (indexMetaData.mappings().containsKey(request.type())) { | |||
| indexService.mapperService().merge(request.type(), indexMetaData.mappings().get(request.type()).source(), false); | |||
| typeIsNew = false; | |||
There was a problem hiding this comment.
With this line of code, are the lines above that also set typeIsNew to false still useful? Or could they be removed?
There was a problem hiding this comment.
I thought because of the continue after typeIsNew = false; above the line 495 will never be reached.
|
@brwe Just left minor comments, but the change looks great in general. |
| private void assertSizeMappingEnabled(String index, String type) throws IOException { | ||
| String errMsg = String.format(Locale.ROOT, "Expected size field mapping to be enabled for %s/%s", index, type); | ||
| @Test | ||
| public void testThatTimestampCanBeSwitchedOnAndOff() throws Exception { |
|
could the some minor things, but looks close |
… disabled
Settings that are not default for _size and _timestamp were only build in
toXContent if these fields were actually enabled.
_timestamp and _size can be dynamically enabled or disabled.
Therfore the settings must be kept, even if the field is disabled.
(Dynamic enabling/disabling was intended, see TimestampFieldMapper.merge(..)
and SizeMappingTests#testThatDisablingWorksWhenMerging
but actually never worked, see below).
To avoid that _timestamp is overwritten by a default mapping
this commit also adds a check to mapping merging if the type is already
in the mapping. In this case the default is not applied anymore.
(see
SimpleTimestampTests#testThatUpdatingMappingShouldNotRemoveTimestampConfiguration)
As a side effect, this fixes
- overwriting of paramters from the _source field by default mappings
(see DefaultSourceMappingTests).
- dynamic enabling and disabling of _timestamp and _size ()
(see SimpleTimestampTests#testThatTimestampCanBeSwitchedOnAndOff and
SizeMappingIntegrationTests#testThatTimestampCanBeSwitchedOnAndOff )
Tests:
Enable UpdateMappingOnClusterTests#test_doc_valuesInvalidMappingOnUpdate again
The missing settings in the mapping for _timestamp and _size caused a the
failure: When creating a mapping which has settings other than default and the
field disabled, still empty field mappings were built from the type mappers.
When creating such a mapping, the mapping source on master and the rest of the cluster
can be out of sync for some time:
1. Master creates the index with source _timestamp:{_store:true}
mapper classes are in a correct state but source is _timestamp:{}
2. Nodes update mapping and refresh source which then completely misses _timestamp
3. After a while source is refreshed again also on master and the _timestamp:{}
vanishes there also.
The test UpdateMappingOnCusterTests#test_doc_valuesInvalidMappingOnUpdate failed
because the cluster state was sampled from master between 1. and 3. because the
randomized testing injected a default mapping with disabled _size and _timestamp
fields that have settings which are not default.
The test
TimestampMappingTests#testThatDisablingFieldMapperDoesNotReturnAnyUselessInfo
must be removed because it actualy expected the timestamp to remove
parameters when it was disabled.
brwe
force-pushed
the
root-mapping-xcontent-inconsistency
branch
from
26215d1
to
bfd4f47
Compare
|
@spinscale I think IndexFieldMapper has the same problem. Will fix that also. |
_index had the same problems as the _timestamp and _size field. In addition, it always appeared to have no doc_values although you can set them.
|
ok, fixed IndexFieldMapper. Also fixed the default template applying. Thanks for spotting it! |
|
LGTM |
|
pushing to 1.3 is hard - I get many merge conflicts and am unsure about possible side effects of missing commits (current status here brwe@35819ac). |
|
Managed to pick in 1.2 also and think now that changes are isolated enough. Will push to master, 1.x, 1.3 and 1.2 |
clintongormley
changed the title
_timestamp and _size even if disabled
clintongormley
changed the title
_timestamp and _size even if disabled_timestamp and _size even if disabled
clintongormley
added
the
:Search/Mapping
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Settings that are not default for _size and _timestamp were only build in
toXContent if these fields were actually enabled.
_timestamp and _size can be dynamically enabled or disabled.
Therfore the settings must be kept, even if the field is disabled.
(Dynamic enabling/disabling was intended, see TimestampFieldMapper.merge(..)
and SizeMappingTests#testThatDisablingWorksWhenMerging
but actually never worked, see below).
To avoid that _timestamp is overwritten by a default mapping
this commit also adds a check to mapping merging if the type is already
in the mapping. In this case the default is not applied anymore.
(see
SimpleTimestampTests#testThatUpdatingMappingShouldNotRemoveTimestampConfiguration)
As a side effect, this fixes
(see DefaultSourceMappingTests).
(see SimpleTimestampTests#testThatTimestampCanBeSwitchedOnAndOff and
SizeMappingIntegrationTests#testThatTimestampCanBeSwitchedOnAndOff )
Tests:
Enable UpdateMappingOnClusterTests#test_doc_valuesInvalidMappingOnUpdate again
The missing settings in the mapping for _timestamp and _size caused a the
failure: When creating a mapping which has settings other than default and the
field disabled, still empty field mappings were built from the type mappers.
When creating such a mapping, the mapping source on master and the rest of the cluster
can be out of sync for some time:
mapper classes are in a correct state but source is _timestamp:{}
vanishes there also.
The test UpdateMappingOnCusterTests#test_doc_valuesInvalidMappingOnUpdate failed
because the cluster state was sampled from master between 1. and 3. because the
randomized testing injected a default mapping with disabled _size and _timestamp
fields that have settings which are not default.
The test
TimestampMappingTests#testThatDisablingFieldMapperDoesNotReturnAnyUselessInfo
must be removed because it actualy expected the timestamp to remove
parameters when it was disabled.