mikemccand commented on PR #633:
URL: https://github.com/apache/lucene/pull/633#issuecomment-1145016933
> > I could either wrap the runningMerges update with a synchronized
(IndexWriter.this) {}, or make runningMerges a synchronizedSet. I like the
second approach as it automatically fixes this at all other places.
>
> I decided to go with the first approach of wrapping critical sections with
a `synchronized (IndexWriter.this) {}`. This feels simpler to reason about as
we're synchronizing on a single object - `IndexWriter.this`.
>
> A `runningMerges` synchronizedSet, would've created three different
objects that were getting locked at different places - the IndexWriter object,
the synchronizedSet lock, and the AddIndexesMergeSource, which has sync
functions so that running and pending merge queues can be transactionally
updated. Reasoning about, and avoiding deadlocks is simpler with a single IW
lock. And given the nature of these critical sections, I don't think this
affects concurrency by much.
>
> Also rebased on the latest main.
Thanks @vigyasharma -- I'll try re-beasting. This is on the nightly Lucene
benchmarking box (128 cores).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
