Correct how meta-field is defined for pre 7.8 hits #57951
Conversation
We keep a static list of meta-fields: META_FIELDS_BEFORE_7_8 as it was before. This is done to ensure the backwards compatability with pre 7.8 nodes. Closes elastic#57831
mayya-sharipova
added
v7.9.0
>test
|
Pinging @elastic/es-search (:Search/Search) |
| } | ||
| // if a node had Size Plugin installed, _size field should also be considered a meta-field | ||
| return fieldName.equals("_size"); | ||
| return META_FIELDS_BEFORE_7_8.contains(fieldName); |
There was a problem hiding this comment.
If I remember correctly we only use isMetadataFieldStatic in a few places now where we read from older nodes. Should we simply make the collection public and remove the static method? Or rename method to something like "isMetaFieldBefore7dot8" or something, just to improve readability in the locations where this it is actually used.
There was a problem hiding this comment.
@cbuescher Thanks for the feedback! I have removed isMetadataFieldStatic method, and made the collection public.
…nces SearchHitsTests.testSerializationPre6_6_0 will fail if you use different meta-fields that were before
| @@ -106,7 +106,7 @@ private static DocumentField mutateDocumentField(DocumentField documentField) { | |||
| Predicate<String> excludeMetaFieldFilter) { | |||
| if (isMetafield) { | |||
| String metaField = randomValueOtherThanMany(excludeMetaFieldFilter, | |||
| () -> randomFrom(IndicesModule.getBuiltInMetadataFields())); | |||
| () -> randomFrom(MapperService.META_FIELDS_BEFORE_7DOT8)); | |||
There was a problem hiding this comment.
Just as a last question: why is this necessary? I though the "old" collection of meta field names is only used when reading from "old" nodes? Why can't we use all built-in fields here since we're already on master?
There was a problem hiding this comment.
@cbuescher I am struggling with BWC tests here, and I am not sure about the best approach.
SearchHitsTests.testSerializationPre6_6_0 tests was failing if we use IndicesModule.getBuiltInMetadataFields().
In this test we create a test SearchHit instance in 7.x and then do wire serialization to pre6_6 node and deserialization from it, and then assert that 7.x instance is equal to the deserialized instance from 6.x. The test was failing if we use IndicesModule.getBuiltInMetadataFields() and include _fied_names as a meta-field in the created test instance. As now during deserialization from pre 7.8 nodes we use META_FIELDS_BEFORE_7DOT8 to decide if a field is meta-field, we put _field_names under document fields, which will not be equal to the original created test instance, where _field_names is under meta-fields.
Probably, the best approach would be to separate how we create test instances of SearchHit between pre 7.8 nodes and after 7.8 nodes. What do you think?
There was a problem hiding this comment.
Are those particular tests needed on 7.x anymore? As far as I understand we'd never see a pre 6.6 node in a cluster running any 7x release at this point, 6.8 should be the last supported version? Maybe we can delete the test? I wonder if this uncovers other kinds of edge cases with the change though, so I'd prefer if we can sample "meta" fields from the "true" source (IndicesModule.getBuiltInMetadataFields()) and only fix tests where we need a specific pre-7.8 behaviour if really needed.
Mabye @javanna knows if SearchHitsTests.testSerializationPre6_6_0 can be removed since he added it way back...
There was a problem hiding this comment.
#36555 I guess the test (actually two tests) can be removed from 7.x given that 7.x nodes will never talk to pre 6.6 nodes.
There was a problem hiding this comment.
@javanna Thanks, this would simplify things
There was a problem hiding this comment.
@cbuescher Thanks for your suggestions. I have removed *pre6_6_0 tests, and also brought back IndicesModule.getBuiltInMetadataFields() for defining meta-fields in test items.
We keep a static list of meta-fields: META_FIELDS_BEFORE_7_8
as it was before.
This is done to ensure the backwards compatability with pre 7.8 nodes.
Relates to #57771
Closes #57831