Skip to content
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.

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

Keep parameters in mapping for `_timestamp` and `_size` even if disabled #7475

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
4 participants
@brwe
Copy link
Contributor

brwe commented Aug 27, 2014

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 brwe added review labels Aug 27, 2014

@jpountz

View changes

src/main/java/org/elasticsearch/cluster/metadata/MetaDataMappingService.java Outdated
@@ -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;

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 27, 2014

Contributor

With this line of code, are the lines above that also set typeIsNew to false still useful? Or could they be removed?

This comment has been minimized.

Copy link
@brwe

brwe Aug 27, 2014

Author Contributor

I thought because of the continue after typeIsNew = false; above the line 495 will never be reached.

@jpountz

View changes

src/main/java/org/elasticsearch/cluster/metadata/MetaDataMappingService.java Outdated
// pre create indices here and add mappings to them so we can merge the mappings here if needed
for (String index : request.indices()) {
if (indicesService.hasIndex(index)) {
if (indicesService.indexServiceSafe(index).mapperService().hasMapping(request.type())) {

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 27, 2014

Contributor

Mappings are modified in place, so potentially the index could be removed between the call to hasIndex and the call to indexServiceSafe.

This comment has been minimized.

Copy link
@brwe

brwe Aug 27, 2014

Author Contributor

But would that not be OK? This would trigger an IndexMissingException which I think is expected in case the index is deleted while a mapping is applied.

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 27, 2014

@brwe Just left minor comments, but the change looks great in general.

@spinscale

View changes

src/test/java/org/elasticsearch/index/mapper/size/SizeMappingIntegrationTests.java Outdated
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 {

This comment has been minimized.

Copy link
@spinscale

spinscale Aug 27, 2014

Member

fix naming here

@spinscale

View changes

src/test/java/org/elasticsearch/index/mapper/size/SizeMappingIntegrationTests.java Outdated

private void assertSizeMappingEnabled(String index, String type, boolean enabled) throws IOException {
String errMsg = String.format(Locale.ROOT, "Expected size field mapping to be " + (enabled ? "enabled" : "disabled") + " for %s/%s", index, type);
GetMappingsResponse getMappingsResponse = client().admin().indices().prepareGetMappings(index).addTypes(type).get();
Map<String, Object> mappingSource = getMappingsResponse.getMappings().get(index).get(type).getSourceAsMap();
assertThat(errMsg, mappingSource, hasKey("_size"));
String ttlAsString = mappingSource.get("_size").toString();

This comment has been minimized.

Copy link
@spinscale

spinscale Aug 27, 2014

Member

naming.. ttl

@spinscale

View changes

src/test/java/org/elasticsearch/index/mapper/size/SizeMappingIntegrationTests.java Outdated
GetMappingsResponse getMappingsResponse = client().admin().indices().prepareGetMappings(index).addTypes(type).get();
Map<String, Object> mappingSource = getMappingsResponse.getMappings().get(index).get(type).getSourceAsMap();
assertThat(errMsg, mappingSource, hasKey("_size"));
String ttlAsString = mappingSource.get("_size").toString();
assertThat(ttlAsString, is(notNullValue()));
assertThat(errMsg, ttlAsString, is("{enabled=true}"));
assertThat(errMsg, ttlAsString, is("{enabled=" + (enabled ? "true" : "false") + "}"));

This comment has been minimized.

Copy link
@spinscale

spinscale Aug 27, 2014

Member

cant you just use the enabled var here?

@spinscale

View changes

src/test/java/org/elasticsearch/index/mapper/source/DefaultSourceMappingTests.java Outdated
mapper = mapperService.documentMapper("my_type");
assertThat(mapper.type(), equalTo("my_type"));
includes = Arrays.asList(mapper.sourceMapper().includes());
assertThat("default_field_path.", isIn(includes));

This comment has been minimized.

Copy link
@spinscale

spinscale Aug 27, 2014

Member

you can just do assertThat(mapper.sourceMapper().includes(), hasItemInArray("default_field_path.")); - no need for list casting

@spinscale

View changes

src/main/java/org/elasticsearch/cluster/metadata/MetaDataMappingService.java Outdated
// pre create indices here and add mappings to them so we can merge the mappings here if needed
for (String index : request.indices()) {
if (indicesService.hasIndex(index)) {
if (indicesService.indexServiceSafe(index).mapperService().hasMapping(request.type())) {
typeIsNew = false;

This comment has been minimized.

Copy link
@spinscale

spinscale Aug 27, 2014

Member

what about typeIsNew = !indicesService.indexServiceSafe(index).mapperService().hasMapping(request.type()

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 28, 2014

Contributor

+1 I think we should not compute typeIsNew in this first loop (especially given that typeIsNew should be per-index) and move it to right before where the merge is done as spinscale suggests?

This comment has been minimized.

Copy link
@brwe

brwe Aug 28, 2014

Author Contributor

Indeed, this would have introduced a bug. Glad you spottet it! I added a test for this and moved the check, also I think we can just use "existingMapper == null" to see if the type already exists (see commit "fix default applying for multiple indices")

@spinscale

This comment has been minimized.

Copy link
Member

spinscale commented Aug 27, 2014

could the IndexFieldMapper suffer the same problem?

some minor things, but looks close

brwe added some commits Aug 26, 2014

mappings: keep parameters in mapping for _timestamp and _size even if…
… 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 brwe force-pushed the brwe:root-mapping-xcontent-inconsistency branch to bfd4f47 Aug 28, 2014

@brwe

This comment has been minimized.

Copy link
Contributor Author

brwe commented Aug 28, 2014

@spinscale I think IndexFieldMapper has the same problem. Will fix that also.

fix _index field
_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.
@brwe

This comment has been minimized.

Copy link
Contributor Author

brwe commented Aug 28, 2014

ok, fixed IndexFieldMapper. Also fixed the default template applying. Thanks for spotting it!

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 28, 2014

LGTM

@jpountz jpountz removed the review label Aug 28, 2014

@brwe

This comment has been minimized.

Copy link
Contributor Author

brwe commented Aug 29, 2014

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).
pushing to 1.2 will be even trickier because 1.2 does not have the ElasticsearchSingleNodesTest
will take a closer look on monday

@brwe

This comment has been minimized.

Copy link
Contributor Author

brwe commented Sep 1, 2014

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

@brwe

This comment has been minimized.

Copy link
Contributor Author

brwe commented Sep 1, 2014

Pushed 9750375 (master), 3237714 and 753c4ad (1.2), 3ad63a4 (1.3) and 24bb3b1(1.x)

@brwe brwe closed this Sep 1, 2014

@clintongormley clintongormley changed the title mappings: keep parameters in mapping for _timestamp and _size even if di... Mapping: Keep parameters in mapping for `_timestamp` and `_size` even if disabled Sep 8, 2014

@clintongormley clintongormley changed the title Mapping: Keep parameters in mapping for `_timestamp` and `_size` even if disabled Keep parameters in mapping for `_timestamp` and `_size` even if disabled Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.