Better logic on sending mapping update new type introduction #6669
Conversation
when an indexing request introduces a new mapping, today we rely on the parsing logic to mark it as modified on the "first" parsing phase. This can cause sending of mapping updates to master even when the mapping has been introduced in the create index/put mapping case, and can cause sending mapping updates without needing to. This bubbled up in the disabled field data format test, where we explicitly define mappings to not have the update mapping behavior happening, yet it still happens because of the current logic, and because in our test we delay the introduction of any mapping updates randomly, it can get in and override updated ones. closes elastic#6669
| @@ -89,19 +91,55 @@ public void test() throws Exception { | |||
| } | |||
| } | |||
|
|
|||
| private void updateFormat(String format) throws Exception { | |||
| private void updateFormat(final String format) throws Exception { | |||
| System.out.println(">> put mapping start " + format); | |||
There was a problem hiding this comment.
Is this intentional or just debug leftovers?
There was a problem hiding this comment.
aye, should go to logger, will change
| @@ -399,10 +400,10 @@ public DocumentMapper documentMapper(String type) { | |||
| return mappers.get(type); | |||
| } | |||
|
|
|||
| public DocumentMapper documentMapperWithAutoCreate(String type) { | |||
| public Tuple<DocumentMapper, Boolean> documentMapperWithAutoCreate(String type) { | |||
There was a problem hiding this comment.
Can you add quick documentation of what the boolean in the tuple means?
|
@jpountz applies you suggestions |
|
I just beasted |
… copies of shards
|
@jpountz that was a test failure, we need to make sure the search request ends up properly loading field data on all copies of the shards, I pushed a fix |
| resp = client().prepareSearch("test").setPreference(Integer.toString(i)).addAggregation(AggregationBuilders.terms("t").field("s") | ||
| .collectMode(aggCollectionMode)).execute().actionGet(); | ||
| assertFailures(resp); | ||
| } |
There was a problem hiding this comment.
Something I'm unhappy with is that the test will pass if any of these requests fails while I'd like to make sure that they all fail (if a SearchPhaseExecutionException is thrown)?
|
Good point. The change looks good to me now, can you just fix the handling of the SearchPhaseExecutionException and the formatting of the mappings when pushing? |
|
LGTM. I beasted it and had no failure this time. |
when an indexing request introduces a new mapping, today we rely on the parsing logic to mark it as modified on the "first" parsing phase. This can cause sending of mapping updates to master even when the mapping has been introduced in the create index/put mapping case, and can cause sending mapping updates without needing to. This bubbled up in the disabled field data format test, where we explicitly define mappings to not have the update mapping behavior happening, yet it still happens because of the current logic, and because in our test we delay the introduction of any mapping updates randomly, it can get in and override updated ones. closes #6669
clintongormley
changed the title
clintongormley
added
the
:Search/Mapping
clintongormley
changed the title
when an indexing request introduces a new mapping, today we rely on the parsing logic to mark it as modified on the "first" parsing phase. This can cause sending of mapping updates to master even when the mapping has been introduced in the create index/put mapping case, and can cause sending mapping updates without needing to.
This bubbled up in the disabled field data format test, where we explicitly define mappings to not have the update mapping behaviour happening, yet it still happens because of the current logic, and because in our test we delay the introduction of any mapping updates randomly, it can get in and override updated ones.