Add an option to the query changes API to allow skipping faulty records
Gerrit uses a secondary index to serve requests for the dashboard.
Currently if the server fails to parse at least one record from the
index (for example because of a field containing an invalid value) the
entire request would fail. This leaves the dashboard unusable.
In this change, we add a new option named 'allow-incomplete-results' to
the 'Query Changes' API. If this option is set, the index can tolerate
handling the parsing of faulty records and create a canonical ChangeData
marked as failed to get parsed from the index. In ChangeJson, we handle
this case by creating a canonical empty entity where we only set the
{project, branch, change_id, number, owner} fields and set the subject
to "***ERROR***".
It is up to the specific index implementation to handle this option.
This is not yet handled in Lucene or Elastic Search.
Release-Notes: skip
Google-Bug-Id: b/234334827
Change-Id: I0eacf0576e929dd38535c33888382aa18190ebb3
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 25db398..ea95680 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -161,6 +161,15 @@
are filtered out. REST requests with the skip-visibility option are rejected when the current
user doesn't have the ADMINISTRATE_SERVER capability.
+The `allow-incomplete-results` query parameter can be used. This is a boolean
+parameter that can optionally be set to true. If set, the server can tolerate
+handling faulty records when parsed from the change index, for example if a
+field contained a value of a wrong format. For faulty records, the server
+will return a canonical empty record where only the fields {project, branch,
+change_id, _number, owner} are set and the subject will be set to
+"\*\**ERROR***". All other fields will be empty.
+Note that the handling of this parameter is up to the index implementation.
+
Clients are allowed to specify more than one query by setting the `q`
parameter multiple times. In this case the result is an array of
arrays, one per query in the same order the queries were given in.
diff --git a/java/com/google/gerrit/extensions/api/changes/Changes.java b/java/com/google/gerrit/extensions/api/changes/Changes.java
index d8741f5..ec392d8 100644
--- a/java/com/google/gerrit/extensions/api/changes/Changes.java
+++ b/java/com/google/gerrit/extensions/api/changes/Changes.java
@@ -16,6 +16,7 @@
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ListMultimap;
+import com.google.gerrit.common.UsedAt;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeInput;
@@ -81,6 +82,7 @@
private int limit;
private int start;
private boolean isNoLimit;
+ private boolean allowIncompleteResults;
private Set<ListChangesOption> options = EnumSet.noneOf(ListChangesOption.class);
private ListMultimap<String, String> pluginOptions = ArrayListMultimap.create();
@@ -106,6 +108,12 @@
return this;
}
+ @UsedAt(UsedAt.Project.GOOGLE)
+ public QueryRequest withAllowIncompleteResults(boolean allow) {
+ this.allowIncompleteResults = allow;
+ return this;
+ }
+
/** Set an option on the request, appending to existing options. */
public QueryRequest withOption(ListChangesOption options) {
this.options.add(options);
@@ -152,6 +160,11 @@
return start;
}
+ @UsedAt(UsedAt.Project.GOOGLE)
+ public boolean getAllowIncompleteResults() {
+ return allowIncompleteResults;
+ }
+
public Set<ListChangesOption> getOptions() {
return options;
}
diff --git a/java/com/google/gerrit/index/BUILD b/java/com/google/gerrit/index/BUILD
index ba1c8bd..8b48fc0 100644
--- a/java/com/google/gerrit/index/BUILD
+++ b/java/com/google/gerrit/index/BUILD
@@ -36,6 +36,7 @@
"//lib/antlr:java-runtime",
"//lib/auto:auto-value",
"//lib/auto:auto-value-annotations",
+ "//lib/errorprone:annotations",
"//lib/flogger:api",
],
)
diff --git a/java/com/google/gerrit/index/QueryOptions.java b/java/com/google/gerrit/index/QueryOptions.java
index 29ab6d0..bee8fa1 100644
--- a/java/com/google/gerrit/index/QueryOptions.java
+++ b/java/com/google/gerrit/index/QueryOptions.java
@@ -52,6 +52,38 @@
int pageSizeMultiplier,
int limit,
Set<String> fields) {
+ return create(
+ config,
+ start,
+ searchAfter,
+ pageSize,
+ pageSizeMultiplier,
+ limit,
+ /* allowIncompleteResults= */ false,
+ fields);
+ }
+
+ public static QueryOptions create(
+ IndexConfig config,
+ int start,
+ int pageSize,
+ int pageSizeMultiplier,
+ int limit,
+ boolean allowIncompleteResults,
+ Set<String> fields) {
+ return create(
+ config, start, null, pageSize, pageSizeMultiplier, limit, allowIncompleteResults, fields);
+ }
+
+ public static QueryOptions create(
+ IndexConfig config,
+ int start,
+ Object searchAfter,
+ int pageSize,
+ int pageSizeMultiplier,
+ int limit,
+ boolean allowIncompleteResults,
+ Set<String> fields) {
checkArgument(start >= 0, "start must be nonnegative: %s", start);
checkArgument(limit > 0, "limit must be positive: %s", limit);
if (searchAfter != null) {
@@ -64,6 +96,7 @@
pageSize,
pageSizeMultiplier,
limit,
+ allowIncompleteResults,
ImmutableSet.copyOf(fields));
}
@@ -77,7 +110,15 @@
Math.min(
Math.min(Ints.saturatedCast((long) pageSize() + start()), config().maxPageSize()),
backendLimit);
- return create(config(), 0, null, pageSize, pageSizeMultiplier(), limit, fields());
+ return create(
+ config(),
+ 0,
+ null,
+ pageSize,
+ pageSizeMultiplier(),
+ limit,
+ allowIncompleteResults(),
+ fields());
}
public abstract IndexConfig config();
@@ -93,28 +134,62 @@
public abstract int limit();
+ /**
+ * When set to true, entities that fail to get parsed from the index are replaced with a canonical
+ * erroneous record. If false, parsing would throw an exception.
+ */
+ public abstract boolean allowIncompleteResults();
+
public abstract ImmutableSet<String> fields();
public QueryOptions withPageSize(int pageSize) {
return create(
- config(), start(), searchAfter(), pageSize, pageSizeMultiplier(), limit(), fields());
+ config(),
+ start(),
+ searchAfter(),
+ pageSize,
+ pageSizeMultiplier(),
+ limit(),
+ allowIncompleteResults(),
+ fields());
}
public QueryOptions withLimit(int newLimit) {
return create(
- config(), start(), searchAfter(), pageSize(), pageSizeMultiplier(), newLimit, fields());
+ config(),
+ start(),
+ searchAfter(),
+ pageSize(),
+ pageSizeMultiplier(),
+ newLimit,
+ allowIncompleteResults(),
+ fields());
}
public QueryOptions withStart(int newStart) {
return create(
- config(), newStart, searchAfter(), pageSize(), pageSizeMultiplier(), limit(), fields());
+ config(),
+ newStart,
+ searchAfter(),
+ pageSize(),
+ pageSizeMultiplier(),
+ limit(),
+ allowIncompleteResults(),
+ 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())
+ config(),
+ start(),
+ newSearchAfter,
+ pageSize(),
+ pageSizeMultiplier(),
+ limit(),
+ allowIncompleteResults(),
+ fields())
.withStart(0);
}
@@ -126,6 +201,7 @@
pageSize(),
pageSizeMultiplier(),
limit(),
+ allowIncompleteResults(),
filter.apply(this));
}
}
diff --git a/java/com/google/gerrit/index/query/QueryProcessor.java b/java/com/google/gerrit/index/query/QueryProcessor.java
index 5dfcef3..d847a06 100644
--- a/java/com/google/gerrit/index/query/QueryProcessor.java
+++ b/java/com/google/gerrit/index/query/QueryProcessor.java
@@ -26,6 +26,7 @@
import com.google.common.collect.Ordering;
import com.google.common.flogger.FluentLogger;
import com.google.common.primitives.Ints;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.index.Index;
@@ -92,6 +93,7 @@
private boolean enforceVisibility = true;
private int userProvidedLimit;
private boolean isNoLimit;
+ private boolean allowIncompleteResults;
private Set<String> requestedFields;
protected QueryProcessor(
@@ -163,6 +165,12 @@
return this;
}
+ @CanIgnoreReturnValue
+ public QueryProcessor<T> setAllowIncompleteResults(boolean allowIncompleteResults) {
+ this.allowIncompleteResults = allowIncompleteResults;
+ return this;
+ }
+
public QueryProcessor<T> setRequestedFields(Set<String> fields) {
requestedFields = fields;
return this;
@@ -271,6 +279,7 @@
// ask for one more result from the query.
// NOTE: This is consistent to the behaviour before the introduction of pagination.`
limit == getBackendSupportedLimit() ? limit : Ints.saturatedCast((long) limit + 1),
+ allowIncompleteResults,
getRequestedFields());
logger.atFine().log("Query options: %s", opts);
// Apply index-specific rewrite first
@@ -358,9 +367,16 @@
int pageSize,
int pageSizeMultiplier,
int limit,
+ boolean allowIncompleteResults,
Set<String> requestedFields) {
return QueryOptions.create(
- indexConfig, start, pageSize, pageSizeMultiplier, limit, requestedFields);
+ indexConfig,
+ start,
+ pageSize,
+ pageSizeMultiplier,
+ limit,
+ allowIncompleteResults,
+ requestedFields);
}
/**
diff --git a/java/com/google/gerrit/server/api/changes/ChangesImpl.java b/java/com/google/gerrit/server/api/changes/ChangesImpl.java
index 6b107f1..1666820 100644
--- a/java/com/google/gerrit/server/api/changes/ChangesImpl.java
+++ b/java/com/google/gerrit/server/api/changes/ChangesImpl.java
@@ -146,6 +146,7 @@
qc.setLimit(q.getLimit());
qc.setStart(q.getStart());
qc.setNoLimit(q.getNoLimit());
+ qc.setAllowIncompleteResults(q.getAllowIncompleteResults());
for (ListChangesOption option : q.getOptions()) {
qc.addOption(option);
}
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index 81f4a17..d83962f 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -518,6 +518,13 @@
// (2) Reusing
boolean isCacheable = cacheQueryResultsByChangeNum && (i != changes.size() - 1);
ChangeData cd = changes.get(i);
+ if (cd.hasFailedParsingFromIndex()) {
+ Optional<ChangeInfo> faultyChangeInfo = createFaultyChangeInfo(cd);
+ if (faultyChangeInfo.isPresent()) {
+ changeInfos.add(faultyChangeInfo.get());
+ }
+ continue;
+ }
ChangeInfo info = cache.get(cd.getId());
if (info != null && isCacheable) {
changeInfos.add(info);
@@ -990,4 +997,25 @@
}
return ImmutableListMultimap.of();
}
+
+ /**
+ * Create an empty {@link ChangeInfo} designating a faulty record if {@link
+ * ChangeData#hasFailedParsingFromIndex()} is true.
+ *
+ * <p>Few fields are populated: project, branch, changeId, _number, subject, owner.
+ */
+ private static Optional<ChangeInfo> createFaultyChangeInfo(ChangeData cd) {
+ ChangeInfo info = new ChangeInfo();
+ Change c = cd.change();
+ if (c == null) {
+ return Optional.empty();
+ }
+ info.project = c.getProject().get();
+ info.branch = c.getDest().shortName();
+ info.changeId = c.getKey().get();
+ info._number = c.getId().get();
+ info.subject = "***ERROR***";
+ info.owner = new AccountInfo(c.getOwner().get());
+ return Optional.of(info);
+ }
}
diff --git a/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java b/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
index c5fa45a..00642a9 100644
--- a/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
+++ b/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
@@ -64,6 +64,24 @@
int pageSizeMultiplier,
int limit,
Set<String> fields) {
+ return createOptions(
+ config,
+ start,
+ pageSize,
+ pageSizeMultiplier,
+ limit,
+ /* allowIncompleteResults= */ false,
+ fields);
+ }
+
+ public static QueryOptions createOptions(
+ IndexConfig config,
+ int start,
+ int pageSize,
+ int pageSizeMultiplier,
+ int limit,
+ boolean allowIncompleteResults,
+ Set<String> fields) {
// Always include project and change id since both are needed to load the change from NoteDb.
if (!fields.contains(CHANGE_SPEC.getName())
&& !(fields.contains(PROJECT_SPEC.getName())
@@ -72,7 +90,8 @@
fields.add(PROJECT_SPEC.getName());
fields.add(NUMERIC_ID_STR_SPEC.getName());
}
- return QueryOptions.create(config, start, pageSize, pageSizeMultiplier, limit, fields);
+ return QueryOptions.create(
+ config, start, pageSize, pageSizeMultiplier, limit, allowIncompleteResults, fields);
}
@VisibleForTesting
@@ -84,6 +103,7 @@
opts.pageSize(),
opts.pageSizeMultiplier(),
opts.limit(),
+ opts.allowIncompleteResults(),
opts.fields());
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java
index 392d00a..ab9d690 100644
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -37,6 +37,7 @@
import com.google.common.flogger.FluentLogger;
import com.google.common.primitives.Ints;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.common.UsedAt;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AttentionSetUpdate;
import com.google.gerrit.entities.BranchNameKey;
@@ -444,6 +445,7 @@
private String gerritServerId;
private String changeServerId;
private ChangeNumberVirtualIdAlgorithm virtualIdFunc;
+ private Boolean failedParsingFromIndex = false;
@Inject
private ChangeData(
@@ -532,6 +534,15 @@
return allUsersName;
}
+ @UsedAt(UsedAt.Project.GOOGLE)
+ public void setFailedParsingFromIndex(Boolean val) {
+ this.failedParsingFromIndex = val;
+ }
+
+ public boolean hasFailedParsingFromIndex() {
+ return failedParsingFromIndex;
+ }
+
@VisibleForTesting
public void setCurrentFilePaths(List<String> filePaths) {
PatchSet ps = currentPatchSet();
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java b/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
index 3097224..2979170 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
@@ -124,9 +124,16 @@
int pageSize,
int pageSizeMultiplier,
int limit,
+ boolean allowIncompleteResults,
Set<String> requestedFields) {
return IndexedChangeQuery.createOptions(
- indexConfig, start, pageSize, pageSizeMultiplier, limit, requestedFields);
+ indexConfig,
+ start,
+ pageSize,
+ pageSizeMultiplier,
+ limit,
+ allowIncompleteResults,
+ requestedFields);
}
@Override
diff --git a/java/com/google/gerrit/server/restapi/change/QueryChanges.java b/java/com/google/gerrit/server/restapi/change/QueryChanges.java
index 6ce4b39..4d279b0 100644
--- a/java/com/google/gerrit/server/restapi/change/QueryChanges.java
+++ b/java/com/google/gerrit/server/restapi/change/QueryChanges.java
@@ -60,6 +60,7 @@
private Integer start;
private Boolean noLimit;
private Boolean skipVisibility;
+ private Boolean allowIncompleteResults;
@Option(
name = "--query",
@@ -112,6 +113,11 @@
skipVisibility = on;
}
+ @Option(name = "--allow-incomplete-results", usage = "Return partial results")
+ public void setAllowIncompleteResults(boolean allowIncompleteResults) {
+ this.allowIncompleteResults = allowIncompleteResults;
+ }
+
@Override
public void setDynamicBean(String plugin, DynamicOptions.DynamicBean dynamicBean) {
dynamicBeans.put(plugin, dynamicBean);
@@ -181,6 +187,9 @@
if (skipVisibility != null) {
queryProcessor.enforceVisibility(!skipVisibility);
}
+ if (allowIncompleteResults != null) {
+ queryProcessor.setAllowIncompleteResults(allowIncompleteResults);
+ }
dynamicBeans.forEach((p, b) -> queryProcessor.setDynamicBean(p, b));
if (queries == null || queries.isEmpty()) {
