-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Parse and validate mappings on index template creation #8802
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
Conversation
client().admin().indices().preparePutTemplate("template_1") | ||
.setTemplate("*") | ||
.setSettings( | ||
" {\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: align this line with the strings below?
@johtani I left some comments. |
Any news on this one? I just ran into the situation that testBrokenMapping test generates. Basically after a broken template like in the test is added to the cluster the global cluster state becomes unserializable, which means |
@rjernst does this issue become any easier with the changes you've made to mappings? |
Unfortunately not. Until mapping parsing is completely isolated from binding to a particular index (eg keeping an analyzer name, instead of looking up an actual analyzer and storing that), we will not be able to do this cleanly. Note that eventually this should be possible, as we move towards immutable mappings. However, IIRC, this PR was done by creating a temporary index, which we already do in at least one other place. As long as we are careful with deleting the index (my last comments on this PR), I think this can work as an intermediate solution. |
3bb487a
to
e8c9a36
Compare
e8c9a36
to
ee8104e
Compare
} | ||
} | ||
|
||
public void merge(Map<String, Map<String, Object>> mappings, boolean updateAllTypes) throws MapperParsingException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you leave a "TODO: make this atomic"?
I left some comments but other than that it looks good to me now! |
3230543
to
1a5c253
Compare
@jpountz I rebased and fixed your comment. Could you review again? |
LGTM |
… puts Share applying template with MetaDataCreateIndexService and MetaDataIndexTemplateService Add some unit test Collapse addMappingsToMapperService and move it to MapperService Extract validateTemplate method use expectThrows in testcase Add TODO comment Closes elastic#2415
1a5c253
to
fd76291
Compare
@seang-es Sorry for late reply, I missed your comment. |
… puts Share applying template with MetaDataCreateIndexService and MetaDataIndexTemplateService Add some unit test Collapse addMappingsToMapperService and move it to MapperService Extract validateTemplate method use expectThrows in testcase Add TODO comment Related elastic#2415 Backport elastic#8802 (cherry picked from commit fd76291)
… puts Share applying template with MetaDataCreateIndexService and MetaDataIndexTemplateService Add some unit test Collapse addMappingsToMapperService and move it to MapperService Extract validateTemplate method use expectThrows in testcase Add TODO comment Related #2415 Backport #8802 (cherry picked from commit fd76291)
Parse and validate mappings in template when it puts.
Close #2415