opendev/gerrit
Code Issues Proposed changes

Bulk load change and patch set data, reuse approvals

Browse Source 
When querying for change information, try to bulk load any change
records or current patch set records that are not yet loaded, and
reuse any patch set approvals to avoid repeated scans of the same
data during a single query.

Change-Id: I4656925c4d2cf406d0a3be99fd554aa505af6e34
This commit is contained in:
Shawn O. Pearce
2012-05-01 17:54:23 -07:00
parent ff29dca4de
commit 31fc8efcd8
6 changed files with 99 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java
View File

@@ -29,6 +29,7 @@ import com.google.gerrit.server.patch.PatchListKey;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.query.change.ChangeData;
import com.googlecode.prolog_cafe.lang.Prolog;
import com.googlecode.prolog_cafe.lang.SystemException;
@@ -40,6 +41,7 @@ import org.eclipse.jgit.lib.Repository;
public final class StoredValues {
public static final StoredValue<ReviewDb> REVIEW_DB = create(ReviewDb.class);
public static final StoredValue<Change> CHANGE = create(Change.class);
public static final StoredValue<ChangeData> CHANGE_DATA = create(ChangeData.class);
public static final StoredValue<PatchSet> PATCH_SET = create(PatchSet.class);
public static final StoredValue<ChangeControl> CHANGE_CONTROL = create(ChangeControl.class);

46
gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
View File

@@ -26,10 +26,11 @@ import com.google.gerrit.rules.PrologEnvironment;
import com.google.gerrit.rules.StoredValues;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.ResultSet;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.util.Providers;
import com.googlecode.prolog_cafe.compiler.CompileException;
import com.googlecode.prolog_cafe.lang.IntegerTerm;
@@ -49,6 +50,8 @@ import java.util.HashSet;
import java.util.List;
import java.util.Set;
import javax.annotation.Nullable;
/** Access control management for a user accessing a single change. */
public class ChangeControl {
@@ -161,7 +164,7 @@ public class ChangeControl {
/** Can this user see this change? */
public boolean isVisible(ReviewDb db) throws OrmException {
if (change.getStatus() == Change.Status.DRAFT && !isDraftVisible(db)) {
if (change.getStatus() == Change.Status.DRAFT && !isDraftVisible(db, null)) {
return false;
}
return isRefVisible();
@@ -174,7 +177,7 @@ public class ChangeControl {
/** Can this user see the given patchset? */
public boolean isPatchVisible(PatchSet ps, ReviewDb db) throws OrmException {
if (ps.isDraft() && !isDraftVisible(db)) {
if (ps.isDraft() && !isDraftVisible(db, null)) {
return false;
}
return isVisible(db);
@@ -235,10 +238,20 @@ public class ChangeControl {
/** Is this user a reviewer for the change? */
public boolean isReviewer(ReviewDb db) throws OrmException {
return isReviewer(db, null);
}
/** Is this user a reviewer for the change? */
public boolean isReviewer(ReviewDb db, @Nullable ChangeData cd)
throws OrmException {
if (getCurrentUser() instanceof IdentifiedUser) {
final IdentifiedUser user = (IdentifiedUser) getCurrentUser();
ResultSet<PatchSetApproval> results =
db.patchSetApprovals().byChange(change.getId());
Iterable<PatchSetApproval> results;
if (cd != null) {
results = cd.currentApprovals(Providers.of(db));
} else {
results = db.patchSetApprovals().byChange(change.getId());
}
for (PatchSetApproval approval : results) {
if (user.getAccountId().equals(approval.getAccountId())) {
return true;
@@ -279,6 +292,11 @@ public class ChangeControl {
}
public List<SubmitRecord> canSubmit(ReviewDb db, PatchSet patchSet) {
return canSubmit(db, patchSet, null);
}
public List<SubmitRecord> canSubmit(ReviewDb db, PatchSet patchSet,
@Nullable ChangeData cd) {
if (change.getStatus().isClosed()) {
SubmitRecord rec = new SubmitRecord();
rec.status = SubmitRecord.Status.CLOSED;
@@ -291,14 +309,14 @@ public class ChangeControl {
try {
if (change.getStatus() == Change.Status.DRAFT) {
if (!isVisible(db)) {
if (!isDraftVisible(db, cd)) {
return ruleError("Patch set " + patchSet.getPatchSetId() + " not found");
} else {
return ruleError("Cannot submit draft changes");
}
}
if (patchSet.isDraft()) {
if (!isVisible(db)) {
if (!isDraftVisible(db, cd)) {
return ruleError("Patch set " + patchSet.getPatchSetId() + " not found");
} else {
return ruleError("Cannot submit draft patch sets");
@@ -323,6 +341,7 @@ public class ChangeControl {
try {
env.set(StoredValues.REVIEW_DB, db);
env.set(StoredValues.CHANGE, change);
env.set(StoredValues.CHANGE_DATA, cd);
env.set(StoredValues.PATCH_SET, patchSet);
env.set(StoredValues.CHANGE_CONTROL, this);
@@ -515,16 +534,9 @@ public class ChangeControl {
}
}
private boolean isDraftVisible(ReviewDb db) throws OrmException {
return isOwner() || isReviewer(db);
}
private boolean isDraftPatchSet(PatchSet.Id id, ReviewDb db) throws OrmException {
PatchSet ps = db.patchSets().get(id);
if (ps == null) {
throw new OrmException("Patch set " + id + " not found");
}
return ps.isDraft();
private boolean isDraftVisible(ReviewDb db, ChangeData cd)
throws OrmException {
return isOwner() || isReviewer(db, cd);
}
private static boolean isUser(Term who) {

56
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
View File

@@ -14,6 +14,8 @@
package com.google.gerrit.server.query.change;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.Patch;
@@ -47,9 +49,42 @@ import java.util.List;
import java.util.Map;
public class ChangeData {
public static void ensureChangeLoaded(
Provider<ReviewDb> db, List<ChangeData> changes) throws OrmException {
Map<Change.Id, ChangeData> missing = Maps.newHashMap();
for (ChangeData cd : changes) {
if (cd.change == null) {
missing.put(cd.getId(), cd);
}
}
if (!missing.isEmpty()) {
for (Change change : db.get().changes().get(missing.keySet())) {
missing.get(change.getId()).change = change;
}
}
}
public static void ensureCurrentPatchSetLoaded(
Provider<ReviewDb> db, List<ChangeData> changes) throws OrmException {
Map<PatchSet.Id, ChangeData> missing = Maps.newHashMap();
for (ChangeData cd : changes) {
if (cd.currentPatchSet == null && cd.patches == null) {
missing.put(cd.change(db).currentPatchSetId(), cd);
}
}
if (!missing.isEmpty()) {
for (PatchSet ps : db.get().patchSets().get(missing.keySet())) {
ChangeData cd = missing.get(ps.getId());
cd.currentPatchSet = ps;
cd.patches = Lists.newArrayList(ps);
}
}
}
private final Change.Id legacyId;
private Change change;
private String commitMessage;
private PatchSet currentPatchSet;
private Collection<PatchSet> patches;
private Collection<PatchSetApproval> approvals;
private Map<PatchSet.Id,Collection<PatchSetApproval>> approvalsMap;
@@ -144,16 +179,19 @@ public class ChangeData {
}
public PatchSet currentPatchSet(Provider<ReviewDb> db) throws OrmException {
if (currentPatchSet == null) {
Change c = change(db);
if (c == null) {
return null;
}
for (PatchSet p : patches(db)) {
if (p.getId().equals(c.currentPatchSetId())) {
currentPatchSet = p;
return p;
}
}
return null;
}
return currentPatchSet;
}
public Collection<PatchSetApproval> currentApprovals(Provider<ReviewDb> db)
@@ -162,24 +200,16 @@ public class ChangeData {
Change c = change(db);
if (c == null) {
currentApprovals = Collections.emptyList();
} else if (approvals != null) {
currentApprovals = approvalsMap(db).get(c.currentPatchSetId());
} else {
currentApprovals = approvalsFor(db, c.currentPatchSetId());
currentApprovals = db.get().patchSetApprovals()
.byPatchSet(c.currentPatchSetId()).toList();
}
}
return currentApprovals;
}
public Collection<PatchSetApproval> approvalsFor(Provider<ReviewDb> db,
PatchSet.Id psId) throws OrmException {
List<PatchSetApproval> r = new ArrayList<PatchSetApproval>();
for (PatchSetApproval p : approvals(db)) {
if (p.getPatchSetId().equals(psId)) {
r.add(p);
}
}
return r;
}
public String commitMessage(GitRepositoryManager repoManager,
Provider<ReviewDb> db) throws IOException, OrmException {
if (commitMessage == null) {

9
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ListChanges.java
View File

@@ -43,6 +43,7 @@ import org.kohsuke.args4j.Option;
import java.io.IOException;
import java.io.Writer;
import java.sql.Timestamp;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
@@ -135,6 +136,8 @@ public class ListChanges {
changes = changes.subList(0, imp.getLimit());
}
}
ChangeData.ensureChangeLoaded(db, changes);
ChangeData.ensureCurrentPatchSetLoaded(db, changes);
List<ChangeInfo> info = Lists.newArrayListWithCapacity(changes.size());
for (ChangeData cd : changes) {
@@ -231,7 +234,7 @@ public class ListChanges {
PatchSet ps = cd.currentPatchSet(db);
Map<String, LabelInfo> labels = Maps.newLinkedHashMap();
for (SubmitRecord rec : ctl.canSubmit(db.get(), ps)) {
for (SubmitRecord rec : ctl.canSubmit(db.get(), ps, cd)) {
if (rec.labels == null) {
continue;
}
@@ -253,7 +256,7 @@ public class ListChanges {
}
}
List<PatchSetApproval> approvals = null;
Collection<PatchSetApproval> approvals = null;
for (Map.Entry<String, LabelInfo> e : labels.entrySet()) {
if (e.getValue().approved != null || e.getValue().rejected != null) {
continue;
@@ -273,7 +276,7 @@ public class ListChanges {
}
if (approvals == null) {
approvals = db.get().patchSetApprovals().byPatchSet(ps.getId()).toList();
approvals = cd.currentApprovals(db);
}
for (PatchSetApproval psa : approvals) {
short val = psa.getValue();

2
gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java
View File

@@ -279,7 +279,7 @@ public class QueryProcessor {
if (current != null) {
c.currentPatchSet = eventFactory.asPatchSetAttribute(current);
eventFactory.addApprovals(c.currentPatchSet, //
d.approvalsFor(db, current.getId()));
d.currentApprovals(db));
if (includeFiles) {
eventFactory.addPatchSetFileNames(c.currentPatchSet,

12
gerrit-server/src/main/java/gerrit/PRED__load_commit_labels_1.java
View File

@@ -9,7 +9,9 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.rules.PrologEnvironment;
import com.google.gerrit.rules.StoredValues;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.google.inject.util.Providers;
import com.googlecode.prolog_cafe.lang.IntegerTerm;
import com.googlecode.prolog_cafe.lang.JavaException;
@@ -45,9 +47,17 @@ class PRED__load_commit_labels_1 extends Predicate.P1 {
PrologEnvironment env = (PrologEnvironment) engine.control;
ReviewDb db = StoredValues.REVIEW_DB.get(engine);
PatchSet patchSet = StoredValues.PATCH_SET.get(engine);
ChangeData cd = StoredValues.CHANGE_DATA.getOrNull(engine);
ApprovalTypes types = env.getInjector().getInstance(ApprovalTypes.class);
for (PatchSetApproval a : db.patchSetApprovals().byPatchSet(patchSet.getId())) {
Iterable<PatchSetApproval> approvals;
if (cd != null) {
approvals = cd.currentApprovals(Providers.of(db));
} else {
approvals = db.patchSetApprovals().byPatchSet(patchSet.getId());
}
for (PatchSetApproval a : approvals) {
if (a.getValue() == 0) {
continue;
}