Update dynamic fields in mapping on master even if parsing fails for the rest of the doc #9874
Conversation
brwe
added
v2.0.0-beta1
review
v1.5.0
v1.4.5
:Search/Mapping
| @@ -494,7 +494,7 @@ public ParsedDocument parse(SourceToParse source, @Nullable ParseListener listen | |||
| } catch (Throwable e) { | |||
| // if its already a mapper parsing exception, no need to wrap it... | |||
| if (e instanceof MapperParsingException) { | |||
| throw (MapperParsingException) e; | |||
| throw ((MapperParsingException) e).setMappingsModified(context.mappingsModified()); | |||
There was a problem hiding this comment.
Could we do it in such a way that we do not need to modify the incoming exception in-place?
There was a problem hiding this comment.
Not sure what you mean. I added it now in the constructor of the MapperParsingException. Is this what you meant?
There was a problem hiding this comment.
Yes! I like it much better!
|
LGTM |
|
@brwe Since this change looks quite significant, I don't think it should be pushed to 1.4.5. |
… fails for the rest of doc The local DocumentMapper is updated while parsing and dynamic fields are added before parsing has finished. If parsing fails after a dynamic field has been added already then the field was not added to the cluster state but was present in the local mapper of this node. New documents with the same field would not necessarily cause an update either and after restarting the node the mapping for these fields were lost. Instead the new fields should always be updated. closes #9851 closes #9874
|
That was not a good idea - the change of MapperParsingException seemes to have screwed up bwc for Exception serialization, see for example http://build-us-00.elasticsearch.org/job/es_bwc_1x/8385/ |
… fails for the rest of doc The local DocumentMapper is updated while parsing and dynamic fields are added before parsing has finished. If parsing fails after a dynamic field has been added already then the field was not added to the cluster state but was present in the local mapper of this node. New documents with the same field would not necessarily cause an update either and after restarting the node the mapping for these fields were lost. Instead the new fields should always be updated. closes elastic#9851 closes elastic#9874
brwe
force-pushed
the
partial-parsed-doc
branch
from
bfb8224
to
732583b
Compare
|
Ok, new plan. Instead of changing the mapper parsing exception we actually can just get the context from the document mapper and ask if the mapping was changed - much easier and I cannot see any downside to it. @jpountz would you mind taking another look? |
|
I'm not too happy with getting back to the cache to check whether mappings have been modified, I liked the previous solution better. But on ther other hand, I don't have a better idea. Can we maybe just use this approach on 1.x and keep the previous approach on master? |
|
Ok, I'll do that. |
… fails for the rest of doc The local DocumentMapper is updated while parsing and dynamic fields are added before parsing has finished. If parsing fails after a dynamic field has been added already then the field was not added to the cluster state but was present in the local mapper of this node. New documents with the same field would not necessarily cause an update either and after restarting the node the mapping for these fields were lost. Instead the new fields should always be updated. closes #9851 closes #9874
clintongormley
changed the title
...ils for the rest of doc
The local DocumentMapper is updated while parsing and dynamic fields are added before
parsing has finished. If parsing fails after a dynamic field has been added already
then the field was not added to the cluster state but was present in the local mapper of this
node. New documents with the same field would not necessarily cause an update either and
after restarting the node the mapping for these fields were lost. Instead the new fields
should always be updated.
closes #9851