jpountz commented on code in PR #13627:
URL: https://github.com/apache/lucene/pull/13627#discussion_r1706129153
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriter.java:
##########
@@ -430,10 +430,16 @@ long updateDocuments(
}
flushingDWPT = flushControl.doAfterDocument(dwpt);
} finally {
- if (dwpt.isFlushPending() || dwpt.isAborted()) {
- dwpt.unlock();
- } else {
- perThreadPool.marksAsFreeAndUnlock(dwpt);
+ // If a flush is occurring, we don't want to allow this dwpt to be reused
+ // If it is aborted, we shouldn't allow it to be reused
+ // If the deleteQueue is advanced, this means the maximum seqNo has been
set and it cannot be
+ // reused
+ synchronized (flushControl) {
Review Comment:
I played with luceneutil's IndexGeoNames which I've found historically found
good at identifying contention at index time. It goes from ~17s (30 threads) to
~20s on my machine, which I suspect is due to this new lock. If it was needed,
it would make sense to me (who cares about performance if it's incorrect) but I
suspect that it is only needed here to make the assertion in
`marksAsFreeAndUnlock` happy?
If I read your previous comments correctly, the point of this assertion is
that we cannot write a good test now that the check moved from
`DocumentsWriterPerThreadPool` to `DocumentsWriter`. This makes me wonder if we
should move all checks (`dwpt.isFlushPending() || dwpt.isAborted() ||
dwpt.isQueueAdvanced()`) to `DocumentsWriterPerThreadPool` for testability
purposes. (Sorry, I realize that this is the opposite from my previous
suggestion.)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
