Merge branch 'stable-3.6'
* stable-3.6:
Improve test coverage for no-limit query pagination
Improve test coverage for index query pagination
Paginate no-limit queries
Fix pagination to stop querying when there are no more results
Avoid fetching more results than needed in paginated index queries
Protect query limit effectively against integer overflows
Fix autocomplete for notification in settings
Add missing return values in MetricMaker
Make groups have slightly more room for long group names
Provide configuration to paginate index queries with increasing size
Introduce a SEARCH_AFTER index pagination type
ChangeIsVisibleToPredicate: Only create PermissionBackend.WithUser once
Move permission filtering for All-Projects from ProjectState to ProjectConfig
Fix stacked votes in firefox
Rerender a diff if it's reconnected and avoid reusing GrDiffCursor
Documentation: remove obsolete reference to bug
Change-Id: I7d995cac9b59929bc0bbca490b1f281defbc0f1a
Release-Notes: skip
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index d4e5912..46d07df 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -3201,6 +3201,25 @@
+
Defaults to true.
+[[index.paginationType]]index.paginationType::
++
+The pagination type to use when index queries are repeated to
+obtain the next set of results. Supported values are:
++
+* `OFFSET`
++
+Index queries are repeated with a non-zero offset to obtain the
+next set of results.
++
+* `SEARCH_AFTER`
++
+Index queries are repeated using a search-after object. Index
+backends can provide their custom implementations for search-after.
+Note that, `SEARCH_AFTER` does not impact using offsets in Gerrit
+query APIs.
++
+Defaults to `OFFSET`.
+
[[index.maxLimit]]index.maxLimit::
+
Maximum limit to allow for search queries. Requesting results above this
@@ -3219,6 +3238,44 @@
+
Defaults to no limit.
+[[index.pageSizeMultiplier]]index.pageSizeMultiplier::
++
+When index queries are repeated to obtain more results, this multiplier
+will be used to determine the limit for the next query. Using a page
+multiplier allows queries to start off small and thus provide good
+latency for queries which may end up only having very few results, and
+then scaling up to have better throughput to handle queries with larger
+result sets without incurring the overhead of making as many queries as
+would be required with a smaller limit. This strategy of using a multiplier
+attempts to create a balance between latency and throughput by dynamically
+adjusting the query size to the number of results being returned by each
+query in the pagination.
++
+The larger the multiplier, the better the throughput on large queries, and
+it also improves latency on large queries by scaling up quickly. However, a
+larger multiplier can hurt latencies a bit by making the "last" query in a
+series longer than needed. The impact of this depends on how much the backend
+latency goes up when specifying a large limit and few results are returned.
+Setting link:#index.maxPageSize[index.maxPageSize] that isn't too large, can
+likely help reduce the impacts of this.
++
+For example, if the limit of the previous query was 500 and pageSizeMultiplier
+is configured to 5, the next query will have a limit of 2500.
++
+Defaults to 1 which effectively turns this feature off.
+
+[[index.maxPageSize]]index.maxPageSize::
++
+Maximum size to allow when index queries are repeated to obtain more results. Note
+that, link:#index.maxLimit[index.maxLimit] will be used to limit page size if it
+is configured to a value lower than maxPageSize.
++
+For example, if the limit of previous query was 500, pageSizeMultiplier is
+configured to 5 and maxPageSize to 2000, the next query will have a limit of
+2000 (instead of 2500).
++
+Defaults to no limit.
+
[[index.maxTerms]]index.maxTerms::
+
Maximum number of leaf terms to allow in a query. Too-large queries may
diff --git a/Documentation/user-privacy.txt b/Documentation/user-privacy.txt
index d61ee76..afedb7e 100644
--- a/Documentation/user-privacy.txt
+++ b/Documentation/user-privacy.txt
@@ -8,7 +8,7 @@
|===
| Note: Gerrit has extensive support for link:config-plugins.html[plugins]
which extend Gerrits functionality, and these plugins could access, export, or
- maniuplate user data. This document only focuses on the behavior of Gerrit
+ manipulate user data. This document only focuses on the behavior of Gerrit
core and its link:dev-core-plugins.html[core plugins].
|===
@@ -98,11 +98,6 @@
* Remove a user's e-mail from all existing commits
* Remove a user's username
-There is also a known
-link:https://bugs.chromium.org/p/gerrit/issues/detail?id=14185[bug] where a
-user's username is stored in metadata for link:user-attention-set.html[Attention
-Set].
-
## Open Source Software Limitations
@@ -110,4 +105,4 @@
required by applicable law or agreed to in writing, software distributed under
the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS
OF ANY KIND, either express or implied. See the License for the specific
-language governing permissions and limitations under the License.
\ No newline at end of file
+language governing permissions and limitations under the License.
diff --git a/java/com/google/gerrit/index/IndexConfig.java b/java/com/google/gerrit/index/IndexConfig.java
index 8676fb2..c21f32e 100644
--- a/java/com/google/gerrit/index/IndexConfig.java
+++ b/java/com/google/gerrit/index/IndexConfig.java
@@ -30,6 +30,7 @@
@AutoValue
public abstract class IndexConfig {
private static final int DEFAULT_MAX_TERMS = 1024;
+ private static final int DEFAULT_PAGE_SIZE_MULTIPLIER = 1;
public static IndexConfig createDefault() {
return builder().build();
@@ -40,7 +41,10 @@
setIfPresent(cfg, "maxLimit", b::maxLimit);
setIfPresent(cfg, "maxPages", b::maxPages);
setIfPresent(cfg, "maxTerms", b::maxTerms);
+ setIfPresent(cfg, "pageSizeMultiplier", b::pageSizeMultiplier);
+ setIfPresent(cfg, "maxPageSize", b::maxPageSize);
setTypeOrDefault(cfg, b::type);
+ setPaginationTypeOrDefault(cfg, b::paginationType);
return b;
}
@@ -56,13 +60,21 @@
setter.accept(new IndexType(type).toString());
}
+ private static void setPaginationTypeOrDefault(Config cfg, Consumer<PaginationType> setter) {
+ setter.accept(
+ cfg != null ? cfg.getEnum("index", null, "paginationType", PaginationType.OFFSET) : null);
+ }
+
public static Builder builder() {
return new AutoValue_IndexConfig.Builder()
.maxLimit(Integer.MAX_VALUE)
.maxPages(Integer.MAX_VALUE)
.maxTerms(DEFAULT_MAX_TERMS)
+ .pageSizeMultiplier(DEFAULT_PAGE_SIZE_MULTIPLIER)
+ .maxPageSize(Integer.MAX_VALUE)
.type(IndexType.getDefault())
- .separateChangeSubIndexes(false);
+ .separateChangeSubIndexes(false)
+ .paginationType(PaginationType.OFFSET);
}
@AutoValue.Builder
@@ -85,6 +97,12 @@
public abstract Builder separateChangeSubIndexes(boolean separate);
+ public abstract Builder paginationType(PaginationType type);
+
+ public abstract Builder pageSizeMultiplier(int pageSizeMultiplier);
+
+ public abstract Builder maxPageSize(int maxPageSize);
+
abstract IndexConfig autoBuild();
public IndexConfig build() {
@@ -92,6 +110,8 @@
checkLimit(cfg.maxLimit(), "maxLimit");
checkLimit(cfg.maxPages(), "maxPages");
checkLimit(cfg.maxTerms(), "maxTerms");
+ checkLimit(cfg.pageSizeMultiplier(), "pageSizeMultiplier");
+ checkLimit(cfg.maxPageSize(), "maxPageSize");
return cfg;
}
}
@@ -124,4 +144,21 @@
* Returns whether different subsets of changes may be stored in different physical sub-indexes.
*/
public abstract boolean separateChangeSubIndexes();
+
+ /**
+ * Returns pagination type to use when index queries are repeated to obtain the next set of
+ * results.
+ */
+ public abstract PaginationType paginationType();
+
+ /**
+ * Returns multiplier to be used to determine the limit when queries are repeated to obtain the
+ * next set of results.
+ */
+ public abstract int pageSizeMultiplier();
+
+ /**
+ * Returns maximum allowed limit when repeating index queries to obtain the next set of results.
+ */
+ public abstract int maxPageSize();
}
diff --git a/java/com/google/gerrit/index/PaginationType.java b/java/com/google/gerrit/index/PaginationType.java
new file mode 100644
index 0000000..e7e34fd
--- /dev/null
+++ b/java/com/google/gerrit/index/PaginationType.java
@@ -0,0 +1,29 @@
+// Copyright (C) 2022 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.index;
+
+public enum PaginationType {
+ /** Index queries are restarted at a non-zero offset to obtain the next set of results */
+ OFFSET,
+
+ /**
+ * Index queries are restarted using a search-after object. Supported index backends can provide
+ * their custom implementations for search-after.
+ *
+ * <p>For example, Lucene implementation uses the last doc from the previous search as
+ * search-after object and uses the IndexSearcher.searchAfter API to get the next set of results.
+ */
+ SEARCH_AFTER
+}
diff --git a/java/com/google/gerrit/index/QueryOptions.java b/java/com/google/gerrit/index/QueryOptions.java
index 0401dab..91c8d1a 100644
--- a/java/com/google/gerrit/index/QueryOptions.java
+++ b/java/com/google/gerrit/index/QueryOptions.java
@@ -19,15 +19,52 @@
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableSet;
import com.google.common.primitives.Ints;
+import com.google.gerrit.common.Nullable;
import java.util.Set;
import java.util.function.Function;
@AutoValue
public abstract class QueryOptions {
public static QueryOptions create(IndexConfig config, int start, int limit, Set<String> fields) {
+ return create(config, start, null, limit, config.pageSizeMultiplier(), limit, fields);
+ }
+
+ public static QueryOptions create(
+ IndexConfig config, int start, int pageSize, int limit, Set<String> fields) {
+ return create(config, start, null, pageSize, config.pageSizeMultiplier(), limit, fields);
+ }
+
+ public static QueryOptions create(
+ IndexConfig config,
+ int start,
+ int pageSize,
+ int pageSizeMultiplier,
+ int limit,
+ Set<String> fields) {
+ return create(config, start, null, pageSize, pageSizeMultiplier, limit, fields);
+ }
+
+ public static QueryOptions create(
+ IndexConfig config,
+ int start,
+ Object searchAfter,
+ int pageSize,
+ int pageSizeMultiplier,
+ int limit,
+ Set<String> fields) {
checkArgument(start >= 0, "start must be nonnegative: %s", start);
checkArgument(limit > 0, "limit must be positive: %s", limit);
- return new AutoValue_QueryOptions(config, start, limit, ImmutableSet.copyOf(fields));
+ if (searchAfter != null) {
+ checkArgument(start == 0, "start must be 0 when searchAfter is specified: %s", start);
+ }
+ return new AutoValue_QueryOptions(
+ config,
+ start,
+ searchAfter,
+ pageSize,
+ pageSizeMultiplier,
+ limit,
+ ImmutableSet.copyOf(fields));
}
public QueryOptions convertForBackend() {
@@ -36,26 +73,56 @@
int backendLimit = config().maxLimit();
int limit = Ints.saturatedCast((long) limit() + start());
limit = Math.min(limit, backendLimit);
- return create(config(), 0, limit, fields());
+ int pageSize = Math.min(Ints.saturatedCast((long) pageSize() + start()), backendLimit);
+ return create(config(), 0, null, pageSize, pageSizeMultiplier(), limit, fields());
}
public abstract IndexConfig config();
public abstract int start();
+ @Nullable
+ public abstract Object searchAfter();
+
+ public abstract int pageSize();
+
+ public abstract int pageSizeMultiplier();
+
public abstract int limit();
public abstract ImmutableSet<String> fields();
+ public QueryOptions withPageSize(int pageSize) {
+ return create(
+ config(), start(), searchAfter(), pageSize, pageSizeMultiplier(), limit(), fields());
+ }
+
public QueryOptions withLimit(int newLimit) {
- return create(config(), start(), newLimit, fields());
+ return create(
+ config(), start(), searchAfter(), pageSize(), pageSizeMultiplier(), newLimit, fields());
}
public QueryOptions withStart(int newStart) {
- return create(config(), newStart, limit(), fields());
+ return create(
+ config(), newStart, searchAfter(), pageSize(), pageSizeMultiplier(), limit(), fields());
+ }
+
+ public QueryOptions withSearchAfter(Object newSearchAfter) {
+ // Index search-after APIs don't use 'start', so set it to 0 to be safe. ElasticSearch for
+ // example, expects it to be 0 when using search-after APIs.
+ return create(
+ config(), start(), newSearchAfter, pageSize(), pageSizeMultiplier(), limit(), fields())
+ .withStart(0);
}
public QueryOptions filterFields(Function<QueryOptions, Set<String>> filter) {
- return create(config(), start(), limit(), filter.apply(this));
+ return create(
+ config(),
+ start(),
+ searchAfter(),
+ pageSize(),
+ pageSizeMultiplier(),
+ limit(),
+ filter.apply(this));
}
}
diff --git a/java/com/google/gerrit/index/query/AndSource.java b/java/com/google/gerrit/index/query/AndSource.java
index 538e11b..de24df0 100644
--- a/java/com/google/gerrit/index/query/AndSource.java
+++ b/java/com/google/gerrit/index/query/AndSource.java
@@ -20,7 +20,11 @@
import com.google.common.collect.FluentIterable;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
+import com.google.common.collect.Ordering;
import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.index.IndexConfig;
+import com.google.gerrit.index.PaginationType;
+import com.google.gerrit.index.QueryOptions;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Comparator;
@@ -33,23 +37,30 @@
private final IsVisibleToPredicate<T> isVisibleToPredicate;
private final int start;
private final int cardinality;
+ private final IndexConfig indexConfig;
- public AndSource(Collection<? extends Predicate<T>> that) {
- this(that, null, 0);
+ public AndSource(Collection<? extends Predicate<T>> that, IndexConfig indexConfig) {
+ this(that, null, 0, indexConfig);
}
- public AndSource(Predicate<T> that, IsVisibleToPredicate<T> isVisibleToPredicate) {
- this(that, isVisibleToPredicate, 0);
+ public AndSource(
+ Predicate<T> that, IsVisibleToPredicate<T> isVisibleToPredicate, IndexConfig indexConfig) {
+ this(that, isVisibleToPredicate, 0, indexConfig);
}
- public AndSource(Predicate<T> that, IsVisibleToPredicate<T> isVisibleToPredicate, int start) {
- this(ImmutableList.of(that), isVisibleToPredicate, start);
+ public AndSource(
+ Predicate<T> that,
+ IsVisibleToPredicate<T> isVisibleToPredicate,
+ int start,
+ IndexConfig indexConfig) {
+ this(ImmutableList.of(that), isVisibleToPredicate, start, indexConfig);
}
public AndSource(
Collection<? extends Predicate<T>> that,
IsVisibleToPredicate<T> isVisibleToPredicate,
- int start) {
+ int start,
+ IndexConfig indexConfig) {
super(that);
checkArgument(start >= 0, "negative start: %s", start);
this.isVisibleToPredicate = isVisibleToPredicate;
@@ -71,6 +82,7 @@
}
this.source = s;
this.cardinality = c;
+ this.indexConfig = indexConfig;
}
@Override
@@ -86,37 +98,42 @@
() -> {
List<T> r = new ArrayList<>();
T last = null;
- int nextStart = 0;
- boolean skipped = false;
+ int pageResultSize = 0;
for (T data : buffer(resultSet)) {
if (!isMatchable() || match(data)) {
r.add(data);
- } else {
- skipped = true;
}
last = data;
- nextStart++;
+ pageResultSize++;
}
- if (skipped && last != null && source instanceof Paginated) {
- // If our source is a paginated source and we skipped at
- // least one of its results, we may not have filled the full
- // limit the caller wants. Restart the source and continue.
+ if (last != null && source instanceof Paginated) {
+ // Restart source and continue if we have not filled the
+ // full limit the caller wants.
//
@SuppressWarnings("unchecked")
Paginated<T> p = (Paginated<T>) source;
- while (skipped && r.size() < p.getOptions().limit() + start) {
- skipped = false;
- ResultSet<T> next = p.restart(nextStart);
-
+ QueryOptions opts = p.getOptions();
+ final int limit = opts.limit();
+ int pageSize = opts.pageSize();
+ int pageSizeMultiplier = opts.pageSizeMultiplier();
+ Object searchAfter = resultSet.searchAfter();
+ int nextStart = pageResultSize;
+ while (pageResultSize == pageSize && r.size() < limit) {
+ pageSize = getNextPageSize(pageSize, pageSizeMultiplier);
+ ResultSet<T> next =
+ indexConfig.paginationType().equals(PaginationType.SEARCH_AFTER)
+ ? p.restart(searchAfter, pageSize)
+ : p.restart(nextStart, pageSize);
+ pageResultSize = 0;
for (T data : buffer(next)) {
if (match(data)) {
r.add(data);
- } else {
- skipped = true;
}
- nextStart++;
+ pageResultSize++;
}
+ nextStart += pageResultSize;
+ searchAfter = next.searchAfter();
}
}
@@ -193,4 +210,20 @@
private DataSource<T> toDataSource(Predicate<T> pred) {
return (DataSource<T>) pred;
}
+
+ private int getNextPageSize(int pageSize, int pageSizeMultiplier) {
+ List<Integer> possiblePageSizes = new ArrayList<>(3);
+ try {
+ possiblePageSizes.add(Math.multiplyExact(pageSize, pageSizeMultiplier));
+ } catch (ArithmeticException e) {
+ possiblePageSizes.add(Integer.MAX_VALUE);
+ }
+ if (indexConfig.maxPageSize() > 0) {
+ possiblePageSizes.add(indexConfig.maxPageSize());
+ }
+ if (indexConfig.maxLimit() > 0) {
+ possiblePageSizes.add(indexConfig.maxLimit());
+ }
+ return Ordering.natural().min(possiblePageSizes);
+ }
}
diff --git a/java/com/google/gerrit/index/query/IndexedQuery.java b/java/com/google/gerrit/index/query/IndexedQuery.java
index ffd442b..ee25ef9 100644
--- a/java/com/google/gerrit/index/query/IndexedQuery.java
+++ b/java/com/google/gerrit/index/query/IndexedQuery.java
@@ -87,19 +87,15 @@
}
@Override
- public ResultSet<T> restart(int start) {
- opts = opts.withStart(start);
- try {
- source = index.getSource(pred, opts);
- } catch (QueryParseException e) {
- // Don't need to show this exception to the user; the only thing that
- // changed about pred was its start, and any other QPEs that might happen
- // should have already thrown from the constructor.
- throw new StorageException(e);
- }
- // Don't convert start to a limit, since the caller of this method (see
- // AndSource) has calculated the actual number to skip.
- return read();
+ public ResultSet<T> restart(int start, int pageSize) {
+ opts = opts.withStart(start).withPageSize(pageSize);
+ return search();
+ }
+
+ @Override
+ public ResultSet<T> restart(Object searchAfter, int pageSize) {
+ opts = opts.withSearchAfter(searchAfter).withPageSize(pageSize);
+ return search();
}
@Override
@@ -127,4 +123,18 @@
public String toString() {
return MoreObjects.toStringHelper("index").add("p", pred).add("opts", opts).toString();
}
+
+ private ResultSet<T> search() {
+ try {
+ source = index.getSource(pred, opts);
+ } catch (QueryParseException e) {
+ // Don't need to show this exception to the user; the only thing that
+ // changed about pred was its start, and any other QPEs that might happen
+ // should have already thrown from the constructor.
+ throw new StorageException(e);
+ }
+ // Don't convert start to a limit, since the caller of this method (see
+ // AndSource) has calculated the actual number to skip.
+ return read();
+ }
}
diff --git a/java/com/google/gerrit/index/query/LazyResultSet.java b/java/com/google/gerrit/index/query/LazyResultSet.java
index f3fab5f..a7d71f0 100644
--- a/java/com/google/gerrit/index/query/LazyResultSet.java
+++ b/java/com/google/gerrit/index/query/LazyResultSet.java
@@ -53,4 +53,9 @@
@Override
public void close() {}
+
+ @Override
+ public Object searchAfter() {
+ return null;
+ }
}
diff --git a/java/com/google/gerrit/index/query/ListResultSet.java b/java/com/google/gerrit/index/query/ListResultSet.java
index 9d7eadf..f09fda0 100644
--- a/java/com/google/gerrit/index/query/ListResultSet.java
+++ b/java/com/google/gerrit/index/query/ListResultSet.java
@@ -54,4 +54,9 @@
public void close() {
results = null;
}
+
+ @Override
+ public Object searchAfter() {
+ return null;
+ }
}
diff --git a/java/com/google/gerrit/index/query/Paginated.java b/java/com/google/gerrit/index/query/Paginated.java
index e61dd53..5521990 100644
--- a/java/com/google/gerrit/index/query/Paginated.java
+++ b/java/com/google/gerrit/index/query/Paginated.java
@@ -19,5 +19,7 @@
public interface Paginated<T> {
QueryOptions getOptions();
- ResultSet<T> restart(int start);
+ ResultSet<T> restart(int start, int pageSize);
+
+ ResultSet<T> restart(Object searchAfter, int pageSize);
}
diff --git a/java/com/google/gerrit/index/query/QueryProcessor.java b/java/com/google/gerrit/index/query/QueryProcessor.java
index 3c716b9..45a00b6 100644
--- a/java/com/google/gerrit/index/query/QueryProcessor.java
+++ b/java/com/google/gerrit/index/query/QueryProcessor.java
@@ -56,7 +56,6 @@
*/
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;
@@ -231,9 +230,10 @@
checkSupportedForQueries(q);
int limit = getEffectiveLimit(q);
limits.add(limit);
+ int initialPageSize = getInitialPageSize(q);
- if (limit == getBackendSupportedLimit()) {
- limit--;
+ if (initialPageSize == getBackendSupportedLimit()) {
+ initialPageSize--;
}
int page = (start / limit) + 1;
@@ -242,10 +242,31 @@
"Cannot go beyond page " + indexConfig.maxPages() + " of results");
}
- // Always bump limit by 1, even if this results in exceeding the permitted
- // max for this user. The only way to see if there are more entities is to
- // ask for one more result from the query.
- QueryOptions opts = createOptions(indexConfig, start, limit + 1, getRequestedFields());
+ // Always bump initial page size by 1, even if this results in exceeding the
+ // permitted max for this user. The only way to see if there are more entities
+ // is to ask for one more result from the query.
+ try {
+ initialPageSize = Math.addExact(initialPageSize, 1);
+ } catch (ArithmeticException e) {
+ initialPageSize = Integer.MAX_VALUE;
+ }
+
+ // If pageSizeMultiplier is set to 1 (default), update it to 10 for no-limit queries as
+ // it helps improve performance and also prevents no-limit queries from severely degrading
+ // when pagination type is OFFSET.
+ int pageSizeMultiplier = indexConfig.pageSizeMultiplier();
+ if (isNoLimit && pageSizeMultiplier == 1) {
+ pageSizeMultiplier = 10;
+ }
+
+ QueryOptions opts =
+ createOptions(
+ indexConfig,
+ start,
+ initialPageSize,
+ pageSizeMultiplier,
+ limit,
+ getRequestedFields());
logger.atFine().log("Query options: %s", opts);
Predicate<T> pred = rewriter.rewrite(q, opts);
if (enforceVisibility) {
@@ -319,8 +340,14 @@
}
protected QueryOptions createOptions(
- IndexConfig indexConfig, int start, int limit, Set<String> requestedFields) {
- return QueryOptions.create(indexConfig, start, limit, requestedFields);
+ IndexConfig indexConfig,
+ int start,
+ int pageSize,
+ int pageSizeMultiplier,
+ int limit,
+ Set<String> requestedFields) {
+ return QueryOptions.create(
+ indexConfig, start, pageSize, pageSizeMultiplier, limit, requestedFields);
}
/**
@@ -365,10 +392,7 @@
return indexConfig.maxLimit();
}
- private int getEffectiveLimit(Predicate<T> p) {
- if (isNoLimit == true) {
- return getIndexSize() + MAX_LIMIT_BUFFER_MULTIPLIER * getBatchSize();
- }
+ public int getInitialPageSize(Predicate<T> p) {
List<Integer> possibleLimits = new ArrayList<>(4);
possibleLimits.add(getBackendSupportedLimit());
possibleLimits.add(getPermittedLimit());
@@ -383,10 +407,18 @@
}
int result = Ordering.natural().min(possibleLimits);
// Should have short-circuited from #query or thrown some other exception before getting here.
- checkState(result > 0, "effective limit should be positive");
+ checkState(result > 0, "effective initial page size should be positive");
+
return result;
}
+ private int getEffectiveLimit(Predicate<T> p) {
+ if (isNoLimit == true) {
+ return Integer.MAX_VALUE;
+ }
+ return getInitialPageSize(p);
+ }
+
private static Optional<QueryParseException> findQueryParseException(Throwable t) {
return Throwables.getCausalChain(t).stream()
.filter(c -> c instanceof QueryParseException)
diff --git a/java/com/google/gerrit/index/query/ResultSet.java b/java/com/google/gerrit/index/query/ResultSet.java
index 65fcd45..b4bd19e 100644
--- a/java/com/google/gerrit/index/query/ResultSet.java
+++ b/java/com/google/gerrit/index/query/ResultSet.java
@@ -49,4 +49,6 @@
* the iterator has finished.
*/
void close();
+
+ Object searchAfter();
}
diff --git a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
index a58f2b2..50efef0 100644
--- a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
+++ b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
@@ -19,6 +19,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.Change;
@@ -52,6 +53,9 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.stream.IntStream;
+import java.util.stream.Stream;
+import org.eclipse.jgit.annotations.Nullable;
import org.eclipse.jgit.lib.Config;
/**
@@ -69,12 +73,14 @@
private final String indexName;
private final Map<K, D> indexedDocuments;
+ private int queryCount;
AbstractFakeIndex(Schema<V> schema, SitePaths sitePaths, String indexName) {
this.schema = schema;
this.sitePaths = sitePaths;
this.indexName = indexName;
this.indexedDocuments = new HashMap<>();
+ this.queryCount = 0;
}
@Override
@@ -108,18 +114,33 @@
}
}
+ public int getQueryCount() {
+ return queryCount;
+ }
+
@Override
public DataSource<V> getSource(Predicate<V> p, QueryOptions opts) {
List<V> results;
synchronized (indexedDocuments) {
- results =
+ Stream<V> valueStream =
indexedDocuments.values().stream()
.map(doc -> valueFor(doc))
.filter(doc -> p.asMatchable().match(doc))
- .sorted(sortingComparator())
- .skip(opts.start())
- .limit(opts.limit())
- .collect(toImmutableList());
+ .sorted(sortingComparator());
+ if (opts.searchAfter() != null) {
+ ImmutableList<V> valueList = valueStream.collect(toImmutableList());
+ int fromIndex =
+ IntStream.range(0, valueList.size())
+ .filter(i -> keyFor(valueList.get(i)).equals(opts.searchAfter()))
+ .findFirst()
+ .orElse(-1)
+ + 1;
+ int toIndex = Math.min(fromIndex + opts.pageSize(), valueList.size());
+ results = valueList.subList(fromIndex, toIndex);
+ } else {
+ results = valueStream.skip(opts.start()).limit(opts.pageSize()).collect(toImmutableList());
+ }
+ queryCount++;
}
return new DataSource<>() {
@Override
@@ -129,12 +150,19 @@
@Override
public ResultSet<V> read() {
- return new ListResultSet<>(results);
+ return new ListResultSet<>(results) {
+ @Override
+ public Object searchAfter() {
+ @Nullable V last = Iterables.getLast(results, null);
+ return last != null ? keyFor(last) : null;
+ }
+ };
}
@Override
public ResultSet<FieldBundle> readRaw() {
ImmutableList.Builder<FieldBundle> fieldBundles = ImmutableList.builder();
+ K searchAfter = null;
for (V result : results) {
ImmutableListMultimap.Builder<String, Object> fields = ImmutableListMultimap.builder();
for (SchemaField<V, ?> field : getSchema().getSchemaFields().values()) {
@@ -148,8 +176,16 @@
}
}
fieldBundles.add(new FieldBundle(fields.build()));
+ searchAfter = keyFor(result);
}
- return new ListResultSet<>(fieldBundles.build());
+ ImmutableList<FieldBundle> resultSet = fieldBundles.build();
+ K finalSearchAfter = searchAfter;
+ return new ListResultSet<>(resultSet) {
+ @Override
+ public Object searchAfter() {
+ return finalSearchAfter;
+ }
+ };
}
};
}
diff --git a/java/com/google/gerrit/lucene/AbstractLuceneIndex.java b/java/com/google/gerrit/lucene/AbstractLuceneIndex.java
index 3e502f8..7b47248 100644
--- a/java/com/google/gerrit/lucene/AbstractLuceneIndex.java
+++ b/java/com/google/gerrit/lucene/AbstractLuceneIndex.java
@@ -523,20 +523,31 @@
private <T> ResultSet<T> readImpl(Function<Document, T> mapper) {
IndexSearcher searcher = null;
+ ScoreDoc scoreDoc = null;
try {
searcher = acquire();
- int realLimit = opts.start() + opts.limit();
- TopFieldDocs docs = searcher.search(query, realLimit, sort);
+ int realLimit = opts.start() + opts.pageSize();
+ TopFieldDocs docs =
+ opts.searchAfter() != null
+ ? searcher.searchAfter(
+ (ScoreDoc) opts.searchAfter(), query, realLimit, sort, false, false)
+ : searcher.search(query, realLimit, sort);
ImmutableList.Builder<T> b = ImmutableList.builderWithExpectedSize(docs.scoreDocs.length);
for (int i = opts.start(); i < docs.scoreDocs.length; i++) {
- ScoreDoc sd = docs.scoreDocs[i];
- Document doc = searcher.doc(sd.doc, opts.fields());
+ scoreDoc = docs.scoreDocs[i];
+ Document doc = searcher.doc(scoreDoc.doc, opts.fields());
T mapperResult = mapper.apply(doc);
if (mapperResult != null) {
b.add(mapperResult);
}
}
- return new ListResultSet<>(b.build());
+ ScoreDoc searchAfter = scoreDoc;
+ return new ListResultSet<T>(b.build()) {
+ @Override
+ public Object searchAfter() {
+ return searchAfter;
+ }
+ };
} catch (IOException e) {
throw new StorageException(e);
} finally {
diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
index 551f675..e7b2336 100644
--- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java
+++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
@@ -63,8 +63,11 @@
import java.io.IOException;
import java.nio.file.Path;
import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
+import java.util.Map;
import java.util.Set;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
@@ -348,9 +351,9 @@
final Set<String> fields = IndexUtils.changeFields(opts);
return new ChangeDataResults(
executor.submit(
- new Callable<List<Document>>() {
+ new Callable<Results>() {
@Override
- public List<Document> call() throws IOException {
+ public Results call() throws IOException {
return doRead(fields);
}
@@ -365,8 +368,12 @@
@Override
public ResultSet<FieldBundle> readRaw() {
List<Document> documents;
+ Map<ChangeSubIndex, ScoreDoc> searchAfterBySubIndex;
+
try {
- documents = doRead(IndexUtils.changeFields(opts));
+ Results r = doRead(IndexUtils.changeFields(opts));
+ documents = r.docs;
+ searchAfterBySubIndex = r.searchAfterBySubIndex;
} catch (IOException e) {
throw new StorageException(e);
}
@@ -387,29 +394,49 @@
public void close() {
// Do nothing.
}
+
+ @Override
+ public Object searchAfter() {
+ return searchAfterBySubIndex;
+ }
};
}
- private List<Document> doRead(Set<String> fields) throws IOException {
+ private Results doRead(Set<String> fields) throws IOException {
IndexSearcher[] searchers = new IndexSearcher[indexes.size()];
+ Map<ChangeSubIndex, ScoreDoc> searchAfterBySubIndex = new HashMap<>();
try {
- int realLimit = opts.start() + opts.limit();
- if (Integer.MAX_VALUE - opts.limit() < opts.start()) {
- realLimit = Integer.MAX_VALUE;
+ int realPageSize = opts.start() + opts.pageSize();
+ if (Integer.MAX_VALUE - opts.pageSize() < opts.start()) {
+ realPageSize = Integer.MAX_VALUE;
}
TopFieldDocs[] hits = new TopFieldDocs[indexes.size()];
for (int i = 0; i < indexes.size(); i++) {
- searchers[i] = indexes.get(i).acquire();
- hits[i] = searchers[i].search(query, realLimit, sort);
+ ChangeSubIndex subIndex = indexes.get(i);
+ searchers[i] = subIndex.acquire();
+ if (opts.searchAfter() != null && opts.searchAfter() instanceof HashMap) {
+ hits[i] =
+ searchers[i].searchAfter(
+ ((HashMap<ChangeSubIndex, ScoreDoc>) opts.searchAfter()).get(subIndex),
+ query,
+ realPageSize,
+ sort,
+ /* doDocScores= */ false,
+ /* doMaxScore= */ false);
+ } else {
+ hits[i] = searchers[i].search(query, realPageSize, sort);
+ }
+ searchAfterBySubIndex.put(
+ subIndex, Iterables.getLast(Arrays.asList(hits[i].scoreDocs), null));
}
- TopDocs docs = TopDocs.merge(sort, realLimit, hits);
+ TopDocs docs = TopDocs.merge(sort, realPageSize, hits);
List<Document> result = new ArrayList<>(docs.scoreDocs.length);
for (int i = opts.start(); i < docs.scoreDocs.length; i++) {
ScoreDoc sd = docs.scoreDocs[i];
result.add(searchers[sd.shardIndex].doc(sd.doc, fields));
}
- return result;
+ return new Results(result, searchAfterBySubIndex);
} finally {
for (int i = 0; i < indexes.size(); i++) {
if (searchers[i] != null) {
@@ -424,11 +451,22 @@
}
}
- private class ChangeDataResults implements ResultSet<ChangeData> {
- private final Future<List<Document>> future;
- private final Set<String> fields;
+ private static class Results {
+ List<Document> docs;
+ Map<ChangeSubIndex, ScoreDoc> searchAfterBySubIndex;
- ChangeDataResults(Future<List<Document>> future, Set<String> fields) {
+ public Results(List<Document> docs, Map<ChangeSubIndex, ScoreDoc> searchAfterBySubIndex) {
+ this.docs = docs;
+ this.searchAfterBySubIndex = searchAfterBySubIndex;
+ }
+ }
+
+ private class ChangeDataResults implements ResultSet<ChangeData> {
+ private final Future<Results> future;
+ private final Set<String> fields;
+ private Map<ChangeSubIndex, ScoreDoc> searchAfterBySubIndex;
+
+ ChangeDataResults(Future<Results> future, Set<String> fields) {
this.future = future;
this.fields = fields;
}
@@ -441,7 +479,9 @@
@Override
public ImmutableList<ChangeData> toList() {
try {
- List<Document> docs = future.get();
+ Results r = future.get();
+ List<Document> docs = r.docs;
+ searchAfterBySubIndex = r.searchAfterBySubIndex;
ImmutableList.Builder<ChangeData> result =
ImmutableList.builderWithExpectedSize(docs.size());
for (Document doc : docs) {
@@ -461,6 +501,11 @@
public void close() {
future.cancel(false /* do not interrupt Lucene */);
}
+
+ @Override
+ public Object searchAfter() {
+ return searchAfterBySubIndex;
+ }
}
private static ListMultimap<String, IndexableField> fields(Document doc, Set<String> fields) {
diff --git a/java/com/google/gerrit/metrics/MetricMaker.java b/java/com/google/gerrit/metrics/MetricMaker.java
index 42ec8a0..618d421 100644
--- a/java/com/google/gerrit/metrics/MetricMaker.java
+++ b/java/com/google/gerrit/metrics/MetricMaker.java
@@ -78,14 +78,15 @@
* @param name unique name of the metric.
* @param value only value of the metric.
* @param desc description of the metric.
+ * @return registration handle
*/
- public <V> void newConstantMetric(String name, V value, Description desc) {
+ public <V> RegistrationHandle newConstantMetric(String name, V value, Description desc) {
desc.setConstant();
@SuppressWarnings("unchecked")
Class<V> type = (Class<V>) value.getClass();
CallbackMetric0<V> metric = newCallbackMetric(name, type, desc);
- newTrigger(metric, () -> metric.set(value));
+ return newTrigger(metric, () -> metric.set(value));
}
/**
@@ -107,11 +108,12 @@
* @param valueClass type of value recorded by the metric.
* @param desc description of the metric.
* @param trigger function to compute the value of the metric.
+ * @return registration handle
*/
- public <V> void newCallbackMetric(
+ public <V> RegistrationHandle newCallbackMetric(
String name, Class<V> valueClass, Description desc, Supplier<V> trigger) {
CallbackMetric0<V> metric = newCallbackMetric(name, valueClass, desc);
- newTrigger(metric, () -> metric.set(trigger.get()));
+ return newTrigger(metric, () -> metric.set(trigger.get()));
}
/**
diff --git a/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java b/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java
index e380e00..a1756bf 100644
--- a/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java
+++ b/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java
@@ -285,7 +285,7 @@
private Predicate<ChangeData> copy(Predicate<ChangeData> in, List<Predicate<ChangeData>> all) {
if (in instanceof AndPredicate) {
- return new AndChangeSource(all);
+ return new AndChangeSource(all, config);
} else if (in instanceof OrPredicate) {
return new OrSource(all);
}
diff --git a/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java b/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
index 958876e..339d7bb 100644
--- a/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
+++ b/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
@@ -52,24 +52,40 @@
public class IndexedChangeQuery extends IndexedQuery<Change.Id, ChangeData>
implements ChangeDataSource, Matchable<ChangeData> {
public static QueryOptions oneResult() {
- return createOptions(IndexConfig.createDefault(), 0, 1, ImmutableSet.of());
+ 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) {
+ return createOptions(config, start, limit, config.pageSizeMultiplier(), limit, fields);
+ }
+
+ public static QueryOptions createOptions(
+ IndexConfig config,
+ int start,
+ int pageSize,
+ int pageSizeMultiplier,
+ int limit,
+ Set<String> fields) {
// Always include project since it is needed to load the change from NoteDb.
if (!fields.contains(CHANGE.getName()) && !fields.contains(PROJECT.getName())) {
fields = new HashSet<>(fields);
fields.add(PROJECT.getName());
}
- return QueryOptions.create(config, start, limit, fields);
+ return QueryOptions.create(config, start, pageSize, pageSizeMultiplier, limit, fields);
}
@VisibleForTesting
static QueryOptions convertOptions(QueryOptions opts) {
opts = opts.convertForBackend();
return IndexedChangeQuery.createOptions(
- opts.config(), opts.start(), opts.limit(), opts.fields());
+ opts.config(),
+ opts.start(),
+ opts.pageSize(),
+ opts.pageSizeMultiplier(),
+ opts.limit(),
+ opts.fields());
}
private final Map<ChangeData, DataSource<ChangeData>> fromSource;
@@ -110,6 +126,11 @@
public void close() {
rs.close();
}
+
+ @Override
+ public Object searchAfter() {
+ return rs.searchAfter();
+ }
};
}
diff --git a/java/com/google/gerrit/server/index/group/IndexedGroupQuery.java b/java/com/google/gerrit/server/index/group/IndexedGroupQuery.java
index cacf919..7013e27 100644
--- a/java/com/google/gerrit/server/index/group/IndexedGroupQuery.java
+++ b/java/com/google/gerrit/server/index/group/IndexedGroupQuery.java
@@ -34,13 +34,13 @@
implements DataSource<InternalGroup> {
public static QueryOptions createOptions(
- IndexConfig config, int start, int limit, Set<String> fields) {
+ IndexConfig config, int start, int pageSize, int limit, Set<String> fields) {
// Always include GroupField.UUID since it is needed to load the group from NoteDb.
if (!fields.contains(GroupField.UUID_FIELD_SPEC.getName())) {
fields = new HashSet<>(fields);
fields.add(GroupField.UUID_FIELD_SPEC.getName());
}
- return QueryOptions.create(config, start, limit, fields);
+ return QueryOptions.create(config, start, pageSize, limit, fields);
}
public IndexedGroupQuery(
diff --git a/java/com/google/gerrit/server/index/group/StalenessChecker.java b/java/com/google/gerrit/server/index/group/StalenessChecker.java
index 0bce282..72370bb 100644
--- a/java/com/google/gerrit/server/index/group/StalenessChecker.java
+++ b/java/com/google/gerrit/server/index/group/StalenessChecker.java
@@ -66,7 +66,7 @@
}
Optional<FieldBundle> result =
- i.getRaw(uuid, IndexedGroupQuery.createOptions(indexConfig, 0, 1, FIELDS));
+ i.getRaw(uuid, IndexedGroupQuery.createOptions(indexConfig, 0, 1, 1, FIELDS));
if (!result.isPresent()) {
// The document is missing in the index.
try (Repository repo = repoManager.openRepository(allUsers)) {
diff --git a/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java b/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java
index e380ef1..9893d1a 100644
--- a/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java
+++ b/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java
@@ -43,6 +43,7 @@
public class AccountQueryProcessor extends QueryProcessor<AccountState> {
private final AccountControl.Factory accountControlFactory;
private final Sequences sequences;
+ private final IndexConfig indexConfig;
static {
// It is assumed that basic rewrites do not touch visibleto predicates.
@@ -71,12 +72,13 @@
() -> limitsFactory.create(userProvider.get()).getQueryLimit());
this.accountControlFactory = accountControlFactory;
this.sequences = sequences;
+ this.indexConfig = indexConfig;
}
@Override
protected Predicate<AccountState> enforceVisibility(Predicate<AccountState> pred) {
return new AndSource<>(
- pred, new AccountIsVisibleToPredicate(accountControlFactory.get()), start);
+ pred, new AccountIsVisibleToPredicate(accountControlFactory.get()), start, indexConfig);
}
@Override
diff --git a/java/com/google/gerrit/server/query/change/AndChangeSource.java b/java/com/google/gerrit/server/query/change/AndChangeSource.java
index 749204f..98cada3 100644
--- a/java/com/google/gerrit/server/query/change/AndChangeSource.java
+++ b/java/com/google/gerrit/server/query/change/AndChangeSource.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.query.change;
+import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.index.query.AndSource;
import com.google.gerrit.index.query.IsVisibleToPredicate;
import com.google.gerrit.index.query.Predicate;
@@ -22,15 +23,16 @@
public class AndChangeSource extends AndSource<ChangeData> implements ChangeDataSource {
- public AndChangeSource(Collection<Predicate<ChangeData>> that) {
- super(that);
+ public AndChangeSource(Collection<Predicate<ChangeData>> that, IndexConfig indexConfig) {
+ super(that, indexConfig);
}
public AndChangeSource(
Predicate<ChangeData> that,
IsVisibleToPredicate<ChangeData> isVisibleToPredicate,
- int start) {
- super(that, isVisibleToPredicate, start);
+ int start,
+ IndexConfig indexConfig) {
+ super(that, isVisibleToPredicate, start, indexConfig);
}
@Override
diff --git a/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java b/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java
index ac72d15..24042ad 100644
--- a/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java
+++ b/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java
@@ -41,9 +41,8 @@
}
protected final CurrentUser user;
- protected final PermissionBackend permissionBackend;
protected final ProjectCache projectCache;
- private final Provider<AnonymousUser> anonymousUserProvider;
+ private final PermissionBackend.WithUser withUser;
@Inject
public ChangeIsVisibleToPredicate(
@@ -53,9 +52,14 @@
@Assisted CurrentUser user) {
super(ChangeQueryBuilder.FIELD_VISIBLETO, IndexUtils.describe(user));
this.user = user;
- this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
- this.anonymousUserProvider = anonymousUserProvider;
+ withUser =
+ user.isIdentifiedUser()
+ ? permissionBackend.absentUser(user.getAccountId())
+ : permissionBackend.user(
+ Optional.of(user)
+ .filter(u -> u instanceof GroupBackedUser || u instanceof InternalUser)
+ .orElseGet(anonymousUserProvider::get));
}
@Override
@@ -74,18 +78,10 @@
return false;
}
if (!projectState.get().statePermitsRead()) {
- logger.atFine().log("Filter out change %s of non-reabable project %s", cd, cd.project());
+ logger.atFine().log("Filter out change %s of non-readable project %s", cd, cd.project());
return false;
}
- PermissionBackend.WithUser withUser =
- user.isIdentifiedUser()
- ? permissionBackend.absentUser(user.getAccountId())
- : permissionBackend.user(
- Optional.of(user)
- .filter(u -> u instanceof GroupBackedUser || u instanceof InternalUser)
- .orElseGet(anonymousUserProvider::get));
-
try {
if (!withUser.change(cd).test(ChangePermission.READ)) {
logger.atFine().log("Filter out non-visisble change: %s", cd);
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java b/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
index 7d02ecd..7a2dc44a 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
@@ -63,6 +63,7 @@
private final List<Extension<ChangePluginDefinedInfoFactory>>
changePluginDefinedInfoFactoriesByPlugin = new ArrayList<>();
private final Sequences sequences;
+ private final IndexConfig indexConfig;
static {
// It is assumed that basic rewrites do not touch visibleto predicates.
@@ -93,6 +94,7 @@
this.userProvider = userProvider;
this.changeIsVisibleToPredicateFactory = changeIsVisibleToPredicateFactory;
this.sequences = sequences;
+ this.indexConfig = indexConfig;
changePluginDefinedInfoFactories
.entries()
@@ -107,8 +109,14 @@
@Override
protected QueryOptions createOptions(
- IndexConfig indexConfig, int start, int limit, Set<String> requestedFields) {
- return IndexedChangeQuery.createOptions(indexConfig, start, limit, requestedFields);
+ IndexConfig indexConfig,
+ int start,
+ int pageSize,
+ int pageSizeMultiplier,
+ int limit,
+ Set<String> requestedFields) {
+ return IndexedChangeQuery.createOptions(
+ indexConfig, start, pageSize, pageSizeMultiplier, limit, requestedFields);
}
@Override
@@ -135,7 +143,7 @@
@Override
protected Predicate<ChangeData> enforceVisibility(Predicate<ChangeData> pred) {
return new AndChangeSource(
- pred, changeIsVisibleToPredicateFactory.forUser(userProvider.get()), start);
+ pred, changeIsVisibleToPredicateFactory.forUser(userProvider.get()), start, indexConfig);
}
@Override
diff --git a/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java b/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java
index e5fb036..c6683fa 100644
--- a/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java
+++ b/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java
@@ -44,6 +44,7 @@
private final Provider<CurrentUser> userProvider;
private final GroupControl.GenericFactory groupControlFactory;
private final Sequences sequences;
+ private final IndexConfig indexConfig;
static {
// It is assumed that basic rewrites do not touch visibleto predicates.
@@ -73,12 +74,16 @@
this.userProvider = userProvider;
this.groupControlFactory = groupControlFactory;
this.sequences = sequences;
+ this.indexConfig = indexConfig;
}
@Override
protected Predicate<InternalGroup> enforceVisibility(Predicate<InternalGroup> pred) {
return new AndSource<>(
- pred, new GroupIsVisibleToPredicate(groupControlFactory, userProvider.get()), start);
+ pred,
+ new GroupIsVisibleToPredicate(groupControlFactory, userProvider.get()),
+ start,
+ indexConfig);
}
@Override
diff --git a/java/com/google/gerrit/server/query/project/ProjectQueryProcessor.java b/java/com/google/gerrit/server/query/project/ProjectQueryProcessor.java
index 5465d6d..6dafa92 100644
--- a/java/com/google/gerrit/server/query/project/ProjectQueryProcessor.java
+++ b/java/com/google/gerrit/server/query/project/ProjectQueryProcessor.java
@@ -46,6 +46,7 @@
private final PermissionBackend permissionBackend;
private final Provider<CurrentUser> userProvider;
private final ProjectCache projectCache;
+ private final IndexConfig indexConfig;
static {
// It is assumed that basic rewrites do not touch visibleto predicates.
@@ -75,12 +76,16 @@
this.permissionBackend = permissionBackend;
this.userProvider = userProvider;
this.projectCache = projectCache;
+ this.indexConfig = indexConfig;
}
@Override
protected Predicate<ProjectData> enforceVisibility(Predicate<ProjectData> pred) {
return new AndSource<>(
- pred, new ProjectIsVisibleToPredicate(permissionBackend, userProvider.get()), start);
+ pred,
+ new ProjectIsVisibleToPredicate(permissionBackend, userProvider.get()),
+ start,
+ indexConfig);
}
@Override
diff --git a/javatests/com/google/gerrit/index/query/LazyDataSourceTest.java b/javatests/com/google/gerrit/index/query/LazyDataSourceTest.java
index 7064f64..4105a1d 100644
--- a/javatests/com/google/gerrit/index/query/LazyDataSourceTest.java
+++ b/javatests/com/google/gerrit/index/query/LazyDataSourceTest.java
@@ -17,6 +17,7 @@
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.common.collect.ImmutableList;
+import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeDataSource;
import com.google.gerrit.server.query.change.OrSource;
@@ -88,11 +89,17 @@
public void close() {
// No-op
}
+
+ @Override
+ public Object searchAfter() {
+ return null;
+ }
}
@Test
public void andSourceIsLazy() {
- AndSource<ChangeData> and = new AndSource<>(ImmutableList.of(new LazyPredicate()));
+ AndSource<ChangeData> and =
+ new AndSource<>(ImmutableList.of(new LazyPredicate()), IndexConfig.createDefault());
ResultSet<ChangeData> resultSet = and.read();
assertThrows(AssertionError.class, () -> resultSet.toList());
}
diff --git a/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java b/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java
index 3a47ad8..27e5b4f 100644
--- a/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java
+++ b/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java
@@ -257,7 +257,7 @@
@SafeVarargs
private static AndChangeSource andSource(Predicate<ChangeData>... preds) {
- return new AndChangeSource(Arrays.asList(preds));
+ return new AndChangeSource(Arrays.asList(preds), IndexConfig.createDefault());
}
private Predicate<ChangeData> rewrite(Predicate<ChangeData> in) throws QueryParseException {
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index ab2b9b6..9be7772 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -199,9 +199,9 @@
@Inject protected MetaDataUpdate.Server metaDataUpdateFactory;
@Inject protected AuthRequest.Factory authRequestFactory;
@Inject protected ExternalIdFactory externalIdFactory;
+ @Inject protected ProjectOperations projectOperations;
@Inject private ProjectConfig.Factory projectConfigFactory;
- @Inject private ProjectOperations projectOperations;
protected Injector injector;
protected LifecycleManager lifecycle;
@@ -737,7 +737,7 @@
assertThat(thrown).hasMessageThat().contains("invalid value");
}
- private Change createChange(TestRepository<Repo> repo, PersonIdent person) throws Exception {
+ protected Change createChange(TestRepository<Repo> repo, PersonIdent person) throws Exception {
RevCommit commit =
repo.parseBody(repo.commit().message("message").author(person).committer(person).create());
return insert(repo, newChangeForCommit(repo, commit), null);
diff --git a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesLatestIndexVersionTest.java b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesLatestIndexVersionTest.java
index 5496f56..4dde452 100644
--- a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesLatestIndexVersionTest.java
+++ b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesLatestIndexVersionTest.java
@@ -26,4 +26,11 @@
public static Config defaultConfig() {
return IndexConfig.createForFake();
}
+
+ @ConfigSuite.Config
+ public static Config searchAfterPaginationType() {
+ Config config = defaultConfig();
+ config.setString("index", null, "paginationType", "SEARCH_AFTER");
+ return config;
+ }
}
diff --git a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesPreviousIndexVersionTest.java b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesPreviousIndexVersionTest.java
index 1610eca..95896dc 100644
--- a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesPreviousIndexVersionTest.java
+++ b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesPreviousIndexVersionTest.java
@@ -36,4 +36,11 @@
IndexConfig.createForFake())
.values());
}
+
+ @ConfigSuite.Config
+ public static Config searchAfterPaginationType() {
+ Config config = againstPreviousIndexVersion();
+ config.setString("index", null, "paginationType", "SEARCH_AFTER");
+ return config;
+ }
}
diff --git a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java
index 1e23420..01ae960 100644
--- a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java
@@ -14,10 +14,27 @@
package com.google.gerrit.server.query.change;
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
+import static com.google.gerrit.common.data.GlobalCapability.QUERY_LIMIT;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+
+import com.google.gerrit.acceptance.UseClockStep;
+import com.google.gerrit.entities.Permission;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.index.testing.AbstractFakeIndex;
+import com.google.gerrit.server.config.AllProjectsName;
+import com.google.gerrit.server.index.change.ChangeIndexCollection;
import com.google.gerrit.testing.InMemoryModule;
+import com.google.gerrit.testing.InMemoryRepositoryManager;
+import com.google.gerrit.testing.InMemoryRepositoryManager.Repo;
import com.google.inject.Guice;
+import com.google.inject.Inject;
import com.google.inject.Injector;
+import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
+import org.junit.Test;
/**
* Test against {@link com.google.gerrit.index.testing.AbstractFakeIndex}. This test might seem
@@ -25,6 +42,9 @@
* results as production indices.
*/
public abstract class FakeQueryChangesTest extends AbstractQueryChangesTest {
+ @Inject private ChangeIndexCollection changeIndexCollection;
+ @Inject protected AllProjectsName allProjects;
+
@Override
protected Injector createInjector() {
Config fakeConfig = new Config(config);
@@ -32,4 +52,57 @@
fakeConfig.setString("index", null, "type", "fake");
return Guice.createInjector(new InMemoryModule(fakeConfig));
}
+
+ @Test
+ @UseClockStep
+ @SuppressWarnings("unchecked")
+ public void stopQueryIfNoMoreResults() throws Exception {
+ // create 2 visible changes
+ TestRepository<InMemoryRepositoryManager.Repo> testRepo = createProject("repo");
+ insert(testRepo, newChange(testRepo));
+ insert(testRepo, newChange(testRepo));
+
+ // create 2 invisible changes
+ TestRepository<Repo> hiddenProject = createProject("hiddenProject");
+ insert(hiddenProject, newChange(hiddenProject));
+ insert(hiddenProject, newChange(hiddenProject));
+ projectOperations
+ .project(Project.nameKey("hiddenProject"))
+ .forUpdate()
+ .add(block(Permission.READ).ref("refs/*").group(REGISTERED_USERS))
+ .update();
+
+ AbstractFakeIndex idx = (AbstractFakeIndex) changeIndexCollection.getSearchIndex();
+ newQuery("status:new").withLimit(5).get();
+ // Since the limit of the query (i.e. 5) is more than the total number of changes (i.e. 4),
+ // only 1 index search is expected.
+ assertThat(idx.getQueryCount()).isEqualTo(1);
+ }
+
+ @Test
+ @UseClockStep
+ @SuppressWarnings("unchecked")
+ public void noLimitQueryPaginates() throws Exception {
+ TestRepository<InMemoryRepositoryManager.Repo> testRepo = createProject("repo");
+ // create 4 changes
+ insert(testRepo, newChange(testRepo));
+ insert(testRepo, newChange(testRepo));
+ insert(testRepo, newChange(testRepo));
+ insert(testRepo, newChange(testRepo));
+
+ // Set queryLimit to 2
+ projectOperations
+ .project(allProjects)
+ .forUpdate()
+ .add(allowCapability(QUERY_LIMIT).group(REGISTERED_USERS).range(0, 2))
+ .update();
+
+ AbstractFakeIndex idx = (AbstractFakeIndex) changeIndexCollection.getSearchIndex();
+
+ // 2 index searches are expected. The first index search will run with size 2 (i.e.
+ // the configured query-limit), and then we will paginate to get the remaining 2
+ // changes with the second index search.
+ newQuery("status:new").withNoLimit().get();
+ assertThat(idx.getQueryCount()).isEqualTo(2);
+ }
}
diff --git a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesLatestIndexVersionTest.java b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesLatestIndexVersionTest.java
index 52a9170..4587943 100644
--- a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesLatestIndexVersionTest.java
+++ b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesLatestIndexVersionTest.java
@@ -23,4 +23,11 @@
public static Config defaultConfig() {
return IndexConfig.createForLucene();
}
+
+ @ConfigSuite.Config
+ public static Config searchAfterPaginationType() {
+ Config config = defaultConfig();
+ config.setString("index", null, "paginationType", "SEARCH_AFTER");
+ return config;
+ }
}
diff --git a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesPreviousIndexVersionTest.java b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesPreviousIndexVersionTest.java
index 62483fa..1782697 100644
--- a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesPreviousIndexVersionTest.java
+++ b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesPreviousIndexVersionTest.java
@@ -33,4 +33,11 @@
IndexConfig.createForLucene())
.values());
}
+
+ @ConfigSuite.Config
+ public static Config searchAfterPaginationType() {
+ Config config = againstPreviousIndexVersion();
+ config.setString("index", null, "paginationType", "SEARCH_AFTER");
+ return config;
+ }
}
