mikemccand commented on a change in pull request #1772:
URL: https://github.com/apache/lucene-solr/pull/1772#discussion_r475177874



##########
File path: 
lucene/core/src/test/org/apache/lucene/index/TestIndexWriterDelete.java
##########
@@ -300,10 +300,11 @@ public void testDeleteAllSimple() throws IOException {
     modifier.close();
     dir.close();
   }
-  
+
   public void testDeleteAllNoDeadLock() throws IOException, 
InterruptedException {
     Directory dir = newDirectory();
-    final RandomIndexWriter modifier = new RandomIndexWriter(random(), dir); 
+    final RandomIndexWriter modifier = new RandomIndexWriter(random(), dir,
+        newIndexWriterConfig().setMergePolicy(new 
MockRandomMergePolicy(random())));

Review comment:
       Nice -- this forced `merge-on-getReader/commit` to be used more often in 
this test?

##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexWriter.java
##########
@@ -2427,7 +2426,13 @@ public long deleteAll() throws IOException {
           synchronized (this) {
             try {
               // Abort any running merges
-              abortMerges();
+              try {
+                abortMerges();
+                assert merges.areEnabled() == false : "merges should be 
disabled - who enabled them?";
+                assert mergingSegments.isEmpty() : "found merging segments but 
merges are disabled: " + mergingSegments;
+              } finally {
+                merges.enable();

Review comment:
       Aha, this is the meat of the change!  We need to be able to stop all 
running merges, but not prevent new merges.
   
   Maybe add a comment explaining this?  The code looks a little odd to 
`abortMerges` to only then `merges.enable()` in a `finally` clause.  The 
`finally` clause is necessary here because on exception we want to be certain 
to re-enable merges?  An exception during `abortMerges` is not tragic for `IW` 
right?  I.e. it can resume operations, and maybe not all docs were deleted?




----------------------------------------------------------------
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.

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