You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by dn...@apache.org on 2018/07/24 16:44:39 UTC
lucene-solr:master: LUCENE-8414: Make segmentInfos private in
IndexWriter
Repository: lucene-solr
Updated Branches:
refs/heads/master d010cdd87 -> d66c05ce4
LUCENE-8414: Make segmentInfos private in IndexWriter
Today IW#SegmentInfos is package-private and can be accessed directly.
Unlike the production code which accesses the SegmentInfo with IW lock,
tests can access it without holding IW lock. This causes these tests
sometimes fail because they are checking an inconsistent view of
SegmentInfos due to a concurrent modification.
This commit makes SegmentInfos private in IndexWriter and ensures that
we snapshot SegmentInfos in our tests.
Project: http://git-wip-us.apache.org/repos/asf/lucene-solr/repo
Commit: http://git-wip-us.apache.org/repos/asf/lucene-solr/commit/d66c05ce
Tree: http://git-wip-us.apache.org/repos/asf/lucene-solr/tree/d66c05ce
Diff: http://git-wip-us.apache.org/repos/asf/lucene-solr/diff/d66c05ce
Branch: refs/heads/master
Commit: d66c05ce4f9ebe107f9589007f6899681ce2444a
Parents: d010cdd
Author: Nhat Nguyen <nh...@elastic.co>
Authored: Mon Jul 23 23:02:07 2018 -0400
Committer: Nhat Nguyen <nh...@elastic.co>
Committed: Tue Jul 24 12:39:27 2018 -0400
----------------------------------------------------------------------
lucene/CHANGES.txt | 2 ++
.../apache/lucene/index/FrozenBufferedUpdates.java | 12 ++++++------
.../java/org/apache/lucene/index/IndexWriter.java | 17 ++++++++++++++++-
.../org/apache/lucene/index/TestAddIndexes.java | 2 +-
.../apache/lucene/index/TestForceMergeForever.java | 2 +-
.../org/apache/lucene/index/TestIndexWriter.java | 16 ++++++++--------
.../apache/lucene/index/TestPerSegmentDeletes.java | 6 +++---
.../index/TestSoftDeletesRetentionMergePolicy.java | 14 +++++++-------
.../apache/lucene/index/TestTieredMergePolicy.java | 16 ++++++++--------
9 files changed, 52 insertions(+), 35 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/d66c05ce/lucene/CHANGES.txt
----------------------------------------------------------------------
diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt
index e324f2c..9757505 100644
--- a/lucene/CHANGES.txt
+++ b/lucene/CHANGES.txt
@@ -204,6 +204,8 @@ Improvements
* LUCENE-8415: Clean up Directory contracts and JavaDoc comments. (Dawid Weiss)
+* LUCENE-8414: Make segmentInfos private in IndexWriter (Simon Willnauer, Nhat Nguyen)
+
Other:
* LUCENE-8366: Upgrade to ICU 62.1. Emoji handling now uses Unicode 11's
http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/d66c05ce/lucene/core/src/java/org/apache/lucene/index/FrozenBufferedUpdates.java
----------------------------------------------------------------------
diff --git a/lucene/core/src/java/org/apache/lucene/index/FrozenBufferedUpdates.java b/lucene/core/src/java/org/apache/lucene/index/FrozenBufferedUpdates.java
index ee36cf4..1e234b7 100644
--- a/lucene/core/src/java/org/apache/lucene/index/FrozenBufferedUpdates.java
+++ b/lucene/core/src/java/org/apache/lucene/index/FrozenBufferedUpdates.java
@@ -196,18 +196,18 @@ final class FrozenBufferedUpdates {
* if the private segment was already merged away. */
private List<SegmentCommitInfo> getInfosToApply(IndexWriter writer) {
assert Thread.holdsLock(writer);
- List<SegmentCommitInfo> infos;
+ final List<SegmentCommitInfo> infos;
if (privateSegment != null) {
- if (writer.segmentInfos.indexOf(privateSegment) == -1) {
+ if (writer.segmentCommitInfoExist(privateSegment)) {
+ infos = Collections.singletonList(privateSegment);
+ }else {
if (infoStream.isEnabled("BD")) {
infoStream.message("BD", "private segment already gone; skip processing updates");
}
- return null;
- } else {
- infos = Collections.singletonList(privateSegment);
+ infos = null;
}
} else {
- infos = writer.segmentInfos.asList();
+ infos = writer.listOfSegmentCommitInfos();
}
return infos;
}
http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/d66c05ce/lucene/core/src/java/org/apache/lucene/index/IndexWriter.java
----------------------------------------------------------------------
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 3782843..ca977a9 100644
--- a/lucene/core/src/java/org/apache/lucene/index/IndexWriter.java
+++ b/lucene/core/src/java/org/apache/lucene/index/IndexWriter.java
@@ -291,7 +291,7 @@ public class IndexWriter implements Closeable, TwoPhaseCommit, Accountable,
private Collection<String> filesToCommit;
- final SegmentInfos segmentInfos; // the segments
+ private final SegmentInfos segmentInfos;
final FieldNumbers globalFieldNumberMap;
final DocumentsWriter docWriter;
@@ -5229,4 +5229,19 @@ public class IndexWriter implements Closeable, TwoPhaseCommit, Accountable,
throw new IllegalArgumentException("SegmentCommitInfo must be from the same directory");
}
}
+
+ /** Checks if the provided segment exists in the current segmentInfos */
+ final synchronized boolean segmentCommitInfoExist(SegmentCommitInfo sci) {
+ return segmentInfos.contains(sci);
+ }
+
+ /** Returns an unmodifiable view of the list of all segments of the current segmentInfos */
+ final synchronized List<SegmentCommitInfo> listOfSegmentCommitInfos() {
+ return segmentInfos.asList();
+ }
+
+ /** Tests should use this method to snapshot the current segmentInfos to have a consistent view */
+ final synchronized SegmentInfos cloneSegmentInfos() {
+ return segmentInfos.clone();
+ }
}
http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/d66c05ce/lucene/core/src/test/org/apache/lucene/index/TestAddIndexes.java
----------------------------------------------------------------------
diff --git a/lucene/core/src/test/org/apache/lucene/index/TestAddIndexes.java b/lucene/core/src/test/org/apache/lucene/index/TestAddIndexes.java
index 1307318..6b9312b 100644
--- a/lucene/core/src/test/org/apache/lucene/index/TestAddIndexes.java
+++ b/lucene/core/src/test/org/apache/lucene/index/TestAddIndexes.java
@@ -1442,7 +1442,7 @@ public class TestAddIndexes extends LuceneTestCase {
assertEquals(wrappedReader.numDocs(), writer.numDocs());
assertEquals(maxDoc, writer.maxDoc());
writer.commit();
- SegmentCommitInfo commitInfo = writer.segmentInfos.asList().get(0);
+ SegmentCommitInfo commitInfo = writer.listOfSegmentCommitInfos().get(0);
assertEquals(maxDoc-wrappedReader.numDocs(), commitInfo.getSoftDelCount());
writer.close();
Directory dir3 = newDirectory();
http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/d66c05ce/lucene/core/src/test/org/apache/lucene/index/TestForceMergeForever.java
----------------------------------------------------------------------
diff --git a/lucene/core/src/test/org/apache/lucene/index/TestForceMergeForever.java b/lucene/core/src/test/org/apache/lucene/index/TestForceMergeForever.java
index 8626fe5..c2152f0 100644
--- a/lucene/core/src/test/org/apache/lucene/index/TestForceMergeForever.java
+++ b/lucene/core/src/test/org/apache/lucene/index/TestForceMergeForever.java
@@ -70,7 +70,7 @@ public class TestForceMergeForever extends LuceneTestCase {
w.addDocument(docs.nextDoc());
}
MergePolicy mp = w.getConfig().getMergePolicy();
- final int mergeAtOnce = 1+w.segmentInfos.size();
+ final int mergeAtOnce = 1+w.listOfSegmentCommitInfos().size();
if (mp instanceof TieredMergePolicy) {
((TieredMergePolicy) mp).setMaxMergeAtOnce(mergeAtOnce);
} else if (mp instanceof LogMergePolicy) {
http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/d66c05ce/lucene/core/src/test/org/apache/lucene/index/TestIndexWriter.java
----------------------------------------------------------------------
diff --git a/lucene/core/src/test/org/apache/lucene/index/TestIndexWriter.java b/lucene/core/src/test/org/apache/lucene/index/TestIndexWriter.java
index 31740f9..85e6979 100644
--- a/lucene/core/src/test/org/apache/lucene/index/TestIndexWriter.java
+++ b/lucene/core/src/test/org/apache/lucene/index/TestIndexWriter.java
@@ -3140,7 +3140,7 @@ public class TestIndexWriter extends LuceneTestCase {
topDocs = searcher.search(new TermQuery(new Term("id", "1")), 10);
assertEquals(0, topDocs.totalHits);
int numSoftDeleted = 0;
- for (SegmentCommitInfo info : writer.segmentInfos) {
+ for (SegmentCommitInfo info : writer.cloneSegmentInfos()) {
numSoftDeleted += info.getSoftDelCount();
}
assertEquals(writer.maxDoc() - writer.numDocs(), numSoftDeleted);
@@ -3283,7 +3283,7 @@ public class TestIndexWriter extends LuceneTestCase {
}
}
int numSoftDeleted = 0;
- for (SegmentCommitInfo info : writer.segmentInfos) {
+ for (SegmentCommitInfo info : writer.cloneSegmentInfos()) {
numSoftDeleted += info.getSoftDelCount() + info.getDelCount();
}
assertEquals(writer.maxDoc() - writer.numDocs(), numSoftDeleted);
@@ -3349,7 +3349,7 @@ public class TestIndexWriter extends LuceneTestCase {
private static void assertFiles(IndexWriter writer) throws IOException {
Predicate<String> filter = file -> file.startsWith("segments") == false && file.equals("write.lock") == false;
// remove segment files we don't know if we have committed and what is kept around
- Set<String> segFiles = writer.segmentInfos.files(true).stream()
+ Set<String> segFiles = new HashSet<>(writer.cloneSegmentInfos().files(true)).stream()
.filter(filter).collect(Collectors.toSet());
Set<String> dirFiles = Arrays.stream(writer.getDirectory().listAll())
.filter(file -> !ExtrasFS.isExtra(file)) // ExtraFS might add an files, ignore them
@@ -3371,12 +3371,12 @@ public class TestIndexWriter extends LuceneTestCase {
d.add(new StringField("id", "doc-1", Field.Store.YES));
writer.addDocument(d);
writer.deleteDocuments(new Term("id", "doc-1"));
- assertEquals(1, writer.segmentInfos.asList().size());
+ assertEquals(1, writer.listOfSegmentCommitInfos().size());
writer.flush();
- assertEquals(1, writer.segmentInfos.asList().size());
+ assertEquals(1, writer.listOfSegmentCommitInfos().size());
writer.commit();
assertFiles(writer);
- assertEquals(1, writer.segmentInfos.asList().size());
+ assertEquals(1, writer.listOfSegmentCommitInfos().size());
IOUtils.close(writer, dir);
}
@@ -3437,7 +3437,7 @@ public class TestIndexWriter extends LuceneTestCase {
tombstone.add(new NumericDocValuesField("my_deletes", 1));
writer.addDocument(tombstone);
writer.flush();
- for (SegmentCommitInfo si : writer.segmentInfos) {
+ for (SegmentCommitInfo si : writer.cloneSegmentInfos()) {
FieldInfos fieldInfos = IndexWriter.readFieldInfos(si);
assertEquals("my_deletes", fieldInfos.getSoftDeletesField());
assertTrue(fieldInfos.fieldInfo("my_deletes").isSoftDeletesField());
@@ -3475,7 +3475,7 @@ public class TestIndexWriter extends LuceneTestCase {
IndexWriterConfig config = newIndexWriterConfig().setSoftDeletesField("soft_deletes_1");
IndexWriter w3 = new IndexWriter(dir3, config);
w3.addIndexes(dir1);
- for (SegmentCommitInfo si : w3.segmentInfos) {
+ for (SegmentCommitInfo si : w3.cloneSegmentInfos()) {
FieldInfo softDeleteField = IndexWriter.readFieldInfos(si).fieldInfo("soft_deletes_1");
assertTrue(softDeleteField.isSoftDeletesField());
}
http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/d66c05ce/lucene/core/src/test/org/apache/lucene/index/TestPerSegmentDeletes.java
----------------------------------------------------------------------
diff --git a/lucene/core/src/test/org/apache/lucene/index/TestPerSegmentDeletes.java b/lucene/core/src/test/org/apache/lucene/index/TestPerSegmentDeletes.java
index 6e2bd13..c1032dc 100644
--- a/lucene/core/src/test/org/apache/lucene/index/TestPerSegmentDeletes.java
+++ b/lucene/core/src/test/org/apache/lucene/index/TestPerSegmentDeletes.java
@@ -49,14 +49,14 @@ public class TestPerSegmentDeletes extends LuceneTestCase {
}
//System.out.println("commit1");
writer.commit();
- assertEquals(1, writer.segmentInfos.size());
+ assertEquals(1, writer.listOfSegmentCommitInfos().size());
for (int x = 5; x < 10; x++) {
writer.addDocument(DocHelper.createDocument(x, "2", 2));
//System.out.println("numRamDocs(" + x + ")" + writer.numRamDocs());
}
//System.out.println("commit2");
writer.commit();
- assertEquals(2, writer.segmentInfos.size());
+ assertEquals(2, writer.listOfSegmentCommitInfos().size());
for (int x = 10; x < 15; x++) {
writer.addDocument(DocHelper.createDocument(x, "3", 2));
@@ -90,7 +90,7 @@ public class TestPerSegmentDeletes extends LuceneTestCase {
fsmp.length = 2;
writer.maybeMerge();
- assertEquals(2, writer.segmentInfos.size());
+ assertEquals(2, writer.listOfSegmentCommitInfos().size());
// id:2 shouldn't exist anymore because
// it's been applied in the merge and now it's gone
http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/d66c05ce/lucene/core/src/test/org/apache/lucene/index/TestSoftDeletesRetentionMergePolicy.java
----------------------------------------------------------------------
diff --git a/lucene/core/src/test/org/apache/lucene/index/TestSoftDeletesRetentionMergePolicy.java b/lucene/core/src/test/org/apache/lucene/index/TestSoftDeletesRetentionMergePolicy.java
index fec55ac..f6c1f6c 100644
--- a/lucene/core/src/test/org/apache/lucene/index/TestSoftDeletesRetentionMergePolicy.java
+++ b/lucene/core/src/test/org/apache/lucene/index/TestSoftDeletesRetentionMergePolicy.java
@@ -392,7 +392,7 @@ public class TestSoftDeletesRetentionMergePolicy extends LuceneTestCase {
// We expect any MP to merge these segments into one segment
// when calling forceMergeDeletes.
writer.forceMergeDeletes(true);
- assertEquals(1, writer.segmentInfos.asList().size());
+ assertEquals(1, writer.listOfSegmentCommitInfos().size());
assertEquals(1, writer.numDocs());
assertEquals(1, writer.maxDoc());
writer.close();
@@ -415,7 +415,7 @@ public class TestSoftDeletesRetentionMergePolicy extends LuceneTestCase {
writer.addDocument(d);
}
writer.flush();
- assertEquals(1, writer.segmentInfos.asList().size());
+ assertEquals(1, writer.listOfSegmentCommitInfos().size());
if (softDelete != null) {
// the newly created segment should be dropped as it is fully deleted (i.e. only contains deleted docs).
@@ -443,7 +443,7 @@ public class TestSoftDeletesRetentionMergePolicy extends LuceneTestCase {
IndexReader reader = writer.getReader();
assertEquals(reader.numDocs(), 1);
reader.close();
- assertEquals(1, writer.segmentInfos.asList().size());
+ assertEquals(1, writer.listOfSegmentCommitInfos().size());
writer.close();
dir.close();
@@ -602,8 +602,8 @@ public class TestSoftDeletesRetentionMergePolicy extends LuceneTestCase {
}
}
writer.forceMergeDeletes(true);
- assertEquals(1, writer.segmentInfos.size());
- SegmentCommitInfo si = writer.segmentInfos.info(0);
+ assertEquals(1, writer.listOfSegmentCommitInfos().size());
+ SegmentCommitInfo si = writer.listOfSegmentCommitInfos().get(0);
assertEquals(0, si.getSoftDelCount()); // hard-delete should supersede the soft-delete
assertEquals(0, si.getDelCount());
assertEquals(1, si.info.maxDoc());
@@ -623,8 +623,8 @@ public class TestSoftDeletesRetentionMergePolicy extends LuceneTestCase {
doUpdate(new Term("id", "0"), writer,
new NumericDocValuesField("soft_delete", 1), new NumericDocValuesField("other-field", 1));
sm.maybeRefreshBlocking();
- assertEquals(1, writer.segmentInfos.asList().size());
- SegmentCommitInfo si = writer.segmentInfos.asList().get(0);
+ assertEquals(1, writer.listOfSegmentCommitInfos().size());
+ SegmentCommitInfo si = writer.listOfSegmentCommitInfos().get(0);
assertEquals(1, si.getSoftDelCount());
assertEquals(1, si.info.maxDoc());
IOUtils.close(sm, writer, dir);
http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/d66c05ce/lucene/core/src/test/org/apache/lucene/index/TestTieredMergePolicy.java
----------------------------------------------------------------------
diff --git a/lucene/core/src/test/org/apache/lucene/index/TestTieredMergePolicy.java b/lucene/core/src/test/org/apache/lucene/index/TestTieredMergePolicy.java
index 02a919f..a7f2111 100644
--- a/lucene/core/src/test/org/apache/lucene/index/TestTieredMergePolicy.java
+++ b/lucene/core/src/test/org/apache/lucene/index/TestTieredMergePolicy.java
@@ -181,7 +181,7 @@ public class TestTieredMergePolicy extends BaseMergePolicyTestCase {
//
// 2> We could have 10 segments and a target count of 2. In that case there could be 5 segments resulting.
// as long as they're all < 125% max seg size, that's valid.
- Iterator<SegmentCommitInfo> iterator = w.segmentInfos.iterator();
+ Iterator<SegmentCommitInfo> iterator = w.cloneSegmentInfos().iterator();
while (iterator.hasNext()) {
SegmentCommitInfo info = iterator.next();
assertTrue("No segment should be more than 125% of max segment size ",
@@ -275,7 +275,7 @@ public class TestTieredMergePolicy extends BaseMergePolicyTestCase {
w.forceMerge(Integer.MAX_VALUE);
checkSegmentsInExpectations(w, segNamesBefore, true);
- checkSegmentSizeNotExceeded(w.segmentInfos, maxSegBytes);
+ checkSegmentSizeNotExceeded(w.cloneSegmentInfos(), maxSegBytes);
// Delete 12-17% of each segment and expungeDeletes. This should result in:
@@ -285,7 +285,7 @@ public class TestTieredMergePolicy extends BaseMergePolicyTestCase {
int remainingDocs = numDocs - deletePctDocsFromEachSeg(w, random().nextInt(5) + 12, true);
w.forceMergeDeletes();
w.commit();
- checkSegmentSizeNotExceeded(w.segmentInfos, maxSegBytes);
+ checkSegmentSizeNotExceeded(w.cloneSegmentInfos(), maxSegBytes);
assertFalse("There should be no deleted docs in the index.", w.hasDeletions());
// Check that deleting _fewer_ than 10% doesn't merge inappropriately. Nothing should be merged since no segment
@@ -301,7 +301,7 @@ public class TestTieredMergePolicy extends BaseMergePolicyTestCase {
// This time, forceMerge. By default this should respect max segment size.
// Will change for LUCENE-8236
w.forceMerge(Integer.MAX_VALUE);
- checkSegmentSizeNotExceeded(w.segmentInfos, maxSegBytes);
+ checkSegmentSizeNotExceeded(w.cloneSegmentInfos(), maxSegBytes);
// Now forceMerge down to one segment, there should be exactly remainingDocs in exactly one segment.
w.forceMerge(1);
@@ -346,9 +346,9 @@ public class TestTieredMergePolicy extends BaseMergePolicyTestCase {
w.commit(); // want to trigger merge no matter what.
- assertEquals("There should be exactly one very large and one small segment", w.segmentInfos.size(), 2);
- SegmentCommitInfo info0 = w.segmentInfos.info(0);
- SegmentCommitInfo info1 = w.segmentInfos.info(1);
+ assertEquals("There should be exactly one very large and one small segment", 2, w.listOfSegmentCommitInfos().size());
+ SegmentCommitInfo info0 = w.listOfSegmentCommitInfos().get(0);
+ SegmentCommitInfo info1 = w.listOfSegmentCommitInfos().get(1);
int largeSegDocCount = Math.max(info0.info.maxDoc(), info1.info.maxDoc());
int smallSegDocCount = Math.min(info0.info.maxDoc(), info1.info.maxDoc());
assertEquals("The large segment should have a bunch of docs", largeSegDocCount, remainingDocs);
@@ -413,7 +413,7 @@ public class TestTieredMergePolicy extends BaseMergePolicyTestCase {
List<String> getSegmentNames(IndexWriter w) {
List<String> names = new ArrayList<>();
- for (SegmentCommitInfo info : w.segmentInfos) {
+ for (SegmentCommitInfo info : w.cloneSegmentInfos()) {
names.add(info.info.name);
}
return names;