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
diff --git a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticIndexModule.java b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticIndexModule.java
index 566e159..53e7c19 100644
--- a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticIndexModule.java
+++ b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticIndexModule.java
@@ -68,6 +68,6 @@
@Provides
@Singleton
IndexConfig getIndexConfig(@GerritServerConfig Config cfg) {
- return IndexConfig.fromConfig(cfg);
+ return IndexConfig.fromConfig(cfg).separateChangeSubIndexes(true).build();
}
}
diff --git a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneIndexModule.java b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneIndexModule.java
index 699fd51..89fd819 100644
--- a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneIndexModule.java
+++ b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneIndexModule.java
@@ -84,7 +84,7 @@
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 {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeFinder.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeFinder.java
index 2f3a76f..2d80ceb 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeFinder.java
+++ b/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.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 @@
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;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexConfig.java
index 313ffc2..5c3cdf2 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexConfig.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexConfig.java
@@ -34,12 +34,12 @@
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 @@
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 @@
abstract int maxTerms();
+ public abstract Builder separateChangeSubIndexes(boolean separate);
+
abstract IndexConfig autoBuild();
public IndexConfig build() {
@@ -101,4 +104,9 @@
* 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();
}
