throw exception if a copy_to is within a multi field #15213
Conversation
brwe
added
:Search Foundations/Mapping
|
Just found that versions are wrong. Will fix. |
|
Is it possible to remove the |
Copy to within multi field is ignored from 2.0 on, see elastic#10802. Instead of just ignoring it, we should throw an exception if this is found in the mapping when a mapping is added. For already existing indices we should at least log a warning. related to elastic#14946
brwe
force-pushed
the
copy-to-in-multi-fields-exception
branch
from
9f98c8a to
80c7cf5
Compare
|
Discussed with @clintongormley and we decided to just leave it as is as the warning will not flood logs code with log messages. Ready for review. |
| ESLoggerFactory.getLogger("mapping [" + parserContext.type() + "]").warn("Found a copy_to in field [" + name + "] which is within a multi field. This feature has been removed and the copy_to will be ignored."); | ||
| // we still parse this, otherwise the message will only appear once and the copy_to removed. After that it will appear again. Better to have it always. | ||
| } | ||
| } | ||
| parseCopyFields(propNode, builder); |
There was a problem hiding this comment.
Can you make the parseCopyFields happen in an else here? Then the setting will not be serialized back out because the multi field will never have a copyTo member.
There was a problem hiding this comment.
I can do that but it might be weird with rolling upgrades. The mapping will have the copy_to until an upgraded node becomes master and then this entry suddenly vanishes and the warning stops. Seems like an undesirable behavior.
@clintongormley and I discussed this yesterday and we thought that it is not a big deal to keep the copy_to because it will only warn whenever the mapping is changed so it will not spam the log with warnings in any case.
If one wants to get rid of the warning they can still remove it from the mapping by updating it explicitly.
There was a problem hiding this comment.
Hmm thinking about this again... If you're doing a rolling upgrade, then when the shard is assigned to a new node (as part of the upgrade) it should update the mapping and send the new mapping to the master. So we should only get this warning once (I think for the first shard?).
There was a problem hiding this comment.
The mappings will get merged on master so if the master is an old node and still has the copy_to and the new mapping without the copy_to comes in it will be merged with the copy_to from master and the resulting mapping will still have it. Only when a new node becomes master it will actually remove the copy_to.
My concern is actually not so much that the log message appears too often but more that it appears in any case at least once but then when a user sees it and checks the mapping then the copy_to might or might not be there depending on if the master was upgraded already or not.
|
Thanks @brwe! I left a few comments. |
|
Since the user can explicitly remove the troublesome |
throw exception if a copy_to is within a multi field
throw exception if a copy_to is within a multi field Copy to within multi field is ignored from 2.0 on, see #10802. Instead of just ignoring it, we should throw an exception if this is found in the mapping when a mapping is added. For already existing indices we should at least log a warning. related to #14946
throw exception if a copy_to is within a multi field Copy to within multi field is ignored from 2.0 on, see #10802. Instead of just ignoring it, we should throw an exception if this is found in the mapping when a mapping is added. For already existing indices we should at least log a warning. related to #14946
Copy to within multi field is ignored from 2.0 on, see #10802.
Instead of just ignoring it, we should throw an exception if this
is found in the mapping when a mapping is added. For already
existing indices we should at least log a warning.
related to #14946
I made it so that a warning is logged each time a mapping is parsed that has a copy_to in a multi field.
But the copy_to is not removed from the mapping so now the exception is logged each time the mapping is parsed. Is that OK?