During online reindexing of all changes skip changes already present

During online reindexing after an upgrade, all changes were reindexed,
even if they were already present in the new index. If the Gerrit
instance was restarted before the index was ready, this caused a lot
of repeated indexing.

Now, changes that are already present in the newest index version
won't be reindexed anymore. However, they will still be checked
for staleness and reindexed if necessary.

Revert the change "Remove unused method IndexedChangeQuery#oneResult"
(commit 585022e176ad31a82ab3fc0b7fe049f4bb13c6ff) since we need it here.

Release-Notes: Skip already reindexed changes during reindexing
Change-Id: I1fd78958f6fa0848f81630abab0428520f412baa
(cherry picked from commit 366ae421f263acf201adbcc1f13c69f03fef40cf)

4 files changed, 46 insertions, 12 deletions

diff --git a/java/com/google/gerrit/server/index/OnlineReindexer.java b/java/com/google/gerrit/server/index/OnlineReindexer.java
index eef394d66d..e3b8e7cd12 100644
--- a/java/com/google/gerrit/server/index/OnlineReindexer.java
+++ b/java/com/google/gerrit/server/index/OnlineReindexer.java
@@ -106,9 +106,6 @@ public class OnlineReindexer<K, V, I extends Index<K, V>> {
         "Starting online reindex of %s from schema version %s to %s",
         name, version(indexes.getSearchIndex()), version(index));
 
