From 4a98918bfa2d57d35ac12c5d49e8f04fcd9791ae Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Wed, 22 Apr 2020 21:26:45 +0200 Subject: [PATCH] LUCENE-9339: Only call MergeScheduler when we actually found new merges (#1445) IW#maybeMerge calls the MergeScheduler even if it didn't find any merges we should instead only do this if there is in-fact anything there to merge and safe the call into a sync'd method. --- lucene/CHANGES.txt | 3 +++ .../lucene/index/ConcurrentMergeScheduler.java | 4 ++-- .../java/org/apache/lucene/index/IndexWriter.java | 13 +++++++------ .../org/apache/lucene/index/MergeScheduler.java | 6 ++---- .../org/apache/lucene/index/NoMergeScheduler.java | 2 +- .../apache/lucene/index/SerialMergeScheduler.java | 2 +- .../apache/lucene/TestMergeSchedulerExternal.java | 2 +- .../lucene/index/TestConcurrentMergeScheduler.java | 6 ++++-- .../apache/lucene/index/TestIndexWriterMerging.java | 2 +- .../apache/lucene/index/TestNoMergeScheduler.java | 2 +- .../lucene/index/BaseMergePolicyTestCase.java | 4 ++-- 11 files changed, 25 insertions(+), 21 deletions(-) diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt index a9301eb9f1c..02b93f64a8d 100644 --- a/lucene/CHANGES.txt +++ b/lucene/CHANGES.txt @@ -126,6 +126,9 @@ API Changes The DocumentsWriterPerThreadPool is a packaged protected final class which made it impossible to customize. (Simon Willnauer) +* LUCENE-9339: MergeScheduler#merge doesn't accept a parameter if a new merge was found anymore. + (Simon Willnauer) + New Features --------------------- (No changes) diff --git a/lucene/core/src/java/org/apache/lucene/index/ConcurrentMergeScheduler.java b/lucene/core/src/java/org/apache/lucene/index/ConcurrentMergeScheduler.java index 0324cd3a191..4eb8a654202 100644 --- a/lucene/core/src/java/org/apache/lucene/index/ConcurrentMergeScheduler.java +++ b/lucene/core/src/java/org/apache/lucene/index/ConcurrentMergeScheduler.java @@ -495,7 +495,7 @@ public class ConcurrentMergeScheduler extends MergeScheduler { } @Override - public synchronized void merge(IndexWriter writer, MergeTrigger trigger, boolean newMergesFound) throws IOException { + public synchronized void merge(IndexWriter writer, MergeTrigger trigger) throws IOException { assert !Thread.holdsLock(writer); @@ -641,7 +641,7 @@ public class ConcurrentMergeScheduler extends MergeScheduler { assert mergeThreads.contains(Thread.currentThread()) : "caller is not a merge thread"; // Let CMS run new merges if necessary: try { - merge(writer, MergeTrigger.MERGE_FINISHED, true); + merge(writer, MergeTrigger.MERGE_FINISHED); } catch (AlreadyClosedException ace) { // OK } catch (IOException ioe) { diff --git a/lucene/core/src/java/org/apache/lucene/index/IndexWriter.java b/lucene/core/src/java/org/apache/lucene/index/IndexWriter.java index d7cedda6d4e..78bed20a7dc 100644 --- a/lucene/core/src/java/org/apache/lucene/index/IndexWriter.java +++ b/lucene/core/src/java/org/apache/lucene/index/IndexWriter.java @@ -2070,7 +2070,7 @@ public class IndexWriter implements Closeable, TwoPhaseCommit, Accountable, } } - mergeScheduler.merge(this, MergeTrigger.EXPLICIT, newMergesFound); + mergeScheduler.merge(this, MergeTrigger.EXPLICIT); if (spec != null && doWait) { final int numMerges = spec.merges.size(); @@ -2152,8 +2152,9 @@ public class IndexWriter implements Closeable, TwoPhaseCommit, Accountable, final void maybeMerge(MergePolicy mergePolicy, MergeTrigger trigger, int maxNumSegments) throws IOException { ensureOpen(false); - boolean newMergesFound = updatePendingMerges(mergePolicy, trigger, maxNumSegments); - mergeScheduler.merge(this, trigger, newMergesFound); + if (updatePendingMerges(mergePolicy, trigger, maxNumSegments)) { + mergeScheduler.merge(this, trigger); + } } private synchronized boolean updatePendingMerges(MergePolicy mergePolicy, MergeTrigger trigger, int maxNumSegments) @@ -2534,7 +2535,7 @@ public class IndexWriter implements Closeable, TwoPhaseCommit, Accountable, // Give merge scheduler last chance to run, in case // any pending merges are waiting. We can't hold IW's lock // when going into merge because it can lead to deadlock. - mergeScheduler.merge(this, MergeTrigger.CLOSING, false); + mergeScheduler.merge(this, MergeTrigger.CLOSING); synchronized (this) { ensureOpen(false); @@ -3473,7 +3474,7 @@ public class IndexWriter implements Closeable, TwoPhaseCommit, Accountable, } @SuppressWarnings("try") - private final void finishCommit() throws IOException { + private void finishCommit() throws IOException { boolean commitCompleted = false; String committedSegmentsFileName = null; @@ -4381,7 +4382,7 @@ public class IndexWriter implements Closeable, TwoPhaseCommit, Accountable, testPoint("mergeMiddleStart"); merge.checkAborted(); - Directory mergeDirectory = config.getMergeScheduler().wrapForMerge(merge, directory); + Directory mergeDirectory = mergeScheduler.wrapForMerge(merge, directory); List sourceSegments = merge.segments; IOContext context = new IOContext(merge.getStoreMergeInfo()); diff --git a/lucene/core/src/java/org/apache/lucene/index/MergeScheduler.java b/lucene/core/src/java/org/apache/lucene/index/MergeScheduler.java index 66d08706080..831b3e8ce55 100644 --- a/lucene/core/src/java/org/apache/lucene/index/MergeScheduler.java +++ b/lucene/core/src/java/org/apache/lucene/index/MergeScheduler.java @@ -40,10 +40,8 @@ public abstract class MergeScheduler implements Closeable { /** Run the merges provided by {@link IndexWriter#getNextMerge()}. * @param writer the {@link IndexWriter} to obtain the merges from. - * @param trigger the {@link MergeTrigger} that caused this merge to happen - * @param newMergesFound true iff any new merges were found by the caller otherwise false - * */ - public abstract void merge(IndexWriter writer, MergeTrigger trigger, boolean newMergesFound) throws IOException; + * @param trigger the {@link MergeTrigger} that caused this merge to happen */ + public abstract void merge(IndexWriter writer, MergeTrigger trigger) throws IOException; /** * Wraps the incoming {@link Directory} so that we can merge-throttle it diff --git a/lucene/core/src/java/org/apache/lucene/index/NoMergeScheduler.java b/lucene/core/src/java/org/apache/lucene/index/NoMergeScheduler.java index e4c01366031..311f4faf7d1 100644 --- a/lucene/core/src/java/org/apache/lucene/index/NoMergeScheduler.java +++ b/lucene/core/src/java/org/apache/lucene/index/NoMergeScheduler.java @@ -42,7 +42,7 @@ public final class NoMergeScheduler extends MergeScheduler { public void close() {} @Override - public void merge(IndexWriter writer, MergeTrigger trigger, boolean newMergesFound) {} + public void merge(IndexWriter writer, MergeTrigger trigger) {} @Override public Directory wrapForMerge(OneMerge merge, Directory in) { diff --git a/lucene/core/src/java/org/apache/lucene/index/SerialMergeScheduler.java b/lucene/core/src/java/org/apache/lucene/index/SerialMergeScheduler.java index ce68247593b..74ec67a2507 100644 --- a/lucene/core/src/java/org/apache/lucene/index/SerialMergeScheduler.java +++ b/lucene/core/src/java/org/apache/lucene/index/SerialMergeScheduler.java @@ -31,7 +31,7 @@ public class SerialMergeScheduler extends MergeScheduler { * "synchronized" so that even if the application is using * multiple threads, only one merge may run at a time. */ @Override - synchronized public void merge(IndexWriter writer, MergeTrigger trigger, boolean newMergesFound) throws IOException { + synchronized public void merge(IndexWriter writer, MergeTrigger trigger) throws IOException { while(true) { MergePolicy.OneMerge merge = writer.getNextMerge(); if (merge == null) { diff --git a/lucene/core/src/test/org/apache/lucene/TestMergeSchedulerExternal.java b/lucene/core/src/test/org/apache/lucene/TestMergeSchedulerExternal.java index b6897259a93..0534172ba68 100644 --- a/lucene/core/src/test/org/apache/lucene/TestMergeSchedulerExternal.java +++ b/lucene/core/src/test/org/apache/lucene/TestMergeSchedulerExternal.java @@ -153,7 +153,7 @@ public class TestMergeSchedulerExternal extends LuceneTestCase { private static class ReportingMergeScheduler extends MergeScheduler { @Override - public void merge(IndexWriter writer, MergeTrigger trigger, boolean newMergesFound) throws IOException { + public void merge(IndexWriter writer, MergeTrigger trigger) throws IOException { OneMerge merge = null; while ((merge = writer.getNextMerge()) != null) { if (VERBOSE) { diff --git a/lucene/core/src/test/org/apache/lucene/index/TestConcurrentMergeScheduler.java b/lucene/core/src/test/org/apache/lucene/index/TestConcurrentMergeScheduler.java index f3a91ef9ad0..c93d6382b61 100644 --- a/lucene/core/src/test/org/apache/lucene/index/TestConcurrentMergeScheduler.java +++ b/lucene/core/src/test/org/apache/lucene/index/TestConcurrentMergeScheduler.java @@ -463,7 +463,8 @@ public class TestConcurrentMergeScheduler extends LuceneTestCase { public void testMaybeStallCalled() throws Exception { final AtomicBoolean wasCalled = new AtomicBoolean(); Directory dir = newDirectory(); - IndexWriterConfig iwc = newIndexWriterConfig(new MockAnalyzer(random())); + IndexWriterConfig iwc = newIndexWriterConfig(new MockAnalyzer(random())) + .setMergePolicy(new LogByteSizeMergePolicy()); iwc.setMergeScheduler(new ConcurrentMergeScheduler() { @Override protected boolean maybeStall(IndexWriter writer) { @@ -473,9 +474,10 @@ public class TestConcurrentMergeScheduler extends LuceneTestCase { }); IndexWriter w = new IndexWriter(dir, iwc); w.addDocument(new Document()); + w.flush(); + w.addDocument(new Document()); w.forceMerge(1); assertTrue(wasCalled.get()); - w.close(); dir.close(); } diff --git a/lucene/core/src/test/org/apache/lucene/index/TestIndexWriterMerging.java b/lucene/core/src/test/org/apache/lucene/index/TestIndexWriterMerging.java index 9abd2867988..321bdf7dc1b 100644 --- a/lucene/core/src/test/org/apache/lucene/index/TestIndexWriterMerging.java +++ b/lucene/core/src/test/org/apache/lucene/index/TestIndexWriterMerging.java @@ -310,7 +310,7 @@ public class TestIndexWriterMerging extends LuceneTestCase { // merging a segment with >= 20 (maxMergeDocs) docs private static class MyMergeScheduler extends MergeScheduler { @Override - synchronized public void merge(IndexWriter writer, MergeTrigger trigger, boolean newMergesFound) throws IOException { + synchronized public void merge(IndexWriter writer, MergeTrigger trigger) throws IOException { while(true) { MergePolicy.OneMerge merge = writer.getNextMerge(); diff --git a/lucene/core/src/test/org/apache/lucene/index/TestNoMergeScheduler.java b/lucene/core/src/test/org/apache/lucene/index/TestNoMergeScheduler.java index 135d33e3cbe..7db10253cbf 100644 --- a/lucene/core/src/test/org/apache/lucene/index/TestNoMergeScheduler.java +++ b/lucene/core/src/test/org/apache/lucene/index/TestNoMergeScheduler.java @@ -32,7 +32,7 @@ public class TestNoMergeScheduler extends LuceneTestCase { public void testNoMergeScheduler() throws Exception { MergeScheduler ms = NoMergeScheduler.INSTANCE; ms.close(); - ms.merge(null, RandomPicks.randomFrom(random(), MergeTrigger.values()), random().nextBoolean()); + ms.merge(null, RandomPicks.randomFrom(random(), MergeTrigger.values())); } @Test diff --git a/lucene/test-framework/src/java/org/apache/lucene/index/BaseMergePolicyTestCase.java b/lucene/test-framework/src/java/org/apache/lucene/index/BaseMergePolicyTestCase.java index 94a85dffcff..bc07bf72167 100644 --- a/lucene/test-framework/src/java/org/apache/lucene/index/BaseMergePolicyTestCase.java +++ b/lucene/test-framework/src/java/org/apache/lucene/index/BaseMergePolicyTestCase.java @@ -70,7 +70,7 @@ public abstract class BaseMergePolicyTestCase extends LuceneTestCase { final AtomicBoolean mayMerge = new AtomicBoolean(true); final MergeScheduler mergeScheduler = new SerialMergeScheduler() { @Override - synchronized public void merge(IndexWriter writer, MergeTrigger trigger, boolean newMergesFound) throws IOException { + synchronized public void merge(IndexWriter writer, MergeTrigger trigger) throws IOException { if (mayMerge.get() == false) { MergePolicy.OneMerge merge = writer.getNextMerge(); if (merge != null) { @@ -79,7 +79,7 @@ public abstract class BaseMergePolicyTestCase extends LuceneTestCase { } } - super.merge(writer, trigger, newMergesFound); + super.merge(writer, trigger); } };