[GitHub] [lucene-solr] jpountz commented on a change in pull request #2212: LUCENE-9669: Add an expert API to allow opening indices created < N-1

[GitBox <gi...@apache.org>]

jpountz commented on a change in pull request #2212:
URL: https://github.com/apache/lucene-solr/pull/2212#discussion_r559039707



##########
File path: lucene/core/src/java/org/apache/lucene/index/DirectoryReader.java
##########
@@ -104,6 +105,17 @@ public static DirectoryReader open(final IndexCommit commit) throws IOException
     return StandardDirectoryReader.open(commit.getDirectory(), commit);
   }
 
+  /**
+   * Expert: returns an IndexReader reading the index in the given {@link IndexCommit}.
+   *
+   * @param commit the commit point to open
+   * @param minIndexVersionCreated the minimum index version created to check before opening the index

Review comment:
       I like how `listCommits` calls it a "major" version which avoids some ambiguity, maybe do the same here?
   
   Also could you improve javadocs to better describe how this version parameter affects the behavior of this method?

##########
File path: lucene/core/src/java/org/apache/lucene/index/LeafMetaData.java
##########
@@ -31,15 +31,19 @@
   private final Sort sort;
 
   /** Expert: Sole constructor. Public for use by custom {@link LeafReader} impls. */
-  public LeafMetaData(int createdVersionMajor, Version minVersion, Sort sort) {
+  public LeafMetaData(int createdVersionMajor, Version minVersion, int minVersionSupported, Sort sort) {
     this.createdVersionMajor = createdVersionMajor;
+    if (minVersionSupported > Version.LATEST.major || minVersionSupported < 0) {
+      throw new IllegalArgumentException("minVersionSupported must be positive and <= "
+              + Version.LATEST.major + " but was: " + minVersionSupported);
+    }
     if (createdVersionMajor > Version.LATEST.major) {
       throw new IllegalArgumentException(
           "createdVersionMajor is in the future: " + createdVersionMajor);
     }
-    if (createdVersionMajor < 6) {
+    if (createdVersionMajor < minVersionSupported) {
       throw new IllegalArgumentException(
-          "createdVersionMajor must be >= 6, got: " + createdVersionMajor);
+          "createdVersionMajor must be >= " + minVersionSupported + ", got: " + createdVersionMajor);

Review comment:
       These checks were introduced in the first place because 7.0 was the version that started recording the major version that created the index, and any older index would have this constant set to 6, rather than because 6 was the minimum supported version. So I think we can keep the logic how it is today without adding a new parameter to the ctor?

##########
File path: lucene/backward-codecs/src/test/org/apache/lucene/backward_index/TestBackwardsCompatibility.java
##########
@@ -1986,4 +1989,36 @@ static BytesRef toBytes(long value) {
     bytes.bytes[bytes.length++] = (byte) value;
     return bytes;
   }
+
+  public void testFailOpenOldIndex() throws IOException {
+    for (String name : oldNames) {
+      Directory directory = oldIndexDirs.get(name);
+      IndexCommit commit = DirectoryReader.listCommits(directory).get(0);
+      IndexFormatTooOldException ex = expectThrows(IndexFormatTooOldException.class, () ->
+              StandardDirectoryReader.open(commit, Version.LATEST.major));
+      ex.getMessage().contains("only supports reading from version " + Version.LATEST.major + " upwards." );

Review comment:
       this should be within an `assertTrue`?

##########
File path: lucene/core/src/java/org/apache/lucene/index/StandardDirectoryReader.java
##########
@@ -40,42 +41,49 @@
   final SegmentInfos segmentInfos;
   private final boolean applyAllDeletes;
   private final boolean writeAllDeletes;
+  private final int minVersionCreated;
 
   /** called only from static open() methods */
   StandardDirectoryReader(
-      Directory directory,
-      LeafReader[] readers,
-      IndexWriter writer,
-      SegmentInfos sis,
-      boolean applyAllDeletes,
-      boolean writeAllDeletes)
+          Directory directory,
+          LeafReader[] readers,
+          IndexWriter writer,
+          SegmentInfos sis,
+          boolean applyAllDeletes,
+          boolean writeAllDeletes, int minVersionCreated)

Review comment:
       put on a new line like other parameters?




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org