-    if (oldVersion != newVersion) {
-      index.deleteAll();
-    }
     SiteIndexer.Result result = batchIndexer.indexAll(index);
     if (!result.success()) {
       logger.atSevere().log(
diff --git a/java/com/google/gerrit/server/index/change/AllChangesIndexer.java b/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
index 3935108199..b98bae0937 100644
--- a/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
+++ b/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
@@ -47,6 +47,7 @@ import com.google.inject.Inject;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.Callable;
 import java.util.concurrent.RejectedExecutionException;
@@ -197,15 +198,20 @@ public class AllChangesIndexer extends SiteIndexer<Change.Id, ChangeData, Change
     return Result.create(sw, ok.get(), nDone, nFailed);
   }
 
+  /**
+   * Reindexes all changes in a given project, even if they already exist in the index. Changes will
+   * not be sliced to allow multithreaded reindexing.
+   */
   @Nullable
   public Callable<Void> reindexProject(
       ChangeIndexer indexer, Project.NameKey project, Task done, Task failed) {
     try (Repository repo = repoManager.openRepository(project)) {
-      return reindexProjectSlice(
+      return new ProjectSliceIndexer(
           indexer,
           ProjectSlice.oneSlice(project, ChangeNotes.Factory.scanChangeIds(repo)),
           done,
-          failed);
+          failed,
+          true);
     } catch (IOException e) {
       logger.atSevere().log("%s", e.getMessage());
       return null;
@@ -214,7 +220,7 @@ public class AllChangesIndexer extends SiteIndexer<Change.Id, ChangeData, Change
 
   public Callable<Void> reindexProjectSlice(
       ChangeIndexer indexer, ProjectSlice projectSlice, Task done, Task failed) {
-    return new ProjectSliceIndexer(indexer, projectSlice, done, failed);
+    return new ProjectSliceIndexer(indexer, projectSlice, done, failed, false);
   }
 
   private class ProjectSliceIndexer implements Callable<Void> {
@@ -222,16 +228,19 @@ public class AllChangesIndexer extends SiteIndexer<Change.Id, ChangeData, Change
     private final ProjectSlice projectSlice;
     private final ProgressMonitor done;
     private final ProgressMonitor failed;
+    private final boolean forceReindex;
 
     private ProjectSliceIndexer(
         ChangeIndexer indexer,
         ProjectSlice projectSlice,
         ProgressMonitor done,
-        ProgressMonitor failed) {
+        ProgressMonitor failed,
+        boolean forceReindex) {
       this.indexer = indexer;
       this.projectSlice = projectSlice;
       this.done = done;
       this.failed = failed;
+      this.forceReindex = forceReindex;
     }
 
     @Override
@@ -247,6 +256,10 @@ public class AllChangesIndexer extends SiteIndexer<Change.Id, ChangeData, Change
                     + projectSlice.slice()
                     + "]");
         OnlineReindexMode.begin();
+        Optional<ChangeIndex> newestIndex = indexer.getNewestIndex();
+        if (newestIndex.isEmpty()) {
+          logger.atWarning().log("No change index available yet");
+        }
         // Order of scanning changes is undefined. This is ok if we assume that packfile locality is
         // not important for indexing, since sites should have a fully populated DiffSummary cache.
         // It does mean that reindexing after invalidating the DiffSummary cache will be expensive,
@@ -257,7 +270,7 @@ public class AllChangesIndexer extends SiteIndexer<Change.Id, ChangeData, Change
                 projectSlice.metaIdByChange(),
                 projectSlice.name(),
                 id -> (id.get() % projectSlice.slices()) == projectSlice.slice())
-            .forEach(r -> index(r));
+            .forEach(r -> index(r, newestIndex));
         OnlineReindexMode.end();
       } finally {
         Thread.currentThread().setName(oldThreadName);
@@ -265,16 +278,23 @@ public class AllChangesIndexer extends SiteIndexer<Change.Id, ChangeData, Change
       return null;
     }
 
-    private void index(ChangeNotesResult r) {
+    private void index(ChangeNotesResult r, Optional<ChangeIndex> newestIndex) {
       if (r.error().isPresent()) {
         fail("Failed to read change " + r.id() + " for indexing", true, r.error().get());
         return;
       }
       try {
-        indexer.index(changeDataFactory.create(r.notes()));
+        if (forceReindex || !indexer.isChangeAlreadyIndexed(r.id(), newestIndex)) {
+          indexer.index(changeDataFactory.create(r.notes()));
+          verboseWriter.format(
+              "Reindexed change %d (project: %s)\n",
+              r.id().get(), r.notes().getProjectName().get());
+
+        } else {
+          verboseWriter.format(
+              "Skipped change %d (project: %s)\n", r.id().get(), r.notes().getProjectName().get());
+        }
         done.update(1);
-        verboseWriter.format(
-            "Reindexed change %d (project: %s)\n", r.id().get(), r.notes().getProjectName().get());
       } catch (RejectedExecutionException e) {
         // Server shutdown, don't spam the logs.
         failSilently();
diff --git a/java/com/google/gerrit/server/index/change/ChangeIndexer.java b/java/com/google/gerrit/server/index/change/ChangeIndexer.java
index 517809a0df..17401ec3a7 100644
--- a/java/com/google/gerrit/server/index/change/ChangeIndexer.java
+++ b/java/com/google/gerrit/server/index/change/ChangeIndexer.java
@@ -45,6 +45,7 @@ import com.google.inject.assistedinject.Assisted;
 import com.google.inject.assistedinject.AssistedInject;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
@@ -244,6 +245,17 @@ public class ChangeIndexer {
     fireChangeIndexedEvent(cd.project().get(), cd.getId().get());
   }
 
+  public boolean isChangeAlreadyIndexed(Change.Id id, Optional<ChangeIndex> newestIndex) {
+    if (newestIndex.isEmpty()) {
+      return false;
+    }
+    return newestIndex.get().get(id, IndexedChangeQuery.oneResult()).isPresent();
+  }
+
+  public Optional<ChangeIndex> getNewestIndex() {
+    return getWriteIndexes().stream().max(Comparator.comparingInt(i -> i.getSchema().getVersion()));
+  }
+
   private void fireChangeScheduledForIndexingEvent(String projectName, int id) {
     indexedListeners.runEach(l -> l.onChangeScheduledForIndexing(projectName, id));
   }
diff --git a/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java b/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
index 00642a911b..f7ff13c688 100644
--- a/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
+++ b/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
@@ -21,6 +21,7 @@ import static com.google.gerrit.server.index.change.ChangeField.PROJECT_SPEC;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.gerrit.entities.Change;
 import com.google.gerrit.index.IndexConfig;
@@ -51,6 +52,10 @@ import java.util.Set;
  */
 public class IndexedChangeQuery extends IndexedQuery<Change.Id, ChangeData>
     implements ChangeDataSource, Matchable<ChangeData> {
+  public static QueryOptions oneResult() {
+    IndexConfig config = IndexConfig.createDefault();
+    return createOptions(config, 0, 1, config.pageSizeMultiplier(), 1, ImmutableSet.of());
+  }
 
   public static QueryOptions createOptions(
       IndexConfig config, int start, int limit, Set<String> fields) {