benwtrent commented on code in PR #13627: URL: https://github.com/apache/lucene/pull/13627#discussion_r1706831238
########## 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: All this logic is way too opaque and complicated to fully understand all the moving parts for me. I added my assertions under the assumption that the only method that called this made all those checks previously. Without the synchronizing, we will 100% put a DWPT back in for reuse even with `isFlushPending` being true. -- 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