[GitHub] [lucene-solr] mikemccand 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>]

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



##########
File path: lucene/core/src/java/org/apache/lucene/index/CheckIndex.java
##########
@@ -3900,6 +3907,12 @@ public static Options parseOptions(String[] args) {
         }
         i++;
         opts.dirImpl = args[i];
+      } else if ("-min_version_created".equals(args[i])) {

Review comment:
       `-min_major_version_created`?

##########
File path: lucene/core/src/java/org/apache/lucene/index/CheckIndex.java
##########
@@ -462,6 +463,11 @@ public void setInfoStream(PrintStream out, boolean verbose) {
     this.verbose = verbose;
   }
 
+  /** Set the minimum index version created for the index to check */
+  public void setMinIndexVersionCreated(int minIndexVersionCreated) {

Review comment:
       Could we consistently rename to `setMinIndexMajorVersionCreated`, and `minIndexMajorVersionCreated`?  (I see e.g. in `SIS.readCommit` below that we include `major` in the name).

##########
File path: lucene/core/src/java/org/apache/lucene/index/CheckIndex.java
##########
@@ -3900,6 +3907,12 @@ public static Options parseOptions(String[] args) {
         }
         i++;
         opts.dirImpl = args[i];
+      } else if ("-min_version_created".equals(args[i])) {
+        if (i == args.length - 1) {
+          throw new IllegalArgumentException("ERROR: missing value for -min_version_created");

Review comment:
       Hmm, we should also update the `Usage: ...` exception (around line 3928 in this modified version) to document this new option?
   
   If a user tries to `CheckIndex` a too-old index without this option they'll see a `IndexFormatTooOldException` right?  Should we catch that and rethrow w/ better message suggesting to use this option?  Should we maybe by default just set this option (always allow `CheckIndex` on a too-old index as long as you have the old Codecs around...)?

##########
File path: lucene/core/src/java/org/apache/lucene/index/DirectoryReader.java
##########
@@ -104,6 +104,23 @@ 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}. This method

Review comment:
       s/`in`/`on`

##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexWriter.java
##########
@@ -1009,6 +1009,14 @@ public IndexWriter(Directory d, IndexWriterConfig conf) throws IOException {
         changed();
 
       } else if (reader != null) {
+        if (reader.segmentInfos.getIndexCreatedVersionMajor() < Version.MIN_SUPPORTED_MAJOR) {

Review comment:
       Hmm does `addIndexes` try to verify version of the incoming index is not too old?  We will keep doing that, right?  I.e. the only added best effort here is when directly opening an `IndexReader` you can (with this change) now ask that older versions be allowed.

##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java
##########
@@ -499,7 +515,7 @@ private static Codec readCodec(DataInput input) throws IOException {
         throw new IllegalArgumentException(
             "Could not load codec '"
                 + name
-                + "'.  Did you forget to add lucene-backward-codecs.jar?",
+                + "'. Did you forget to add lucene-backward-codecs.jar?",

Review comment:
       Heh

##########
File path: lucene/core/src/java/org/apache/lucene/index/DirectoryReader.java
##########
@@ -104,6 +104,23 @@ 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}. This method
+   * allows to open indices that were created wih a Lucene version older than N-1 provided that all
+   * codecs for this index are available in the classpath and the segment file format used was
+   * created with Lucene 7 or older. Users of this API must be aware that Lucene doesn't guarantee
+   * semantic compatibility for indices created with versions older than N-1. All backwards
+   * compatibility aside of the file format is optional and applied on a best effort basis.

Review comment:
       s/`of`/`from`




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