jpountz commented on code in PR #13627:
URL: https://github.com/apache/lucene/pull/13627#discussion_r1705717977
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriterPerThreadPool.java:
##########
@@ -140,6 +140,11 @@ void marksAsFreeAndUnlock(DocumentsWriterPerThread state) {
final long ramBytesUsed = state.ramBytesUsed();
assert contains(state)
: "we tried to add a DWPT back to the pool but the pool doesn't know
about this DWPT";
+ // If we are closed we don't add the DWPT back to the free list. It has to
be a "new" DWPT
+ if (state.deleteQueue.isAdvanced()) {
Review Comment:
Also I wonder if we should rather fix it at the call site in
`DocumentsWriter`. It currently looks like this:
```java
if (dwpt.isFlushPending() || dwpt.isAborted()) {
dwpt.unlock();
} else {
perThreadPool.marksAsFreeAndUnlock(dwpt);
}
```
Since it already takes care of pending flushes and aborted segments, it
would be more consistent to check if the queue is advanced there, e.g.
something like this?
```java
if (dwpt.isFlushPending() || dwpt.isAborted() ||
dwpt.isQueueAdvanced()) {
dwpt.unlock();
} else {
perThreadPool.marksAsFreeAndUnlock(dwpt);
}
```
Relatedly, this makes me wonder if there is another way to fix the bug, by
somehow marking DWPTs for flush (which prevents them from being reused) before
advancing the queue in `DocumentsWriterFlushControl#markForFullFlush`.
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriterPerThreadPool.java:
##########
@@ -140,6 +140,11 @@ void marksAsFreeAndUnlock(DocumentsWriterPerThread state) {
final long ramBytesUsed = state.ramBytesUsed();
assert contains(state)
: "we tried to add a DWPT back to the pool but the pool doesn't know
about this DWPT";
+ // If we are closed we don't add the DWPT back to the free list. It has to
be a "new" DWPT
+ if (state.deleteQueue.isAdvanced()) {
Review Comment:
Do we need to make `DocumentsWriterDeleteQueue#isAdvanced` synchronized for
correctness? I see that writes to `DocumentsWriterDeleteQueue#advanced` are
protected but not reads via `isAdvanced`.
--
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
