ChangeFinder: Dedup results when subindexes are used

Some secondary index implementations store changes in nonoverlapping
subindexes. Due to the non-atomic nature of these subindexes, we might
fleetingly observe a change as present twice. This is not an error that
the user can do anything about, so relax ChangeFinder a bit in this
specific case.

Change-Id: Ie434937ad6e05c063bae04fc0affe3eaef6a51e0

gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticIndexModule.java

@@ -68,6 +68,6 @@ public class ElasticIndexModule extends LifecycleModule {
   @Provides
   @Singleton
   IndexConfig getIndexConfig(@GerritServerConfig Config cfg) {
-    return IndexConfig.fromConfig(cfg);
+    return IndexConfig.fromConfig(cfg).separateChangeSubIndexes(true).build();
   }
 }

gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneIndexModule.java

@@ -84,7 +84,7 @@ public class LuceneIndexModule extends LifecycleModule {
   IndexConfig getIndexConfig(@GerritServerConfig Config cfg) {
     BooleanQuery.setMaxClauseCount(
         cfg.getInt("index", "maxTerms", BooleanQuery.getMaxClauseCount()));
-    return IndexConfig.fromConfig(cfg);
+    return IndexConfig.fromConfig(cfg).separateChangeSubIndexes(true).build();
   }
 
   private static class MultiVersionModule extends LifecycleModule {

gerrit-server/src/main/java/com/google/gerrit/server/ChangeFinder.java

@@ -14,9 +14,11 @@
 
 package com.google.gerrit.server;
 
+import com.google.common.collect.Sets;
 import com.google.common.primitives.Ints;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.server.change.ChangeTriplet;
+import com.google.gerrit.server.index.IndexConfig;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.query.change.ChangeData;
@@ -29,13 +31,16 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
+import java.util.Set;
 
 @Singleton
 public class ChangeFinder {
+  private final IndexConfig indexConfig;
   private final Provider<InternalChangeQuery> queryProvider;
 
   @Inject
-  ChangeFinder(Provider<InternalChangeQuery> queryProvider) {
+  ChangeFinder(IndexConfig indexConfig, Provider<InternalChangeQuery> queryProvider) {
+    this.indexConfig = indexConfig;
     this.queryProvider = queryProvider;
   }
 
@@ -93,8 +98,24 @@ public class ChangeFinder {
   private List<ChangeControl> asChangeControls(List<ChangeData> cds, CurrentUser user)
       throws OrmException {
     List<ChangeControl> ctls = new ArrayList<>(cds.size());
+    if (!indexConfig.separateChangeSubIndexes()) {
+      for (ChangeData cd : cds) {
+        ctls.add(cd.changeControl(user));
+      }
+      return ctls;
+    }
+
+    // If an index implementation uses separate non-atomic subindexes, it's possible to temporarily
+    // observe a change as present in both subindexes, if this search is concurrent with a write.
+    // Dedup to avoid confusing the caller. We can choose an arbitrary ChangeData instance because
+    // the index results have no stored fields, so the data is already reloaded. (It's also possible
+    // that a change might appear in zero subindexes, but there's nothing we can do here to help
+    // this case.)
+    Set<Change.Id> seen = Sets.newHashSetWithExpectedSize(cds.size());
     for (ChangeData cd : cds) {
-      ctls.add(cd.changeControl(user));
+      if (seen.add(cd.getId())) {
+        ctls.add(cd.changeControl(user));
+      }
     }
     return ctls;
   }

gerrit-server/src/main/java/com/google/gerrit/server/index/IndexConfig.java

@@ -34,12 +34,12 @@ public abstract class IndexConfig {
     return builder().build();
   }
 
-  public static IndexConfig fromConfig(Config cfg) {
+  public static Builder fromConfig(Config cfg) {
     Builder b = builder();
     setIfPresent(cfg, "maxLimit", b::maxLimit);
     setIfPresent(cfg, "maxPages", b::maxPages);
     setIfPresent(cfg, "maxTerms", b::maxTerms);
-    return b.build();
+    return b;
   }
 
   private static void setIfPresent(Config cfg, String name, IntConsumer setter) {
@@ -53,7 +53,8 @@ public abstract class IndexConfig {
     return new AutoValue_IndexConfig.Builder()
         .maxLimit(Integer.MAX_VALUE)
         .maxPages(Integer.MAX_VALUE)
-        .maxTerms(DEFAULT_MAX_TERMS);
+        .maxTerms(DEFAULT_MAX_TERMS)
+        .separateChangeSubIndexes(false);
   }
 
   @AutoValue.Builder
@@ -70,6 +71,8 @@ public abstract class IndexConfig {
 
     abstract int maxTerms();
 
+    public abstract Builder separateChangeSubIndexes(boolean separate);
+
     abstract IndexConfig autoBuild();
 
     public IndexConfig build() {
@@ -101,4 +104,9 @@ public abstract class IndexConfig {
    *     for performance reasons.
    */
   public abstract int maxTerms();
+
+  /**
+   * @return whether different subsets of changes may be stored in different physical sub-indexes.
+   */
+  public abstract boolean separateChangeSubIndexes();
 }