Avoid modify merge state in per field mergers #13208
Conversation
jpountz
left a comment
jpountz
left a comment
There was a problem hiding this comment.
Thanks Nhat, I had completely forgotten about the fact that we'd mutate the MergeState in-place at merge time, this is scary! I like your change better than the current approach, regardless of the concurrency issue.
| private final FieldInfos orgMergeFieldInfos; | ||
| private final FieldInfos[] orgFieldInfos; | ||
| private final FieldsProducer[] orgFieldsProducers; | ||
| private final MergeState copy; |
There was a problem hiding this comment.
Should we dynamically allocate this copy in apply() so that each caller gets an independent instance, and we're less subject to a similar kind of bug in the future due to multiple threads trying to modify this copy at the same time?
| import org.apache.lucene.codecs.PointsReader; | ||
| import org.apache.lucene.codecs.StoredFieldsReader; | ||
| import org.apache.lucene.codecs.TermVectorsReader; | ||
| import org.apache.lucene.codecs.*; |
There was a problem hiding this comment.
We prefer to avoid star imports.
There was a problem hiding this comment.
@jpountz Sorry, I haven't changed the imports setting for my new Intellij setup.
| * @param fieldInfos the new field infos | ||
| * @param fieldsProducers the new field producers | ||
| */ | ||
| public MergeState( |
There was a problem hiding this comment.
I'm not too happy with this constructor being public since it's really an implementation detail of PerFieldMergeState . At the same time, I like having the per-field-specific logic in the .perfield package rather than here.
I'm not sure how to fix this cleanly... my best idea at this point would be to store all parameters of the main MergeState constructor on the MergeState object, so that PerFieldMergeState can then call this constructor again, wrapping codec readers with a FilterCodecReader that only exposes a subset of fields?
There was a problem hiding this comment.
I had something different in mind but I realize it wouldn't work without re-computing merge instances over and over again, which we probably don't want. Your approach looks better.
|
I just tested this PR on the nightly benchy and indeed it fixes the merge exception that was blocking the nightly high-cardinality facets test! Yay, thanks @dnhatn! |
|
@jpountz @mikemccand Thank you! |
The PerFieldDocValuesFormat and PerFieldPostingsFormat mutate and reset the fieldInfos of the mergeState during merges. Consequently, other running merge sub-tasks may fail to see some fieldInfos. This was problematic since we introduced concurrency for merge sub-tasks. Relates #13190
The PerFieldDocValuesFormat and PerFieldPostingsFormat mutate and reset the fieldInfos of the mergeState during merges. Consequently, other running merge sub-tasks may fail to see some fieldInfos. This was problematic since we introduced concurrency for merge sub-tasks. Relates #13190
|
Thank you @dnhatn! I've kicked off a one-off nightly benchy run ... |
4 participants
The PerFieldDocValuesFormat and PerFieldPostingsFormat mutate and reset the fieldInfos of the mergeState during merges. Consequently, other running merge sub-tasks may fail to see some fieldInfos. This was problematic since we introduced concurrency for merge sub-tasks.
Relates #13190