Merge "Use sane value for QueryProcessor's effective limit on "--no-limit"" into stable-3.3

8 files changed, 135 insertions, 9 deletions

diff --git a/java/com/google/gerrit/index/query/QueryProcessor.java b/java/com/google/gerrit/index/query/QueryProcessor.java
index c05516be43..125125968f 100644
--- a/java/com/google/gerrit/index/query/QueryProcessor.java
+++ b/java/com/google/gerrit/index/query/QueryProcessor.java
@@ -56,6 +56,7 @@ import java.util.stream.IntStream;
  */
 public abstract class QueryProcessor<T> {
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+  private static final int MAX_LIMIT_BUFFER_MULTIPLIER = 100;
 
   protected static class Metrics {
     final Timer1<String> executionTime;
@@ -356,7 +357,7 @@ public abstract class QueryProcessor<T> {
 
   private int getEffectiveLimit(Predicate<T> p) {
     if (isNoLimit == true) {
-      return Integer.MAX_VALUE;
+      return getIndexSize() + MAX_LIMIT_BUFFER_MULTIPLIER * getBatchSize();
     }
     List<Integer> possibleLimits = new ArrayList<>(4);
     possibleLimits.add(getBackendSupportedLimit());
@@ -384,4 +385,8 @@ public abstract class QueryProcessor<T> {
   }
 
   protected abstract String formatForLogging(T t);
+
+  protected abstract int getIndexSize();
+
+  protected abstract int getBatchSize();
 }
diff --git a/java/com/google/gerrit/server/notedb/RepoSequence.java b/java/com/google/gerrit/server/notedb/RepoSequence.java
index 47e12ff4bd..d743921362 100644
--- a/java/com/google/gerrit/server/notedb/RepoSequence.java
+++ b/java/com/google/gerrit/server/notedb/RepoSequence.java
@@ -340,4 +340,16 @@ public class RepoSequence {
       counterLock.unlock();
     }
   }
+
+  /**
+   * Retrieves the last returned sequence number.
+   *
+   * <p>Explicitly calls {@link #next()} if this instance didn't return sequence number until now.
+   */
+  public int last() {
+    if (counter == 0) {
+      next();
+    }
+    return counter - 1;
+  }
 }
diff --git a/java/com/google/gerrit/server/notedb/Sequences.java b/java/com/google/gerrit/server/notedb/Sequences.java
index 7a8e28f9a4..e44d031a15 100644
--- a/java/com/google/gerrit/server/notedb/Sequences.java
+++ b/java/com/google/gerrit/server/notedb/Sequences.java
@@ -55,6 +55,9 @@ public class Sequences {
   private final RepoSequence changeSeq;
   private final RepoSequence groupSeq;
   private final Timer2<SequenceType, Boolean> nextIdLatency;
+  private final int accountBatchSize;
+  private final int changeBatchSize;
+  private final int groupBatchSize = 1;
 
   @Inject
   public Sequences(
@@ -65,7 +68,7 @@ public class Sequences {
       AllUsersName allUsers,
       MetricMaker metrics) {
 
-    int accountBatchSize =
+    accountBatchSize =
         cfg.getInt(
             SECTION_NOTEDB,
             NAME_ACCOUNTS,
@@ -80,7 +83,7 @@ public class Sequences {
             () -> FIRST_ACCOUNT_ID,
             accountBatchSize);
 
-    int changeBatchSize =
+    changeBatchSize =
         cfg.getInt(
             SECTION_NOTEDB,
             NAME_CHANGES,
@@ -95,7 +98,6 @@ public class Sequences {
             () -> FIRST_CHANGE_ID,
             changeBatchSize);
 
-    int groupBatchSize = 1;
     groupSeq =
         new RepoSequence(
             repoManager,
@@ -144,6 +146,18 @@ public class Sequences {
     }
   }
 
+  public int changeBatchSize() {
+    return changeBatchSize;
+  }
+
+  public int groupBatchSize() {
+    return groupBatchSize;
+  }
+
+  public int accountBatchSize() {
+    return accountBatchSize;
+  }
+
   public int currentChangeId() {
     return changeSeq.current();
   }
@@ -156,6 +170,18 @@ public class Sequences {
     return groupSeq.current();
   }
 
+  public int lastChangeId() {
+    return changeSeq.last();
+  }
+
+  public int lastGroupId() {
+    return groupSeq.last();
+  }
+
+  public int lastAccountId() {
+    return accountSeq.last();
+  }
+
   public void setChangeIdValue(int value) {
     changeSeq.storeNew(value);
   }
diff --git a/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java b/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java
index 2e29bbd4f6..e380ef13dd 100644
--- a/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java
+++ b/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java
@@ -30,6 +30,7 @@ import com.google.gerrit.server.account.AccountState;
 import com.google.gerrit.server.index.account.AccountIndexCollection;
 import com.google.gerrit.server.index.account.AccountIndexRewriter;
 import com.google.gerrit.server.index.account.AccountSchemaDefinitions;
+import com.google.gerrit.server.notedb.Sequences;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 
@@ -41,6 +42,7 @@ import com.google.inject.Provider;
  */
 public class AccountQueryProcessor extends QueryProcessor<AccountState> {
   private final AccountControl.Factory accountControlFactory;
+  private final Sequences sequences;
 
   static {
     // It is assumed that basic rewrites do not touch visibleto predicates.
@@ -57,7 +59,8 @@ public class AccountQueryProcessor extends QueryProcessor<AccountState> {
       IndexConfig indexConfig,
       AccountIndexCollection indexes,
       AccountIndexRewriter rewriter,
-      AccountControl.Factory accountControlFactory) {
+      AccountControl.Factory accountControlFactory,
+      Sequences sequences) {
     super(
         metricMaker,
         AccountSchemaDefinitions.INSTANCE,
@@ -67,6 +70,7 @@ public class AccountQueryProcessor extends QueryProcessor<AccountState> {
         FIELD_LIMIT,
         () -> limitsFactory.create(userProvider.get()).getQueryLimit());
     this.accountControlFactory = accountControlFactory;
+    this.sequences = sequences;
   }
 
   @Override
@@ -79,4 +83,14 @@ public class AccountQueryProcessor extends QueryProcessor<AccountState> {
   protected String formatForLogging(AccountState accountState) {
     return accountState.account().id().toString();
   }
+
+  @Override
+  protected int getIndexSize() {
+    return sequences.lastAccountId();
+  }
+
+  @Override
+  protected int getBatchSize() {
+    return sequences.accountBatchSize();
+  }
 }
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java b/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
index 370bc75f51..9677321090 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
@@ -42,6 +42,7 @@ import com.google.gerrit.server.index.change.ChangeIndexCollection;
 import com.google.gerrit.server.index.change.ChangeIndexRewriter;
 import com.google.gerrit.server.index.change.ChangeSchemaDefinitions;
 import com.google.gerrit.server.index.change.IndexedChangeQuery;
+import com.google.gerrit.server.notedb.Sequences;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import java.util.ArrayList;
@@ -65,6 +66,7 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
   private final Map<String, DynamicBean> dynamicBeans = new HashMap<>();
   private final List<Extension<ChangePluginDefinedInfoFactory>>
       changePluginDefinedInfoFactoriesByPlugin = new ArrayList<>();
+  private final Sequences sequences;
 
   static {
     // It is assumed that basic rewrites do not touch visibleto predicates.
@@ -82,6 +84,7 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
       ChangeIndexCollection indexes,
       ChangeIndexRewriter rewriter,
       DynamicSet<ChangeAttributeFactory> attributeFactories,
+      Sequences sequences,
       ChangeIsVisibleToPredicate.Factory changeIsVisibleToPredicateFactory,
       DynamicSet<ChangePluginDefinedInfoFactory> changePluginDefinedInfoFactories) {
     super(
@@ -94,6 +97,7 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
         () -> limitsFactory.create(userProvider.get()).getQueryLimit());
     this.userProvider = userProvider;
     this.changeIsVisibleToPredicateFactory = changeIsVisibleToPredicateFactory;
+    this.sequences = sequences;
 
     ImmutableListMultimap.Builder<String, ChangeAttributeFactory> factoriesBuilder =
         ImmutableListMultimap.builder();
@@ -163,4 +167,14 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
   protected String formatForLogging(ChangeData changeData) {
     return changeData.getId().toString();
   }
+
+  @Override
+  protected int getIndexSize() {
+    return sequences.lastChangeId();
+  }
+
+  @Override
+  protected int getBatchSize() {
+    return sequences.changeBatchSize();
+  }
 }
diff --git a/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java b/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java
index 86c574d856..ed687f3ead 100644
--- a/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java
+++ b/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java
@@ -30,6 +30,7 @@ import com.google.gerrit.server.group.InternalGroup;
 import com.google.gerrit.server.index.group.GroupIndexCollection;
 import com.google.gerrit.server.index.group.GroupIndexRewriter;
 import com.google.gerrit.server.index.group.GroupSchemaDefinitions;
+import com.google.gerrit.server.notedb.Sequences;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 
@@ -42,6 +43,7 @@ import com.google.inject.Provider;
 public class GroupQueryProcessor extends QueryProcessor<InternalGroup> {
   private final Provider<CurrentUser> userProvider;
   private final GroupControl.GenericFactory groupControlFactory;
+  private final Sequences sequences;
 
   static {
     // It is assumed that basic rewrites do not touch visibleto predicates.
@@ -58,7 +60,8 @@ public class GroupQueryProcessor extends QueryProcessor<InternalGroup> {
       IndexConfig indexConfig,
       GroupIndexCollection indexes,
       GroupIndexRewriter rewriter,
-      GroupControl.GenericFactory groupControlFactory) {
+      GroupControl.GenericFactory groupControlFactory,
+      Sequences sequences) {
     super(
         metricMaker,
         GroupSchemaDefinitions.INSTANCE,
@@ -69,6 +72,7 @@ public class GroupQueryProcessor extends QueryProcessor<InternalGroup> {
         () -> limitsFactory.create(userProvider.get()).getQueryLimit());
     this.userProvider = userProvider;
     this.groupControlFactory = groupControlFactory;
+    this.sequences = sequences;
   }
 
   @Override
@@ -81,4 +85,14 @@ public class GroupQueryProcessor extends QueryProcessor<InternalGroup> {
   protected String formatForLogging(InternalGroup internalGroup) {
     return internalGroup.getGroupUUID().get();
   }
+
+  @Override
+  protected int getIndexSize() {
+    return sequences.lastGroupId();
+  }
+
+  @Override
+  protected int getBatchSize() {
+    return sequences.groupBatchSize();
+  }
 }
diff --git a/java/com/google/gerrit/server/query/project/ProjectQueryProcessor.java b/java/com/google/gerrit/server/query/project/ProjectQueryProcessor.java
index 66eab7b1f3..11783fceed 100644
--- a/java/com/google/gerrit/server/query/project/ProjectQueryProcessor.java
+++ b/java/com/google/gerrit/server/query/project/ProjectQueryProcessor.java
@@ -30,6 +30,7 @@ import com.google.gerrit.metrics.MetricMaker;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.account.AccountLimits;
 import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.project.ProjectCache;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 
@@ -42,6 +43,7 @@ import com.google.inject.Provider;
 public class ProjectQueryProcessor extends QueryProcessor<ProjectData> {
   private final PermissionBackend permissionBackend;
   private final Provider<CurrentUser> userProvider;
+  private final ProjectCache projectCache;
 
   static {
     // It is assumed that basic rewrites do not touch visibleto predicates.
@@ -58,7 +60,8 @@ public class ProjectQueryProcessor extends QueryProcessor<ProjectData> {
       IndexConfig indexConfig,
       ProjectIndexCollection indexes,
       ProjectIndexRewriter rewriter,
-      PermissionBackend permissionBackend) {
+      PermissionBackend permissionBackend,
+      ProjectCache projectCache) {
     super(
         metricMaker,
         ProjectSchemaDefinitions.INSTANCE,
@@ -69,6 +72,7 @@ public class ProjectQueryProcessor extends QueryProcessor<ProjectData> {
         () -> limitsFactory.create(userProvider.get()).getQueryLimit());
     this.permissionBackend = permissionBackend;
     this.userProvider = userProvider;
+    this.projectCache = projectCache;
   }
 
   @Override
@@ -81,4 +85,14 @@ public class ProjectQueryProcessor extends QueryProcessor<ProjectData> {
   protected String formatForLogging(ProjectData projectData) {
     return projectData.getProject().getName();
   }
+
+  @Override
+  protected int getIndexSize() {
+    return projectCache.all().size();
+  }
+
+  @Override
+  protected int getBatchSize() {
+    return 1;
+  }
 }
diff --git a/javatests/com/google/gerrit/server/notedb/RepoSequenceTest.java b/javatests/com/google/gerrit/server/notedb/RepoSequenceTest.java
index a768eaf02d..0c9f731437 100644
--- a/javatests/com/google/gerrit/server/notedb/RepoSequenceTest.java
+++ b/javatests/com/google/gerrit/server/notedb/RepoSequenceTest.java
@@ -91,27 +91,37 @@ public class RepoSequenceTest {
     assertThat(s.acquireCount).isEqualTo(0);
 
     assertThat(s.next()).isEqualTo(1);
+    assertThat(s.last()).isEqualTo(1);
     assertThat(s.acquireCount).isEqualTo(1);
     assertThat(s.next()).isEqualTo(2);
+    assertThat(s.last()).isEqualTo(2);
     assertThat(s.acquireCount).isEqualTo(1);
     assertThat(s.next()).isEqualTo(3);
+    assertThat(s.last()).isEqualTo(3);
     assertThat(s.acquireCount).isEqualTo(1);
 
     assertThat(s.next()).isEqualTo(4);
+    assertThat(s.last()).isEqualTo(4);
     assertThat(s.acquireCount).isEqualTo(2);
     assertThat(s.next()).isEqualTo(5);
+    assertThat(s.last()).isEqualTo(5);
     assertThat(s.acquireCount).isEqualTo(2);
     assertThat(s.next()).isEqualTo(6);
+    assertThat(s.last()).isEqualTo(6);
     assertThat(s.acquireCount).isEqualTo(2);
 
     assertThat(s.next()).isEqualTo(7);
+    assertThat(s.last()).isEqualTo(7);
     assertThat(s.acquireCount).isEqualTo(3);
     assertThat(s.next()).isEqualTo(8);
+    assertThat(s.last()).isEqualTo(8);
     assertThat(s.acquireCount).isEqualTo(3);
     assertThat(s.next()).isEqualTo(9);
+    assertThat(s.last()).isEqualTo(9);
     assertThat(s.acquireCount).isEqualTo(3);
 
     assertThat(s.next()).isEqualTo(10);
+    assertThat(s.last()).isEqualTo(10);
     assertThat(s.acquireCount).isEqualTo(4);
   }
 
@@ -127,6 +137,8 @@ public class RepoSequenceTest {
     assertThat(s2.next()).isEqualTo(5);
     assertThat(s1.next()).isEqualTo(3);
     assertThat(s2.next()).isEqualTo(6);
+    assertThat(s1.last()).isEqualTo(3);
+    assertThat(s2.last()).isEqualTo(6);
 
     // s2 acquires 7-9; s1 acquires 10-12.
     assertThat(s2.next()).isEqualTo(7);
@@ -135,6 +147,8 @@ public class RepoSequenceTest {
     assertThat(s1.next()).isEqualTo(11);
     assertThat(s2.next()).isEqualTo(9);
     assertThat(s1.next()).isEqualTo(12);
+    assertThat(s1.last()).isEqualTo(12);
+    assertThat(s2.last()).isEqualTo(9);
   }
 
   @Test
@@ -284,48 +298,61 @@ public class RepoSequenceTest {
   }
 
   @Test
-  public void nextWithCountOneCaller() throws Exception {
+  public void nextWithCountAndLastByOneCaller() throws Exception {
     RepoSequence s = newSequence("id", 1, 3);
     assertThat(s.next(2)).containsExactly(1, 2).inOrder();
     assertThat(s.acquireCount).isEqualTo(1);
+    assertThat(s.last()).isEqualTo(2);
     assertThat(s.next(2)).containsExactly(3, 4).inOrder();
     assertThat(s.acquireCount).isEqualTo(2);
+    assertThat(s.last()).isEqualTo(4);
     assertThat(s.next(2)).containsExactly(5, 6).inOrder();
     assertThat(s.acquireCount).isEqualTo(2);
+    assertThat(s.last()).isEqualTo(6);
 
     assertThat(s.next(3)).containsExactly(7, 8, 9).inOrder();
     assertThat(s.acquireCount).isEqualTo(3);
+    assertThat(s.last()).isEqualTo(9);
     assertThat(s.next(3)).containsExactly(10, 11, 12).inOrder();
     assertThat(s.acquireCount).isEqualTo(4);
+    assertThat(s.last()).isEqualTo(12);
     assertThat(s.next(3)).containsExactly(13, 14, 15).inOrder();
     assertThat(s.acquireCount).isEqualTo(5);
+    assertThat(s.last()).isEqualTo(15);
 
     assertThat(s.next(7)).containsExactly(16, 17, 18, 19, 20, 21, 22).inOrder();
     assertThat(s.acquireCount).isEqualTo(6);
+    assertThat(s.last()).isEqualTo(22);
     assertThat(s.next(7)).containsExactly(23, 24, 25, 26, 27, 28, 29).inOrder();
     assertThat(s.acquireCount).isEqualTo(7);
+    assertThat(s.last()).isEqualTo(29);
     assertThat(s.next(7)).containsExactly(30, 31, 32, 33, 34, 35, 36).inOrder();
     assertThat(s.acquireCount).isEqualTo(8);
+    assertThat(s.last()).isEqualTo(36);
   }
 
   @Test
-  public void nextWithCountMultipleCallers() throws Exception {
+  public void nextWithCountAndLastByMultipleCallers() throws Exception {
     RepoSequence s1 = newSequence("id", 1, 3);
     RepoSequence s2 = newSequence("id", 1, 4);
 
     assertThat(s1.next(2)).containsExactly(1, 2).inOrder();
+    assertThat(s1.last()).isEqualTo(2);
     assertThat(s1.acquireCount).isEqualTo(1);
 
     // s1 hasn't exhausted its last batch.
     assertThat(s2.next(2)).containsExactly(4, 5).inOrder();
+    assertThat(s2.last()).isEqualTo(5);
     assertThat(s2.acquireCount).isEqualTo(1);
 
     // s1 acquires again to cover this request, plus a whole new batch.
     assertThat(s1.next(3)).containsExactly(3, 8, 9);
+    assertThat(s1.last()).isEqualTo(9);
     assertThat(s1.acquireCount).isEqualTo(2);
 
     // s2 hasn't exhausted its last batch, do so now.
     assertThat(s2.next(2)).containsExactly(6, 7);
+    assertThat(s2.last()).isEqualTo(7);
     assertThat(s2.acquireCount).isEqualTo(1);
   }