Load FieldInfos from store if not yet initialised through a refresh on IndexShard#125650
Load FieldInfos from store if not yet initialised through a refresh on IndexShard#125650original-brownbear merged 6 commits intoelastic:mainfrom original-brownbear:125483
Conversation
original-brownbear
added
>bug
auto-backport
elasticsearchmachine
added
the
Team:Search Foundations
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
|
Hi @original-brownbear, I've created a changelog YAML for you. |
piergm
left a comment
piergm
left a comment
There was a problem hiding this comment.
LGTM! Thanks @original-brownbear for working on this :D
| try (Engine.Searcher hasValueSearcher = getEngine().acquireSearcher("field_has_value")) { | ||
| return FieldInfos.getMergedFieldInfos(hasValueSearcher.getIndexReader()); | ||
| } catch (AlreadyClosedException ignore) { | ||
| // engine is closed - no update to3 FieldInfos is fine |
There was a problem hiding this comment.
lol right and in the process of fixing grammar/typo no less 🤣 thanks!
andreidan
left a comment
andreidan
left a comment
There was a problem hiding this comment.
LGTM too :) Thanks for fixing this Armin
|
|
||
| static { | ||
| try { | ||
| FIELD_INFOS = MethodHandles.lookup().findVarHandle(IndexShard.class, "fieldInfos", FieldInfos.class); |
There was a problem hiding this comment.
Ah this is a bit I don't like about VarHandle, and I'd normally prefer an Atomic... for the CAS operation for readability.
However, the tradeoff here is pretty sweet as we avoid creating (a potential alternative) one Atomic... instance for every IndexShard in order to wrap the fieldInfos giving us a sweet memory saving.
Nicely done ! :)
|
Thanks you two! |
…n IndexShard (#125650) (#125703) Load field caps from store if they haven't been initialised through a refresh yet. Keep the plain reads to not mess with performance characteristics too much on the good path but protect against confusing races when loading field infos now (that probably should have been ordered stores in the first place but this was safe due to other locks/volatiles on the refresh path). Closes #125483
…n IndexShard (#125650) (#125704) Load field caps from store if they haven't been initialised through a refresh yet. Keep the plain reads to not mess with performance characteristics too much on the good path but protect against confusing races when loading field infos now (that probably should have been ordered stores in the first place but this was safe due to other locks/volatiles on the refresh path). Closes #125483
|
Thanks for fixing @original-brownbear I believe it makes sense to backport this to 8.18 as well. |
|
incoming in #125762 :) |
…n IndexShard (#125650) (#125762) Load field caps from store if they haven't been initialised through a refresh yet. Keep the plain reads to not mess with performance characteristics too much on the good path but protect against confusing races when loading field infos now (that probably should have been ordered stores in the first place but this was safe due to other locks/volatiles on the refresh path). Closes #125483
|
|
||
| // check that field_caps empty field filtering works as well | ||
| FieldCapabilitiesResponse response = client().prepareFieldCaps(mountedIndices).setFields("*").setincludeEmptyFields(false).get(); | ||
| assertNotNull(response.getField("keyword")); |
There was a problem hiding this comment.
I wonder if it makes sense to add the same test to the N-2 indices related tests for indices that are imported as verified read-only. We know the fix addresses the same issue for them too, but it'd be safer to have a specific test for them? These tests are under qa/lucene-index-compatibility .
There was a problem hiding this comment.
++ definitely. Let me try something, maybe I can get the full field caps suite running for searchable snapshots and there somehow, would be really nice to have coverage across all the implementations that play tricks on the engine :)
…n IndexShard (elastic#125650) Load field caps from store if they haven't been initialised through a refresh yet. Keep the plain reads to not mess with performance characteristics too much on the good path but protect against confusing races when loading field infos now (that probably should have been ordered stores in the first place but this was safe due to other locks/volatiles on the refresh path). Closes elastic#125483
5 participants
Load field caps from store if they haven't been initialised through a refresh yet.
Keep the plain reads to not mess with performance characteristics too much on the good path but protect against confusing races when loading field infos now (that probably should have been ordered stores in the first place but this was safe due to other locks/volatiles on the refresh path).
Closes #125483