From 6e66b089f9c82693fc4a2643b33a232b48b80f8a Mon Sep 17 00:00:00 2001
From: Dave Borowitz <dborowitz@google.com>
Date: Tue, 11 Aug 2015 16:58:37 -0400
Subject: [PATCH] InternalChangeQuery: Don't batch exact commit queries

The batching functionality is to work around performance problems in
secondary index backends when handling lots of prefix queries. If we
have the exact commit field, then this query has no prefix queries at
all, so no need to do this workaround and split up into multiple
queries on the backend.

Change-Id: Ia150eec1c36c403f9e6d16a85b9d857da840f957
---
 .../query/change/InternalChangeQuery.java     | 45 +++++++++++++------
 .../change/AbstractQueryChangesTest.java      |  6 ++-
 2 files changed, 35 insertions(+), 16 deletions(-)

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
index 857d61d52f..56d26e58cb 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
@@ -27,6 +27,7 @@ import com.google.gerrit.common.Nullable;
 import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.index.ChangeField;
 import com.google.gerrit.server.index.ChangeIndex;
 import com.google.gerrit.server.index.IndexCollection;
 import com.google.gerrit.server.index.IndexConfig;
@@ -126,27 +127,25 @@ public class InternalChangeQuery {
 
   public Iterable<ChangeData> byCommitsOnBranchNotMerged(Branch.NameKey branch,
       List<String> hashes) throws OrmException {
-    return byCommitsOnBranchNotMerged(
-        branch, hashes, indexConfig.maxPrefixTerms());
+    Schema<ChangeData> schema = schema(indexes);
+    if (schema.hasField(ChangeField.EXACT_COMMIT)) {
+      return query(commitsOnBranchNotMerged(branch, commits(schema, hashes)));
+    } else {
+      return byCommitsOnBranchNotMerged(
+          schema, branch, hashes, indexConfig.maxPrefixTerms());
+    }
   }
 
   @VisibleForTesting
-  Iterable<ChangeData> byCommitsOnBranchNotMerged(Branch.NameKey branch,
-      List<String> hashes, int batchSize) throws OrmException {
-    Schema<ChangeData> schema = schema(indexes);
-    List<Predicate<ChangeData>> commits = new ArrayList<>(hashes.size());
-    for (String s : hashes) {
-      commits.add(commit(schema, s));
-    }
+  Iterable<ChangeData> byCommitsOnBranchNotMerged(Schema<ChangeData> schema,
+      Branch.NameKey branch, List<String> hashes, int batchSize)
+      throws OrmException {
+    List<Predicate<ChangeData>> commits = commits(schema, hashes);
     int numBatches = (hashes.size() / batchSize) + 1;
     List<Predicate<ChangeData>> queries = new ArrayList<>(numBatches);
     for (List<Predicate<ChangeData>> batch
         : Iterables.partition(commits, batchSize)) {
-      queries.add(and(
-            ref(branch),
-            project(branch.getParentKey()),
-            not(status(Change.Status.MERGED)),
-            or(batch)));
+      queries.add(commitsOnBranchNotMerged(branch, batch));
     }
     try {
       return FluentIterable.from(qp.queryChanges(queries))
@@ -161,6 +160,24 @@ public class InternalChangeQuery {
     }
   }
 
+  private static List<Predicate<ChangeData>> commits(Schema<ChangeData> schema,
+      List<String> hashes) {
+    List<Predicate<ChangeData>> commits = new ArrayList<>(hashes.size());
+    for (String s : hashes) {
+      commits.add(commit(schema, s));
+    }
+    return commits;
+  }
+
+  private static Predicate<ChangeData> commitsOnBranchNotMerged(
+      Branch.NameKey branch, List<Predicate<ChangeData>> commits) {
+    return and(
+        ref(branch),
+        project(branch.getParentKey()),
+        not(status(Change.Status.MERGED)),
+        or(commits));
+  }
+
   public List<ChangeData> byProjectOpen(Project.NameKey project)
       throws OrmException {
     return query(and(project(project), open()));
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 1b993dfb47..cd10028914 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -54,6 +54,7 @@ import com.google.gerrit.server.change.ChangeInserter;
 import com.google.gerrit.server.change.ChangeTriplet;
 import com.google.gerrit.server.change.PatchSetInserter;
 import com.google.gerrit.server.change.PatchSetInserter.ValidatePolicy;
+import com.google.gerrit.server.index.IndexCollection;
 import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.ProjectControl;
@@ -116,6 +117,7 @@ public abstract class AbstractQueryChangesTest {
   @Inject protected ChangeControl.GenericFactory changeControlFactory;
   @Inject protected GerritApi gApi;
   @Inject protected IdentifiedUser.GenericFactory userFactory;
+  @Inject protected IndexCollection indexes;
   @Inject protected InMemoryDatabase schemaFactory;
   @Inject protected InMemoryRepositoryManager repoManager;
   @Inject protected InternalChangeQuery internalChangeQuery;
@@ -1156,8 +1158,8 @@ public abstract class AbstractQueryChangesTest {
     }
 
     for (int i = 1; i <= 11; i++) {
-      Iterable<ChangeData> cds =
-          internalChangeQuery.byCommitsOnBranchNotMerged(dest, shas, i);
+      Iterable<ChangeData> cds = internalChangeQuery.byCommitsOnBranchNotMerged(
+          indexes.getSearchIndex().getSchema(), dest, shas, i);
       Iterable<Integer> ids = FluentIterable.from(cds).transform(
           new Function<ChangeData, Integer>() {
             @Override