PR comments

Signed-off-by: Kartik Ganesh <gkart@amazon.com>
opensearch-project · Dec 15, 2022 · 58a23b2 · 58a23b2
1 parent 78681fb
commit 58a23b2
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 13 deletions.
diff --git a/server/src/main/java/org/opensearch/index/shard/IndexShard.java b/server/src/main/java/org/opensearch/index/shard/IndexShard.java
@@ -2039,16 +2039,7 @@ private void innerOpenEngineAndTranslog(LongSupplier globalCheckpointSupplier) t
     }
 
     private boolean assertSequenceNumbersInCommit() throws IOException {
-        final Map<String, String> userData;
-        if (indexSettings.isRemoteSnapshot() && indexSettings.getExtendedCompatibilitySnapshotVersion() != null) {
-            // Inefficient method to support reading old Lucene indexes
-            userData = Lucene.readSegmentInfosExtendedCompatibility(
-                store.directory(),
-                indexSettings.getExtendedCompatibilitySnapshotVersion()
-            ).getUserData();
-        } else {
-            userData = SegmentInfos.readLatestCommit(store.directory()).getUserData();
-        }
+        final Map<String, String> userData = fetchUserData();
         assert userData.containsKey(SequenceNumbers.LOCAL_CHECKPOINT_KEY) : "commit point doesn't contains a local checkpoint";
         assert userData.containsKey(MAX_SEQ_NO) : "commit point doesn't contains a maximum sequence number";
         assert userData.containsKey(Engine.HISTORY_UUID_KEY) : "commit point doesn't contains a history uuid";
@@ -2063,6 +2054,16 @@ private boolean assertSequenceNumbersInCommit() throws IOException {
         return true;
     }
 
+    private Map<String, String> fetchUserData() throws IOException {
+        if (indexSettings.isRemoteSnapshot() && indexSettings.getExtendedCompatibilitySnapshotVersion() != null) {
+            // Inefficient method to support reading old Lucene indexes
+            return Lucene.readSegmentInfosExtendedCompatibility(store.directory(), indexSettings.getExtendedCompatibilitySnapshotVersion())
+                .getUserData();
+        } else {
+            return SegmentInfos.readLatestCommit(store.directory()).getUserData();
+        }
+    }
+
     private void onNewEngine(Engine newEngine) {
         assert Thread.holdsLock(engineMutex);
         refreshListeners.setCurrentRefreshLocationSupplier(newEngine::getTranslogLastWriteLocation);

diff --git a/server/src/main/java/org/opensearch/index/store/Store.java b/server/src/main/java/org/opensearch/index/store/Store.java
@@ -222,7 +222,7 @@ public SegmentInfos readLastCommittedSegmentsInfo() throws IOException {
         failIfCorrupted();
         try {
             if (indexSettings.isRemoteSnapshot() && indexSettings.getExtendedCompatibilitySnapshotVersion() != null) {
-                return readSegmentInfosExtendedCompatbility(directory(), indexSettings.getExtendedCompatibilitySnapshotVersion());
+                return readSegmentInfosExtendedCompatibility(directory(), indexSettings.getExtendedCompatibilitySnapshotVersion());
             } else {
                 return readSegmentsInfo(null, directory());
             }
@@ -235,7 +235,7 @@ public SegmentInfos readLastCommittedSegmentsInfo() throws IOException {
     /**
      * Returns the segments info for the given commit or for the latest commit if the given commit is <code>null</code>.
      * This method will throw an exception if the index is older than the standard backwards compatibility
-     * policy ( current major - 1). See also {@link #readSegmentInfosExtendedCompatbility(Directory, org.opensearch.Version)}.
+     * policy ( current major - 1). See also {@link #readSegmentInfosExtendedCompatibility(Directory, org.opensearch.Version)}.
      *
      * @throws IOException if the index is corrupted or the segments file is not present
      */
@@ -260,7 +260,7 @@ private static SegmentInfos readSegmentsInfo(IndexCommit commit, Directory direc
      *
      * @throws IOException if the index is corrupted or the segments file is not present
      */
-    private static SegmentInfos readSegmentInfosExtendedCompatbility(Directory directory, org.opensearch.Version minimumVersion)
+    private static SegmentInfos readSegmentInfosExtendedCompatibility(Directory directory, org.opensearch.Version minimumVersion)
         throws IOException {
         try {
             return Lucene.readSegmentInfosExtendedCompatibility(directory, minimumVersion);

diff --git a/server/src/test/java/org/opensearch/common/lucene/LuceneTests.java b/server/src/test/java/org/opensearch/common/lucene/LuceneTests.java
@@ -407,6 +407,59 @@ public void testReadSegmentInfosExtendedCompatibility() throws IOException {
         dir.close();
     }
 
+    /**
+     * Since the implementation in {@link Lucene#readSegmentInfosExtendedCompatibility(Directory, Version)}
+     * is a workaround, this test verifies that the response from this method is equivalent to
+     * {@link Lucene#readSegmentInfos(Directory)} if the version is N-1
+     */
+    public void testReadSegmentInfosExtendedCompatibilityBaseCase() throws IOException {
+        MockDirectoryWrapper dir = newMockDirectory();
+        IndexWriterConfig iwc = newIndexWriterConfig();
+        IndexWriter writer = new IndexWriter(dir, iwc);
+        Document doc = new Document();
+        doc.add(new TextField("id", "1", random().nextBoolean() ? Field.Store.YES : Field.Store.NO));
+        writer.addDocument(doc);
+        writer.commit();
+        SegmentInfos expectedSI = Lucene.readSegmentInfos(dir);
+        SegmentInfos actualSI = Lucene.readSegmentInfosExtendedCompatibility(dir, Version.CURRENT);
+        assertEquals(Lucene.getNumDocs(expectedSI), Lucene.getNumDocs(actualSI));
+        assertEquals(expectedSI.getGeneration(), actualSI.getGeneration());
+        assertEquals(expectedSI.getSegmentsFileName(), actualSI.getSegmentsFileName());
+        assertEquals(expectedSI.getVersion(), actualSI.getVersion());
+        assertEquals(expectedSI.getCommitLuceneVersion(), actualSI.getCommitLuceneVersion());
+        assertEquals(expectedSI.getMinSegmentLuceneVersion(), actualSI.getMinSegmentLuceneVersion());
+        assertEquals(expectedSI.getIndexCreatedVersionMajor(), actualSI.getIndexCreatedVersionMajor());
+        assertEquals(expectedSI.getUserData(), actualSI.getUserData());
+
+        int numDocsToIndex = randomIntBetween(10, 50);
+        List<Term> deleteTerms = new ArrayList<>();
+        for (int i = 0; i < numDocsToIndex; i++) {
+            doc = new Document();
+            doc.add(new TextField("id", "doc_" + i, random().nextBoolean() ? Field.Store.YES : Field.Store.NO));
+            deleteTerms.add(new Term("id", "doc_" + i));
+            writer.addDocument(doc);
+        }
+        int numDocsToDelete = randomIntBetween(0, numDocsToIndex);
+        Collections.shuffle(deleteTerms, random());
+        for (int i = 0; i < numDocsToDelete; i++) {
+            Term remove = deleteTerms.remove(0);
+            writer.deleteDocuments(remove);
+        }
+        writer.commit();
+        expectedSI = Lucene.readSegmentInfos(dir);
+        actualSI = Lucene.readSegmentInfosExtendedCompatibility(dir, Version.CURRENT);
+        assertEquals(Lucene.getNumDocs(expectedSI), Lucene.getNumDocs(actualSI));
+        assertEquals(expectedSI.getGeneration(), actualSI.getGeneration());
+        assertEquals(expectedSI.getSegmentsFileName(), actualSI.getSegmentsFileName());
+        assertEquals(expectedSI.getVersion(), actualSI.getVersion());
+        assertEquals(expectedSI.getCommitLuceneVersion(), actualSI.getCommitLuceneVersion());
+        assertEquals(expectedSI.getMinSegmentLuceneVersion(), actualSI.getMinSegmentLuceneVersion());
+        assertEquals(expectedSI.getIndexCreatedVersionMajor(), actualSI.getIndexCreatedVersionMajor());
+        assertEquals(expectedSI.getUserData(), actualSI.getUserData());
+        writer.close();
+        dir.close();
+    }
+
     public void testCount() throws Exception {
         Directory dir = newDirectory();
         RandomIndexWriter w = new RandomIndexWriter(random(), dir);