opendev/gerrit
Code Issues Proposed changes

Render JSON results for the change table only using the index

Browse Source 
Define a new type of field, STORED_ONLY, for storing arbitrary raw
bytes that cannot be searched. Add two fields of this type storing the
Change object and the list of SubmitRecord.Labels, which is exactly
the set of data needed to render a ChangeTable2. This avoids hitting
the database in the common case of rendering dashboards and search
results in the web UI, which may be costly on non-SQL implementations
Similarly we can avoid running and re-running the submit rule
evaluator.

This implementation has a minor bug in that changes to submit rules
may invalidate previously indexed labels, as can changing the set of
plugins with PredicateProviders.

There is substantial collateral damage to Reindex stemming from the
fact that indexing now depends on the submit rule evaluator, which in
turn requires pulling in plugins and hence a lot more Guice glue.

Change-Id: I3c10ed936eb9258e453c46f947abf35f5b8fb308
This commit is contained in:
Dave Borowitz
2013-09-10 15:23:57 -07:00
parent 2474f211d4
commit 05b254feb2
10 changed files with 373 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files

52
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
View File

@@ -380,6 +380,10 @@ public class ChangeJson {
if (!standard && !detailed) {
return null;
}
if (cd.getSubmitRecordLabels() != null && standard && !detailed) {
// Use saved labels to avoid recomputing on the fly.
return labelsFromSavedState(cd.getSubmitRecordLabels());
}
ChangeControl ctl = control(cd);
if (ctl == null) {
@@ -394,6 +398,15 @@ public class ChangeJson {
}
}
private Map<String, LabelInfo> labelsFromSavedState(
List<SubmitRecord.Label> savedLabels) {
Map<String, LabelInfo> labels = Maps.newLinkedHashMap();
for (SubmitRecord.Label r : savedLabels) {
labels.put(r.label, newLabelFromSubmitRecord(r, true));
}
return labels;
}
private Map<String, LabelInfo> labelsForOpenChange(ChangeData cd,
LabelTypes labelTypes, boolean standard, boolean detailed)
throws OrmException {
@@ -434,29 +447,34 @@ public class ChangeJson {
for (SubmitRecord.Label r : rec.labels) {
LabelInfo p = labels.get(r.label);
if (p == null || p._status.compareTo(r.status) < 0) {
LabelInfo n = new LabelInfo();
n._status = r.status;
if (standard) {
switch (r.status) {
case OK:
n.approved = accountLoader.get(r.appliedBy);
break;
case REJECT:
n.rejected = accountLoader.get(r.appliedBy);
break;
default:
break;
}
}
n.optional = n._status == SubmitRecord.Label.Status.MAY ? true : null;
labels.put(r.label, n);
labels.put(r.label, newLabelFromSubmitRecord(r, standard));
}
}
}
return labels;
}
private LabelInfo newLabelFromSubmitRecord(SubmitRecord.Label r,
boolean standard) {
LabelInfo n = new LabelInfo();
n._status = r.status;
if (standard) {
switch (r.status) {
case OK:
n.approved = accountLoader.get(r.appliedBy);
break;
case REJECT:
n.rejected = accountLoader.get(r.appliedBy);
break;
default:
break;
}
}
n.optional = n._status == SubmitRecord.Label.Status.MAY ? true : null;
return n;
}
private void setLabelScores(LabelType type,
LabelInfo label, short score, Account.Id accountId)
throws OrmException {

2
gerrit-server/src/main/java/com/google/gerrit/server/config/EmailExpanderProvider.java
View File

@@ -21,7 +21,7 @@ import com.google.inject.Provider;
import org.eclipse.jgit.lib.Config;
class EmailExpanderProvider implements Provider<EmailExpander> {
public class EmailExpanderProvider implements Provider<EmailExpander> {
private final EmailExpander expander;
@Inject

108
gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java
View File

@@ -14,21 +14,35 @@
package com.google.gerrit.server.index;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchLineComment;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.SubmitRecord;
import com.google.gerrit.reviewdb.client.TrackingId;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
import com.google.gerrit.server.query.change.ChangeStatusPredicate;
import com.google.gwtorm.protobuf.CodecFactory;
import com.google.gwtorm.protobuf.ProtobufCodec;
import com.google.gwtorm.server.OrmException;
import com.google.protobuf.CodedOutputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.sql.Timestamp;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Set;
/**
@@ -233,6 +247,82 @@ public class ChangeField {
}
};
public static class ChangeProtoField extends FieldDef.Single<ChangeData, byte[]> {
public static final ProtobufCodec<Change> CODEC =
CodecFactory.encoder(Change.class);
private ChangeProtoField() {
super("_change", FieldType.STORED_ONLY, true);
}
@Override
public byte[] get(ChangeData input, FieldDef.FillArgs args)
throws OrmException {
return CODEC.encodeToByteArray(input.change(args.db));
}
}
/** Serialized change object, used for pre-populating results. */
public static final ChangeProtoField CHANGE = new ChangeProtoField();
public static class SubmitLabelProtoField
extends FieldDef.Repeatable<ChangeData, byte[]> {
public static final ProtobufCodec<SubmitRecord.Label> CODEC =
CodecFactory.encoder(SubmitRecord.Label.class);
private SubmitLabelProtoField() {
super("_label", FieldType.STORED_ONLY, true);
}
@Override
public Iterable<byte[]> get(ChangeData input, FillArgs args)
throws OrmException {
// Flatten the highest-valued labels to mimic the results from ChangeJson
// with standard labels.
Map<String, SubmitRecord.Label> labels = Maps.newLinkedHashMap();
for (SubmitRecord rec : getSubmitRecords(input, args)) {
if (rec.labels == null) {
continue;
}
for (SubmitRecord.Label r : rec.labels) {
SubmitRecord.Label p = labels.get(r.label);
if (p == null || p.status.compareTo(r.status) < 0) {
labels.put(r.label, r);
}
}
}
return toProtos(CODEC, labels.values());
}
private List<SubmitRecord> getSubmitRecords(ChangeData input,
FillArgs args) throws OrmException {
ChangeControl ctl;
try {
// Use the ChangeControl for InternalUser. This will give bogus
// results for whether or not the change is submittable, but does
// not affect label calculation.
ctl = args.changeControlFor(input.change(args.db));
} catch (NoSuchChangeException e) {
throw new OrmException(e);
}
if (ctl == null) {
return ImmutableList.of();
}
PatchSet ps = input.currentPatchSet(args.db);
if (ps == null) {
return ImmutableList.of();
}
return ctl.canSubmit(args.db.get(), ps, input, true, true, true);
}
}
/**
* Serialized labels from the submit rule evaluator, used for pre-populating
* results.
*/
public static final SubmitLabelProtoField SUBMIT_RECORD_LABEL =
new SubmitLabelProtoField();
public static String formatLabel(String label, int value) {
return formatLabel(label, value, null);
}
@@ -273,4 +363,22 @@ public class ChangeField {
return r;
}
};
private static <T> List<byte[]> toProtos(ProtobufCodec<T> codec, Collection<T> objs)
throws OrmException {
List<byte[]> result = Lists.newArrayListWithCapacity(objs.size());
ByteArrayOutputStream out = new ByteArrayOutputStream(256);
try {
for (T obj : objs) {
out.reset();
CodedOutputStream cos = CodedOutputStream.newInstance(out);
codec.encode(obj, cos);
cos.flush();
result.add(out.toByteArray());
}
} catch (IOException e) {
throw new OrmException(e);
}
return result;
}
}

22
gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeSchemas.java
View File

@@ -49,6 +49,28 @@ public class ChangeSchemas {
ChangeField.COMMIT_MESSAGE,
ChangeField.COMMENT);
@SuppressWarnings({"unchecked", "deprecation"})
static final Schema<ChangeData> V2 = release(
ChangeField.LEGACY_ID,
ChangeField.ID,
ChangeField.STATUS,
ChangeField.PROJECT,
ChangeField.REF,
ChangeField.TOPIC,
ChangeField.UPDATED,
ChangeField.SORTKEY,
ChangeField.FILE,
ChangeField.OWNER,
ChangeField.REVIEWER,
ChangeField.COMMIT,
ChangeField.TR,
ChangeField.LABEL,
ChangeField.REVIEWED,
ChangeField.COMMIT_MESSAGE,
ChangeField.COMMENT,
ChangeField.CHANGE,
ChangeField.SUBMIT_RECORD_LABEL);
private static Schema<ChangeData> release(FieldDef<ChangeData, ?>... fields) {
return new Schema<ChangeData>(true, Arrays.asList(fields));
}

27
gerrit-server/src/main/java/com/google/gerrit/server/index/FieldDef.java
View File

@@ -14,9 +14,13 @@
package com.google.gerrit.server.index;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.InternalUser;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -60,14 +64,31 @@ public abstract class FieldDef<I, T> {
final Provider<ReviewDb> db;
final GitRepositoryManager repoManager;
final PatchListCache patchListCache;
final ChangeControl.GenericFactory changeControlFactory;
private final InternalUser internalUser;
@Inject
FillArgs(Provider<ReviewDb> db,
GitRepositoryManager repoManager,
PatchListCache patchListCache) {
FillArgs(Provider<ReviewDb> db, GitRepositoryManager repoManager,
PatchListCache patchListCache,
ChangeControl.GenericFactory changeControlFactory,
InternalUser internalUser) {
this.db = db;
this.repoManager = repoManager;
this.patchListCache = patchListCache;
this.changeControlFactory = changeControlFactory;
this.internalUser = internalUser;
}
/**
* Create a change control.
*
* @param change change object.
* @return change control as seen by Gerrit's internal user; index fields
* may not depend on any per-user state.
*/
ChangeControl changeControlFor(Change change) throws NoSuchChangeException {
return changeControlFactory.controlFor(change, internalUser);
}
}

4
gerrit-server/src/main/java/com/google/gerrit/server/index/FieldType.java
View File

@@ -43,6 +43,10 @@ public class FieldType<T> {
public static final FieldType<String> FULL_TEXT =
new FieldType<String>("FULL_TEXT");
/** A field that is only stored as raw bytes and cannot be queried. */
public static final FieldType<byte[]> STORED_ONLY =
new FieldType<byte[]>("STORED_ONLY");
private final String name;
private FieldType(String name) {

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

@@ -148,6 +148,7 @@ public class ChangeData {
private ChangeControl changeControl;
private List<ChangeMessage> messages;
private List<SubmitRecord> submitRecords;
private List<SubmitRecord.Label> submitRecordLabels;
private boolean patchesLoaded;
public ChangeData(final Change.Id id) {
@@ -459,4 +460,12 @@ public class ChangeData {
public List<SubmitRecord> getSubmitRecords() {
return submitRecords;
}
public void setSubmitRecordLabels(List<SubmitRecord.Label> labels) {
submitRecordLabels = labels;
}
public List<SubmitRecord.Label> getSubmitRecordLabels() {
return submitRecordLabels;
}
}