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

Reply via email to