Change default ReadAdvice from RANDOM to NORMAL (#15040)

This commit changes the default ReadAdvice from RANDOM to NORMAL.

After this change MMapDirectory will not set any specific read advice out-of-the-box. Rather there is a straightforward way to enable either previous behaviour ( in 10.2 ), by setting either:
1. dir.setReadAdvice(MMapDirectory.ADVISE_BY_CONTEXT), or
2. setting the system property -Dorg.apache.lucene.store.defaultReadAdvice=RANDOM.

closes #14408

Author: ChrisHegarty

lucene/CHANGES.txt

@@ -243,6 +243,12 @@ Changes in Runtime Behavior
   tier from 10 to 8. This is expected to result in slightly faster search and
   slightly slower indexing. (Adrien Grand)
 
+* GITHUB#15040: The default ReadAdvice has been changed from RANDOM to NORMAL. MMapDirectory
+  will no longer set any specific read advice out-of-the-box. Rather there is a straightforward
+  way to enable previous behaviour (in 10.2), either by setting:
+  `dir.setReadAdvice(MMapDirectory.ADVISE_BY_CONTEXT)`, or setting the system property
+  `-Dorg.apache.lucene.store.defaultReadAdvice=RANDOM`. (Chris Hegarty)
+
 Bug Fixes
 ---------------------
 * GITHUB*15025: flush segments in addIndexes, not in addIndexesReaderMerge;

lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java

@@ -141,8 +141,40 @@ public class MMapDirectory extends FSDirectory {
         return Optional.of(groupKey);
       };
 
-  private BiFunction<String, IOContext, Optional<ReadAdvice>> readAdviceOverride =
+  /**
+   * Argument for {@link #setReadAdvice(BiFunction)} that configures the read advice based on the
+   * context type and hints.
+   *
+   * <p>Specifically, the advice is determined by evaluating the following conditions in order:
+   *
+   * <ol>
+   *   <li>If the context is either of {@link IOContext.Context#MERGE} or {@link
+   *       IOContext.Context#FLUSH}, then {@link ReadAdvice#SEQUENTIAL},
+   *   <li>If the context {@link IOContext#hints() hints} contains {@link DataAccessHint#RANDOM},
+   *       then {@link ReadAdvice#RANDOM},
+   *   <li>If the context {@link IOContext#hints() hints} contains {@link
+   *       DataAccessHint#SEQUENTIAL}, then {@link ReadAdvice#SEQUENTIAL},
+   *   <li>Otherwise, {@link Constants#DEFAULT_READADVICE} is returned.
+   * </ol>
+   */
+  public static final BiFunction<String, IOContext, Optional<ReadAdvice>> ADVISE_BY_CONTEXT =
+      (String _, IOContext context) -> {
+        if (context.context() == IOContext.Context.MERGE
+            || context.context() == IOContext.Context.FLUSH) {
+          return Optional.of(ReadAdvice.SEQUENTIAL);
+        }
+        if (context.hints().contains(DataAccessHint.RANDOM)) {
+          return Optional.of(ReadAdvice.RANDOM);
+        }
+        if (context.hints().contains(DataAccessHint.SEQUENTIAL)) {
+          return Optional.of(ReadAdvice.SEQUENTIAL);
+        }
+        return Optional.of(Constants.DEFAULT_READADVICE);
+      };
+
+  private BiFunction<String, IOContext, Optional<ReadAdvice>> readAdvice =
       (_, _) -> Optional.empty();
+
   private BiPredicate<String, IOContext> preload = NO_FILES;
 
   /**
@@ -248,17 +280,16 @@ public void setPreload(BiPredicate<String, IOContext> preload) {
   }
 
   /**
-   * Configure {@link ReadAdvice} overrides for certain files. If the function returns {@code
-   * Optional.empty()}, a default {@link ReadAdvice} will be used
+   * Configure {@link ReadAdvice} for certain files. The default implementation uses the {@link
+   * Constants#DEFAULT_READADVICE}.
    *
    * @param toReadAdvice a {@link Function} whose first argument is the file name, and second
    *     argument is the {@link IOContext} used to open the file. Returns {@code
    *     Optional.of(ReadAdvice)} to use a specific read advice, or {@code Optional.empty()} if a
    *     default should be used
    */
-  public void setReadAdviceOverride(
-      BiFunction<String, IOContext, Optional<ReadAdvice>> toReadAdvice) {
-    this.readAdviceOverride = toReadAdvice;
+  public void setReadAdvice(BiFunction<String, IOContext, Optional<ReadAdvice>> toReadAdvice) {
+    this.readAdvice = toReadAdvice;
   }
 
   /**
@@ -285,22 +316,6 @@ public final long getMaxChunkSize() {
     return 1L << chunkSizePower;
   }
 
-  private static ReadAdvice toReadAdvice(IOContext context) {
-    if (context.context() == IOContext.Context.MERGE
-        || context.context() == IOContext.Context.FLUSH) {
-      return ReadAdvice.SEQUENTIAL;
-    }
-
-    if (context.hints().contains(DataAccessHint.RANDOM)) {
-      return ReadAdvice.RANDOM;
-    }
-    if (context.hints().contains(DataAccessHint.SEQUENTIAL)) {
-      return ReadAdvice.SEQUENTIAL;
-    }
-
-    return Constants.DEFAULT_READADVICE;
-  }
-
   /** Creates an IndexInput for the file with the given name. */
   @Override
   public IndexInput openInput(String name, IOContext context) throws IOException {
@@ -314,7 +329,7 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
 
     final boolean confined = context.hints().contains(ReadOnceHint.INSTANCE);
     Function<IOContext, ReadAdvice> toReadAdvice =
-        c -> readAdviceOverride.apply(name, c).orElseGet(() -> toReadAdvice(c));
+        c -> readAdvice.apply(name, c).orElse(Constants.DEFAULT_READADVICE);
     final Arena arena = confined ? Arena.ofConfined() : getSharedArena(name, arenas);
     try (var fc = FileChannel.open(path, StandardOpenOption.READ)) {
       final long fileSize = fc.size();

lucene/core/src/java/org/apache/lucene/util/Constants.java

@@ -176,13 +176,13 @@ private static boolean hasFastCompressMaskCast() {
 
   /**
    * The default {@link ReadAdvice} used for opening index files. It will be {@link
-   * ReadAdvice#RANDOM} by default, unless set by system property {@code
+   * ReadAdvice#NORMAL} by default, unless set by system property {@code
    * org.apache.lucene.store.defaultReadAdvice}.
    */
   public static final ReadAdvice DEFAULT_READADVICE =
       Optional.ofNullable(getSysProp("org.apache.lucene.store.defaultReadAdvice"))
           .map(a -> ReadAdvice.valueOf(a.toUpperCase(Locale.ROOT)))
-          .orElse(ReadAdvice.RANDOM);
+          .orElse(ReadAdvice.NORMAL);
 
   private static String getSysProp(String property) {
     return System.getProperty(property);

lucene/core/src/test/org/apache/lucene/store/TestMMapDirectory.java

@@ -21,6 +21,7 @@
 import java.io.IOException;
 import java.nio.file.Path;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
@@ -32,6 +33,7 @@
 import java.util.concurrent.Future;
 import java.util.function.Supplier;
 import java.util.stream.IntStream;
+import java.util.stream.Stream;
 import org.apache.lucene.tests.store.BaseDirectoryTestCase;
 import org.apache.lucene.util.Constants;
 import org.apache.lucene.util.NamedThreadFactory;
@@ -122,7 +124,7 @@ public void testWithNormal() throws Exception {
         out.writeBytes(bytes, 0, bytes.length);
       }
 
-      dir.setReadAdviceOverride((_, _) -> Optional.of(ReadAdvice.NORMAL));
+      dir.setReadAdvice((_, _) -> Optional.of(ReadAdvice.NORMAL));
       try (final IndexInput in = dir.openInput("test", IOContext.DEFAULT)) {
         final byte[] readBytes = new byte[size];
         in.readBytes(readBytes, 0, readBytes.length);
@@ -131,6 +133,55 @@ public void testWithNormal() throws Exception {
     }
   }
 
+  public void testAdviseByContextFunc() throws IOException {
+    var func = MMapDirectory.ADVISE_BY_CONTEXT;
+    var flush = IOContext.flush(new FlushInfo(1, 2));
+    var merge = IOContext.merge(new MergeInfo(1, 2, true, 4));
+    var random = new DefaultIOContext(DataAccessHint.RANDOM);
+    var sequential = new DefaultIOContext(DataAccessHint.SEQUENTIAL);
+    assertEquals(ReadAdvice.SEQUENTIAL, func.apply("a", merge).get());
+    assertEquals(ReadAdvice.SEQUENTIAL, func.apply("b", flush).get());
+    assertEquals(ReadAdvice.SEQUENTIAL, func.apply("c", sequential).get());
+    assertEquals(ReadAdvice.RANDOM, func.apply("d", random).get());
+
+    // hint types other than DataAccessHint
+    List<IOContext.FileOpenHint> hints = new ArrayList<>();
+    hints.addAll(Arrays.asList(FileDataHint.values()));
+    hints.addAll(Arrays.asList(FileTypeHint.values()));
+    hints.addAll(Arrays.asList(PreloadHint.values()));
+    hints.addAll(Arrays.asList(ReadOnceHint.values()));
+    for (var hint : hints) {
+      var context = new DefaultIOContext(hint);
+      assertEquals(Constants.DEFAULT_READADVICE, func.apply("e", context).get());
+    }
+
+    var l1 = List.of(flush, merge, random, sequential, IOContext.DEFAULT, IOContext.READONCE);
+    var l2 = hints.stream().map(DefaultIOContext::new);
+    testWithAdviseByContext(Stream.concat(l1.stream(), l2).toList());
+  }
+
+  // trivially exercises MMapDirectory.ADVISE_BY_CONTEXT with different contexts
+  void testWithAdviseByContext(List<IOContext> contexts) throws IOException {
+    final int size = 8 * 1024;
+    byte[] bytes = new byte[size];
+    random().nextBytes(bytes);
+
+    try (MMapDirectory dir = new MMapDirectory(createTempDir("testWithAdviseByContext"))) {
+      try (IndexOutput out = dir.createOutput("test", IOContext.DEFAULT)) {
+        out.writeBytes(bytes, 0, bytes.length);
+      }
+      dir.setReadAdvice(MMapDirectory.ADVISE_BY_CONTEXT);
+
+      for (var context : contexts) {
+        try (final IndexInput in = dir.openInput("test", context)) {
+          final byte[] readBytes = new byte[size];
+          in.readBytes(readBytes, 0, readBytes.length);
+          assertArrayEquals(bytes, readBytes);
+        }
+      }
+    }
+  }
+
   public void testPreload() throws Exception {
     assumeTrue("madvise for preloading only works on linux/mac", MMapDirectory.supportsMadvise());