Load FieldInfos from store if not yet initialised through a refresh on IndexShard #125650
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If we don't yet have a cached value ready to go for the field infos, lets load it from the directory. Fixes elastic#125383
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
|
Hi @original-brownbear, I've created a changelog YAML for you. |
piergm
approved these changes
Mar 26, 2025
| 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
approved these changes
Mar 26, 2025
|
|
||
| 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! |
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Mar 26, 2025
…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
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Mar 26, 2025
…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
elasticsearchmachine
pushed a commit
that referenced
this pull request
Mar 26, 2025
…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
elasticsearchmachine
pushed a commit
that referenced
this pull request
Mar 26, 2025
…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. |
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Mar 27, 2025
…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
|
incoming in #125762 :) |
original-brownbear
added a commit
that referenced
this pull request
Mar 27, 2025
…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
javanna
reviewed
Mar 27, 2025
|
|
||
| // 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 :)
omricohenn
pushed a commit
to omricohenn/elasticsearch
that referenced
this pull request
Mar 28, 2025
…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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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