opendev/gerrit
Code Issues Proposed changes

Fix related changes for excessive numbers of patch sets

Browse Source 
The first step in GetRelated is to look for all changes with a group
matching any of the groups mentioned in any of the patch sets of the
input change. The total number of groups is unbounded, and in the common
case of a 1-change series, each patch set will get its own group. This
means that the number of terms in the query grows, unbounded, with the
number of patch sets, potentially exceeding the backend-supported term
limit.

Work around this in the simplest way possible, by issuing multiple
queries and merging the results. This is a little ugly for now because
of the way that InternalQuery uses a single QueryProcessor instance, but
there are only two callers, so it's not that bad.

It is now possible for a single GetRelated call to fan out to multiple
search queries, but as this only affects changes with hundreds or
thousands of patch sets, and the fanout is only by a small factor, it
should have negligible performance impact.

Change-Id: I71b1172c0b91078d320ce56e15e1eaf218687a65
This commit is contained in:
Dave Borowitz
2018-01-05 16:22:36 -05:00
parent a66a680804
commit 776f4c43d9
5 changed files with 96 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
java/com/google/gerrit/acceptance/BUILD
View File

@@ -15,6 +15,7 @@ java_library(
"//java/com/google/gerrit/extensions/restapi/testing:restapi-test-util",
"//java/com/google/gerrit/gpg/testing:gpg-test-util",
"//java/com/google/gerrit/httpd",
"//java/com/google/gerrit/index",
"//java/com/google/gerrit/launcher",
"//java/com/google/gerrit/lucene",
"//java/com/google/gerrit/metrics",

32
java/com/google/gerrit/server/change/GetRelated.java
View File

@@ -14,11 +14,15 @@
package com.google.gerrit.server.change;
import static java.util.stream.Collectors.toSet;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.MoreObjects;
import com.google.common.collect.Lists;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.common.CommitInfo;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
@@ -38,7 +42,6 @@ import com.google.inject.Singleton;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
@@ -50,17 +53,20 @@ public class GetRelated implements RestReadView<RevisionResource> {
private final Provider<InternalChangeQuery> queryProvider;
private final PatchSetUtil psUtil;
private final RelatedChangesSorter sorter;
private final IndexConfig indexConfig;
@Inject
GetRelated(
Provider<ReviewDb> db,
Provider<InternalChangeQuery> queryProvider,
PatchSetUtil psUtil,
RelatedChangesSorter sorter) {
RelatedChangesSorter sorter,
IndexConfig indexConfig) {
this.db = db;
this.queryProvider = queryProvider;
this.psUtil = psUtil;
this.sorter = sorter;
this.indexConfig = indexConfig;
}
@Override
@@ -74,16 +80,14 @@ public class GetRelated implements RestReadView<RevisionResource> {
private List<ChangeAndCommit> getRelated(RevisionResource rsrc)
throws OrmException, IOException, PermissionBackendException {
Set<String> groups = getAllGroups(rsrc.getNotes());
Set<String> groups = getAllGroups(rsrc.getNotes(), db.get(), psUtil);
if (groups.isEmpty()) {
return Collections.emptyList();
}
List<ChangeData> cds =
queryProvider
.get()
.enforceVisibility(true)
.byProjectGroups(rsrc.getChange().getProject(), groups);
InternalChangeQuery.byProjectGroups(
queryProvider, indexConfig, rsrc.getChange().getProject(), groups);
if (cds.isEmpty()) {
return Collections.emptyList();
}
@@ -119,12 +123,14 @@ public class GetRelated implements RestReadView<RevisionResource> {
return result;
}
private Set<String> getAllGroups(ChangeNotes notes) throws OrmException {
Set<String> result = new HashSet<>();
for (PatchSet ps : psUtil.byChange(db.get(), notes)) {
result.addAll(ps.getGroups());
}
return result;
@VisibleForTesting
public static Set<String> getAllGroups(ChangeNotes notes, ReviewDb db, PatchSetUtil psUtil)
throws OrmException {
return psUtil
.byChange(db, notes)
.stream()
.flatMap(ps -> ps.getGroups().stream())
.collect(toSet());
}
private void reloadChangeIfStale(List<ChangeData> cds, PatchSet wantedPs) throws OrmException {

9
java/com/google/gerrit/server/events/EventFactory.java
View File

@@ -23,6 +23,7 @@ import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
@@ -93,6 +94,7 @@ public class EventFactory {
private final ChangeKindCache changeKindCache;
private final Provider<InternalChangeQuery> queryProvider;
private final SchemaFactory<ReviewDb> schema;
private final IndexConfig indexConfig;
@Inject
EventFactory(
@@ -105,7 +107,8 @@ public class EventFactory {
ApprovalsUtil approvalsUtil,
ChangeKindCache changeKindCache,
Provider<InternalChangeQuery> queryProvider,
SchemaFactory<ReviewDb> schema) {
SchemaFactory<ReviewDb> schema,
IndexConfig indexConfig) {
this.accountCache = accountCache;
this.emails = emails;
this.urlProvider = urlProvider;
@@ -116,6 +119,7 @@ public class EventFactory {
this.changeKindCache = changeKindCache;
this.queryProvider = queryProvider;
this.schema = schema;
this.indexConfig = indexConfig;
}
/**
@@ -313,7 +317,8 @@ public class EventFactory {
// Find changes in the same related group as this patch set, having a patch
// set whose parent matches this patch set's revision.
for (ChangeData cd :
queryProvider.get().byProjectGroups(change.getProject(), currentPs.getGroups())) {
InternalChangeQuery.byProjectGroups(
queryProvider, indexConfig, change.getProject(), currentPs.getGroups())) {
PATCH_SETS:
for (PatchSet ps : cd.patchSets()) {
RevCommit commit = rw.parseCommit(ObjectId.fromString(ps.getRevision().get()));

32
java/com/google/gerrit/server/query/change/InternalChangeQuery.java
View File

@@ -22,6 +22,7 @@ import static com.google.gerrit.server.query.change.ChangeStatusPredicate.open;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Strings;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
import com.google.gerrit.index.FieldDef;
@@ -37,10 +38,12 @@ import com.google.gerrit.server.index.change.ChangeIndexCollection;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import org.eclipse.jgit.lib.ObjectId;
@@ -275,12 +278,39 @@ public class InternalChangeQuery extends InternalQuery<ChangeData> {
return query(new SubmissionIdPredicate(cs));
}
public List<ChangeData> byProjectGroups(Project.NameKey project, Collection<String> groups)
private List<ChangeData> byProjectGroups(Project.NameKey project, Collection<String> groups)
throws OrmException {
int n = indexConfig.maxTerms() - 1;
checkArgument(groups.size() <= n, "cannot exceed %s groups", n);
List<GroupPredicate> groupPredicates = new ArrayList<>(groups.size());
for (String g : groups) {
groupPredicates.add(new GroupPredicate(g));
}
return query(and(project(project), or(groupPredicates)));
}
// Batching via multiple queries requires passing in a Provider since the underlying
// QueryProcessor instance is not reusable.
public static List<ChangeData> byProjectGroups(
Provider<InternalChangeQuery> queryProvider,
IndexConfig indexConfig,
Project.NameKey project,
Collection<String> groups)
throws OrmException {
int batchSize = indexConfig.maxTerms() - 1;
if (groups.size() <= batchSize) {
return queryProvider.get().enforceVisibility(true).byProjectGroups(project, groups);
}
Set<Change.Id> seen = new HashSet<>();
List<ChangeData> result = new ArrayList<>();
for (List<String> part : Iterables.partition(groups, batchSize)) {
for (ChangeData cd :
queryProvider.get().enforceVisibility(true).byProjectGroups(project, part)) {
if (!seen.add(cd.getId())) {
result.add(cd);
}
}
}
return result;
}
}

38
javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.server.change;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.GitUtil.assertPushOk;
import static com.google.gerrit.acceptance.GitUtil.pushHead;
import static com.google.gerrit.extensions.common.testing.EditInfoSubject.assertThat;
import static java.util.concurrent.TimeUnit.SECONDS;
@@ -29,20 +30,26 @@ import com.google.gerrit.common.RawInputUtil;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.common.CommitInfo;
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.change.ChangesCollection;
import com.google.gerrit.server.change.GetRelated;
import com.google.gerrit.server.change.GetRelated.ChangeAndCommit;
import com.google.gerrit.server.change.GetRelated.RelatedInfo;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.testing.ConfigSuite;
import com.google.gerrit.testing.TestTimeUtil;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.After;
@@ -50,6 +57,15 @@ import org.junit.Before;
import org.junit.Test;
public class GetRelatedIT extends AbstractDaemonTest {
private static final int MAX_TERMS = 10;
@ConfigSuite.Default
public static Config defaultConfig() {
Config cfg = new Config();
cfg.setInt("index", null, "maxTerms", MAX_TERMS);
return cfg;
}
private String systemTimeZone;
@Before
@@ -64,6 +80,7 @@ public class GetRelatedIT extends AbstractDaemonTest {
System.setProperty("user.timezone", systemTimeZone);
}
@Inject private IndexConfig indexConfig;
@Inject private ChangesCollection changes;
@Test
@@ -539,6 +556,27 @@ public class GetRelatedIT extends AbstractDaemonTest {
assertRelated(psId2_2, changeAndCommit(psId2_2, c2_2, 2), changeAndCommit(psId1_1, c1_1, 1));
}
@Test
public void getRelatedManyGroups() throws Exception {
List<RevCommit> commits = new ArrayList<>();
RevCommit last = null;
int n = 2 * MAX_TERMS;
assertThat(n).isGreaterThan(indexConfig.maxTerms());
for (int i = 1; i <= n; i++) {
TestRepository<?>.CommitBuilder cb = last != null ? amendBuilder() : commitBuilder();
last = cb.add("a.txt", Integer.toString(i)).message("subject: " + i).create();
testRepo.reset(last);
assertPushOk(pushHead(testRepo, "refs/for/master", false), "refs/for/master");
commits.add(last);
}
ChangeData cd = getChange(last);
assertThat(cd.patchSets().size()).isEqualTo(n);
assertThat(GetRelated.getAllGroups(cd.notes(), db, psUtil).size()).isEqualTo(n);
assertRelated(cd.change().currentPatchSetId());
}
private List<ChangeAndCommit> getRelated(PatchSet.Id ps) throws Exception {
return getRelated(ps.getParentKey(), ps.get());
}