Make SourceProvider using stored fields segment-thread-safe#95082
Make SourceProvider using stored fields segment-thread-safe#95082romseygeek merged 7 commits intoelastic:mainfrom
Conversation
|
Hi @romseygeek, I've created a changelog YAML for you. |
|
Pinging @elastic/es-search (Team:Search) |
|
In terms of synchronization cost, this adds a single volatile read to each call to |
|
This will only effect search-time lookups, btw, because the FetchPhase doesn't use this standalone SourceProvider implementation (except in some fairly unusual circumstances - so it does help, but isn't a complete solution). We'll need to add a similar per-segment cache to the ProvidedSourceProvider in FetchPhase to get that working, but that won't need any locking at all so should be simpler to implement. |
| public Source getSource(LeafReaderContext ctx, int doc) throws IOException { | ||
| LeafStoredFieldSourceProvider[] leaves = getLeavesUnderLock(ctx.parent); | ||
| if (leaves[ctx.ord] == null) { | ||
| // individual segments are only accessed on one thread so there's no need |
There was a problem hiding this comment.
This is true as far as concurrency is done at segment level. If/When inter segment concurrency is implemented, then we might need extra locking here? If this is true we might want to add it in the comment.
There was a problem hiding this comment.
If we implement within-segment concurrency then this will have to work completely differently I think. I'll add a comment though.
…' into source/segment-thread-safety
|
Quick question: does this effectively solve the concurrency issues for source based runtime fields when the search is executed concurrently across segments? |
|
@javanna it should do, yes. I remember that we muted some tests that were failing due to a lack of concurrency, but I haven't been able to find which ones they were to unmute them - @cbuescher might remember? |
|
I only remember touching concurrency with the cache cleaning around the |
|
Maybe this?: #90712 |
This adds a per-segment cache to the SourceProvider returned by calling
SourceProvider.fromStoredFields(). Previously, this source provider couldonly be safely accessed from a single thread. With this new cache, we now
have a separate saved Source for each segment, making this safe to use for
concurrent searches that still use a single thread for each segment.