s1monw commented on code in PR #12205:
URL: https://github.com/apache/lucene/pull/12205#discussion_r1138779734
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriterFlushControl.java:
##########
@@ -604,7 +620,9 @@ long markForFullFlush() {
* blocking indexing.*/
pruneBlockedQueue(flushingQueue);
assert assertBlockedFlushes(documentsWriter.deleteQueue);
+ assert numQueued == flushQueue.size();
Review Comment:
can you please add a message to the assert. it's very hard to reason if that
fails what the actual values were and since this is concurrency it might be
crucial.
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriterFlushControl.java:
##########
@@ -437,11 +438,18 @@ public String toString() {
}
DocumentsWriterPerThread nextPendingFlush() {
+ if (numQueued == 0 && numPending == 0) {
+ // Common case, avoid taking a lock.
Review Comment:
It is really hard to reason about concurrency with these volatile reads /
writes. I can see how this all works with the queue design we use in IW / DW
but it deserves some more comments. We should really tell why we do these extra
cases and why it works to avoid changes that break common behavior. it's
crucial that we state that we will ensure queue draining after we added to the
queue ie. in the full flush case otherwise we might miss pending flushes.
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriterFlushControl.java:
##########
@@ -634,7 +652,9 @@ private void pruneBlockedQueue(final
DocumentsWriterDeleteQueue flushingQueue) {
iterator.remove();
addFlushingDWPT(blockedFlush);
// don't decr pending here - it's already done when DWPT is blocked
+ assert numQueued == flushQueue.size();
Review Comment:
same here
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriterFlushControl.java:
##########
@@ -634,7 +652,9 @@ private void pruneBlockedQueue(final
DocumentsWriterDeleteQueue flushingQueue) {
iterator.remove();
addFlushingDWPT(blockedFlush);
// don't decr pending here - it's already done when DWPT is blocked
+ assert numQueued == flushQueue.size();
Review Comment:
another question, why are we not incrementing the numQueued for every
blocked flush? I think this would be more intuitive... the assert is still good
--
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]
