Mappings: Remove ability to set path for _id and _routing on 2.0+ indexes #9623
Conversation
|
@rjernst I like the cleanup! If I recall correctly, setting the version is a way of the fake the index version. So maybe try this out? |
|
@martijnvg That worked! I've added back a couple of the tests so there is some direct testing of backcompat here. |
| @@ -144,6 +146,8 @@ protected IdFieldMapper(String name, String indexName, float boost, FieldType fi | |||
| super(new Names(name, indexName, indexName, name), boost, fieldType, docValues, Lucene.KEYWORD_ANALYZER, | |||
| Lucene.KEYWORD_ANALYZER, postingsProvider, docValuesProvider, null, null, fieldDataSettings, indexSettings); | |||
| this.path = path; | |||
| this.writePre20Settings = indexSettings.equals(ImmutableSettings.EMPTY) == false && | |||
There was a problem hiding this comment.
Just wondering, but I don't think that the indexSettings.equals(ImmutableSettings.EMPTY) == false part of the expression is needed?
There was a problem hiding this comment.
It is, because Version.indexCreated throws an exception if it cannot find the version setting (and the default constructor above uses empty settings).
There was a problem hiding this comment.
Right I see... I think the Version.indexCreated should just return null in that case, but that is outside of the scope of this change.
There was a problem hiding this comment.
@rjernst It looks like we can easily make this constructor take some index settings. It is only called from DocumentMapper.Builder which has index settings available (btw some other mappers already take settings).
There was a problem hiding this comment.
Since all 1.x versions will have the created version in the index settings, the empty settings check can be removed.
|
LGTM! |
| @@ -331,8 +359,7 @@ public void testRequiredRoutingWithPathNumericType() throws Exception { | |||
| assertThat(client().prepareGet(indexOrAlias(), "type1", "1").setRouting("0").execute().actionGet().isExists(), equalTo(true)); | |||
| } | |||
| } | |||
|
|
|||
| @Test | |||
|
LGTM too but can you please fix the |
…exes _id and _routing now no longer support the 'path' setting on indexes created with 2.0. Indexes created before 2.0 still support this setting for backcompat. closes elastic#6730
clintongormley
added
>breaking
:Search Foundations/Mapping
closes #6730
Note: I would like to keep the BulkTests checking pre-parsing when path is set works, but I don't see a way without moving that to the static backcompat tests. It would be nice if we could fake out the index settings (ie set the index_created_version) but I don't think that is possible...