opendev/gerrit
Code Issues Proposed changes

ChangeData: Disallow computing submit rules when serving query results

Browse Source 
ChangeData has a mechanism that prevents it from touching NoteDb when
querying changes (boolean lazyLoad). NoteDb is much slower than the
change index, so we don't want to use it when rendering dashboards.

At the same time, the change index does not have all data. Therefor
some query parameters require Gerrit to load additional data from
NoteDb. Reading this data is more expensive than reading from the
change index, but still OK-ish.

SubmitRequirements, however, might be really expensive to compute
(think of code owners). So when serving query results, we always
want to serve the values sourced from the change index.

This commit swaps the lazyload boolean for an enum to make it so.

Change-Id: I296955dc41ae378254db6c7a1b04225468206489
This commit is contained in:
Patrick Hiesel
2021-03-18 14:39:21 +01:00
parent 2425b53eed
commit c774890f15
2 changed files with 66 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
java/com/google/gerrit/server/change/ChangeJson.java
View File

@@ -405,6 +405,10 @@ public class ChangeJson {
private void ensureLoaded(Iterable<ChangeData> all) { private void ensureLoaded(Iterable<ChangeData> all) {
if (lazyLoad) { if (lazyLoad) {
for (ChangeData cd : all) {
// Mark all ChangeDatas as coming from the index, but allow backfilling data from NoteDb
cd.setStorageConstraint(ChangeData.StorageConstraint.INDEX_PRIMARY_NOTEDB_SECONDARY);
}
ChangeData.ensureChangeLoaded(all); ChangeData.ensureChangeLoaded(all);
if (has(ALL_REVISIONS)) { if (has(ALL_REVISIONS)) {
ChangeData.ensureAllPatchSetsLoaded(all); ChangeData.ensureAllPatchSetsLoaded(all);
@@ -417,7 +421,8 @@ public class ChangeJson {
ChangeData.ensureCurrentApprovalsLoaded(all); ChangeData.ensureCurrentApprovalsLoaded(all);
} else { } else {
for (ChangeData cd : all) { for (ChangeData cd : all) {
cd.setLazyLoad(false); // Mark all ChangeDatas as coming from the index. Disallow using NoteDb
cd.setStorageConstraint(ChangeData.StorageConstraint.INDEX_ONLY);
} }
} }
} }

91
java/com/google/gerrit/server/query/change/ChangeData.java
View File

@@ -33,6 +33,7 @@ import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists; import com.google.common.collect.Lists;
import com.google.common.collect.Maps; import com.google.common.collect.Maps;
import com.google.common.collect.SetMultimap; import com.google.common.collect.SetMultimap;
import com.google.common.flogger.FluentLogger;
import com.google.common.primitives.Ints; import com.google.common.primitives.Ints;
import com.google.gerrit.common.Nullable; import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.UsedAt; import com.google.gerrit.common.UsedAt;
@@ -117,6 +118,23 @@ import org.eclipse.jgit.revwalk.RevWalk;
* lazyLoad=false disables loading from NoteDb, so we don't accidentally enable a slow path. * lazyLoad=false disables loading from NoteDb, so we don't accidentally enable a slow path.
*/ */
public class ChangeData { public class ChangeData {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public enum StorageConstraint {
/**
* This instance was loaded from the change index. Backfilling missing data from NoteDb is not
* allowed.
*/
INDEX_ONLY,
/**
* This instance was loaded from the change index. Backfilling missing data from NoteDb is
* allowed.
*/
INDEX_PRIMARY_NOTEDB_SECONDARY,
/** This instance was loaded from NoteDb. */
NOTEDB_ONLY
}
public static List<Change> asChanges(List<ChangeData> changeDatas) { public static List<Change> asChanges(List<ChangeData> changeDatas) {
List<Change> result = new ArrayList<>(changeDatas.size()); List<Change> result = new ArrayList<>(changeDatas.size());
for (ChangeData cd : changeDatas) { for (ChangeData cd : changeDatas) {
@@ -282,7 +300,7 @@ public class ChangeData {
private final Map<SubmitRuleOptions, List<SubmitRecord>> submitRecords = private final Map<SubmitRuleOptions, List<SubmitRecord>> submitRecords =
Maps.newLinkedHashMapWithExpectedSize(1); Maps.newLinkedHashMapWithExpectedSize(1);
private boolean lazyLoad = true; private StorageConstraint storageConstraint = StorageConstraint.NOTEDB_ONLY;
private Change change; private Change change;
private ChangeNotes notes; private ChangeNotes notes;
private String commitMessage; private String commitMessage;
@@ -375,11 +393,17 @@ public class ChangeData {
* lazyLoad} is on, the {@code ChangeData} object will load from the database ("lazily") when a * lazyLoad} is on, the {@code ChangeData} object will load from the database ("lazily") when a
* field accessor is called. * field accessor is called.
*/ */
public ChangeData setLazyLoad(boolean load) { public ChangeData setStorageConstraint(StorageConstraint storageConstraint) {
lazyLoad = load; this.storageConstraint = storageConstraint;
return this; return this;
} }
/** Returns {@code true} if we allow reading data from NoteDb. */
public boolean lazyload() {
return storageConstraint.ordinal()
>= StorageConstraint.INDEX_PRIMARY_NOTEDB_SECONDARY.ordinal();
}
public AllUsersName getAllUsersNameForIndexing() { public AllUsersName getAllUsersNameForIndexing() {
return allUsersName; return allUsersName;
} }
@@ -394,7 +418,7 @@ public class ChangeData {
public List<String> currentFilePaths() { public List<String> currentFilePaths() {
if (currentFiles == null) { if (currentFiles == null) {
if (!lazyLoad) { if (!lazyload()) {
return Collections.emptyList(); return Collections.emptyList();
} }
Optional<DiffSummary> p = getDiffSummary(); Optional<DiffSummary> p = getDiffSummary();
@@ -405,7 +429,7 @@ public class ChangeData {
private Optional<DiffSummary> getDiffSummary() { private Optional<DiffSummary> getDiffSummary() {
if (diffSummary == null) { if (diffSummary == null) {
if (!lazyLoad) { if (!lazyload()) {
return Optional.empty(); return Optional.empty();
} }
@@ -436,7 +460,7 @@ public class ChangeData {
public Optional<ChangedLines> changedLines() { public Optional<ChangedLines> changedLines() {
if (changedLines == null) { if (changedLines == null) {
if (!lazyLoad) { if (!lazyload()) {
return Optional.empty(); return Optional.empty();
} }
changedLines = computeChangedLines(); changedLines = computeChangedLines();
@@ -469,7 +493,7 @@ public class ChangeData {
} }
public Change change() { public Change change() {
if (change == null && lazyLoad) { if (change == null && lazyload()) {
reloadChange(); reloadChange();
} }
return change; return change;
@@ -500,7 +524,7 @@ public class ChangeData {
public ChangeNotes notes() { public ChangeNotes notes() {
if (notes == null) { if (notes == null) {
if (!lazyLoad) { if (!lazyload()) {
throw new StorageException("ChangeNotes not available, lazyLoad = false"); throw new StorageException("ChangeNotes not available, lazyLoad = false");
} }
notes = notesFactory.create(project(), legacyId); notes = notesFactory.create(project(), legacyId);
@@ -526,7 +550,7 @@ public class ChangeData {
public List<PatchSetApproval> currentApprovals() { public List<PatchSetApproval> currentApprovals() {
if (currentApprovals == null) { if (currentApprovals == null) {
if (!lazyLoad) { if (!lazyload()) {
return Collections.emptyList(); return Collections.emptyList();
} }
Change c = change(); Change c = change();
@@ -621,7 +645,7 @@ public class ChangeData {
/** Returns the most recent update (i.e. status) per user. */ /** Returns the most recent update (i.e. status) per user. */
public ImmutableSet<AttentionSetUpdate> attentionSet() { public ImmutableSet<AttentionSetUpdate> attentionSet() {
if (attentionSet == null) { if (attentionSet == null) {
if (!lazyLoad) { if (!lazyload()) {
return ImmutableSet.of(); return ImmutableSet.of();
} }
attentionSet = notes().getAttentionSet(); attentionSet = notes().getAttentionSet();
@@ -699,7 +723,7 @@ public class ChangeData {
*/ */
public ListMultimap<PatchSet.Id, PatchSetApproval> approvals() { public ListMultimap<PatchSet.Id, PatchSetApproval> approvals() {
if (allApprovals == null) { if (allApprovals == null) {
if (!lazyLoad) { if (!lazyload()) {
return ImmutableListMultimap.of(); return ImmutableListMultimap.of();
} }
allApprovals = approvalsUtil.byChange(notes()); allApprovals = approvalsUtil.byChange(notes());
@@ -716,7 +740,7 @@ public class ChangeData {
public ReviewerSet reviewers() { public ReviewerSet reviewers() {
if (reviewers == null) { if (reviewers == null) {
if (!lazyLoad) { if (!lazyload()) {
// We are not allowed to load values from NoteDb. Reviewers were not populated with values // We are not allowed to load values from NoteDb. Reviewers were not populated with values
// from the index. However, we need these values for permission checks. // from the index. However, we need these values for permission checks.
throw new IllegalStateException("reviewers not populated"); throw new IllegalStateException("reviewers not populated");
@@ -732,7 +756,7 @@ public class ChangeData {
public ReviewerByEmailSet reviewersByEmail() { public ReviewerByEmailSet reviewersByEmail() {
if (reviewersByEmail == null) { if (reviewersByEmail == null) {
if (!lazyLoad) { if (!lazyload()) {
return ReviewerByEmailSet.empty(); return ReviewerByEmailSet.empty();
} }
reviewersByEmail = notes().getReviewersByEmail(); reviewersByEmail = notes().getReviewersByEmail();
@@ -758,7 +782,7 @@ public class ChangeData {
public ReviewerSet pendingReviewers() { public ReviewerSet pendingReviewers() {
if (pendingReviewers == null) { if (pendingReviewers == null) {
if (!lazyLoad) { if (!lazyload()) {
return ReviewerSet.empty(); return ReviewerSet.empty();
} }
pendingReviewers = notes().getPendingReviewers(); pendingReviewers = notes().getPendingReviewers();
@@ -776,7 +800,7 @@ public class ChangeData {
public ReviewerByEmailSet pendingReviewersByEmail() { public ReviewerByEmailSet pendingReviewersByEmail() {
if (pendingReviewersByEmail == null) { if (pendingReviewersByEmail == null) {
if (!lazyLoad) { if (!lazyload()) {
return ReviewerByEmailSet.empty(); return ReviewerByEmailSet.empty();
} }
pendingReviewersByEmail = notes().getPendingReviewersByEmail(); pendingReviewersByEmail = notes().getPendingReviewersByEmail();
@@ -786,7 +810,7 @@ public class ChangeData {
public List<ReviewerStatusUpdate> reviewerUpdates() { public List<ReviewerStatusUpdate> reviewerUpdates() {
if (reviewerUpdates == null) { if (reviewerUpdates == null) {
if (!lazyLoad) { if (!lazyload()) {
return Collections.emptyList(); return Collections.emptyList();
} }
reviewerUpdates = approvalsUtil.getReviewerUpdates(notes()); reviewerUpdates = approvalsUtil.getReviewerUpdates(notes());
@@ -804,7 +828,7 @@ public class ChangeData {
public Collection<HumanComment> publishedComments() { public Collection<HumanComment> publishedComments() {
if (publishedComments == null) { if (publishedComments == null) {
if (!lazyLoad) { if (!lazyload()) {
return Collections.emptyList(); return Collections.emptyList();
} }
publishedComments = commentsUtil.publishedHumanCommentsByChange(notes()); publishedComments = commentsUtil.publishedHumanCommentsByChange(notes());
@@ -814,7 +838,7 @@ public class ChangeData {
public Collection<RobotComment> robotComments() { public Collection<RobotComment> robotComments() {
if (robotComments == null) { if (robotComments == null) {
if (!lazyLoad) { if (!lazyload()) {
return Collections.emptyList(); return Collections.emptyList();
} }
robotComments = commentsUtil.robotCommentsByChange(notes()); robotComments = commentsUtil.robotCommentsByChange(notes());
@@ -824,7 +848,7 @@ public class ChangeData {
public Integer unresolvedCommentCount() { public Integer unresolvedCommentCount() {
if (unresolvedCommentCount == null) { if (unresolvedCommentCount == null) {
if (!lazyLoad) { if (!lazyload()) {
return null; return null;
} }
@@ -846,7 +870,7 @@ public class ChangeData {
public Integer totalCommentCount() { public Integer totalCommentCount() {
if (totalCommentCount == null) { if (totalCommentCount == null) {
if (!lazyLoad) { if (!lazyload()) {
return null; return null;
} }
@@ -863,7 +887,7 @@ public class ChangeData {
public List<ChangeMessage> messages() { public List<ChangeMessage> messages() {
if (messages == null) { if (messages == null) {
if (!lazyLoad) { if (!lazyload()) {
return Collections.emptyList(); return Collections.emptyList();
} }
messages = cmUtil.byChange(notes()); messages = cmUtil.byChange(notes());
@@ -878,7 +902,12 @@ public class ChangeData {
// evaluation. // evaluation.
List<SubmitRecord> records = submitRecords.get(options); List<SubmitRecord> records = submitRecords.get(options);
if (records == null) { if (records == null) {
if (!lazyLoad) { if (storageConstraint != StorageConstraint.NOTEDB_ONLY) {
// Submit requirements are expensive. We allow loading them only if this change did not
// originate from the change index and we can invest the extra time.
logger.atWarning().log(
"Tried to load SubmitRecords for change fetched from index %s: %d",
project(), getId().get());
return Collections.emptyList(); return Collections.emptyList();
} }
records = submitRuleEvaluatorFactory.create(options).evaluate(this); records = submitRuleEvaluatorFactory.create(options).evaluate(this);
@@ -926,7 +955,7 @@ public class ChangeData {
} else if (c.isWorkInProgress()) { } else if (c.isWorkInProgress()) {
return null; return null;
} else { } else {
if (!lazyLoad) { if (!lazyload()) {
return null; return null;
} }
PatchSet ps = currentPatchSet(); PatchSet ps = currentPatchSet();
@@ -972,7 +1001,7 @@ public class ChangeData {
public Map<Account.Id, Ref> editRefs() { public Map<Account.Id, Ref> editRefs() {
if (editsByUser == null) { if (editsByUser == null) {
if (!lazyLoad) { if (!lazyload()) {
return Collections.emptyMap(); return Collections.emptyMap();
} }
Change c = change(); Change c = change();
@@ -1004,7 +1033,7 @@ public class ChangeData {
public Map<Account.Id, Ref> draftRefs() { public Map<Account.Id, Ref> draftRefs() {
if (draftsByUser == null) { if (draftsByUser == null) {
if (!lazyLoad) { if (!lazyload()) {
return Collections.emptyMap(); return Collections.emptyMap();
} }
Change c = change(); Change c = change();
@@ -1049,7 +1078,7 @@ public class ChangeData {
public Set<Account.Id> reviewedBy() { public Set<Account.Id> reviewedBy() {
if (reviewedBy == null) { if (reviewedBy == null) {
if (!lazyLoad) { if (!lazyload()) {
return Collections.emptySet(); return Collections.emptySet();
} }
Change c = change(); Change c = change();
@@ -1081,7 +1110,7 @@ public class ChangeData {
public Set<String> hashtags() { public Set<String> hashtags() {
if (hashtags == null) { if (hashtags == null) {
if (!lazyLoad) { if (!lazyload()) {
return Collections.emptySet(); return Collections.emptySet();
} }
hashtags = notes().getHashtags(); hashtags = notes().getHashtags();
@@ -1095,7 +1124,7 @@ public class ChangeData {
public ImmutableListMultimap<Account.Id, String> stars() { public ImmutableListMultimap<Account.Id, String> stars() {
if (stars == null) { if (stars == null) {
if (!lazyLoad) { if (!lazyload()) {
return ImmutableListMultimap.of(); return ImmutableListMultimap.of();
} }
ImmutableListMultimap.Builder<Account.Id, String> b = ImmutableListMultimap.builder(); ImmutableListMultimap.Builder<Account.Id, String> b = ImmutableListMultimap.builder();
@@ -1113,7 +1142,7 @@ public class ChangeData {
public ImmutableMap<Account.Id, StarRef> starRefs() { public ImmutableMap<Account.Id, StarRef> starRefs() {
if (starRefs == null) { if (starRefs == null) {
if (!lazyLoad) { if (!lazyload()) {
return ImmutableMap.of(); return ImmutableMap.of();
} }
starRefs = requireNonNull(starredChangesUtil).byChange(legacyId); starRefs = requireNonNull(starredChangesUtil).byChange(legacyId);
@@ -1131,7 +1160,7 @@ public class ChangeData {
if (stars != null) { if (stars != null) {
starsOf = StarsOf.create(accountId, stars.get(accountId)); starsOf = StarsOf.create(accountId, stars.get(accountId));
} else { } else {
if (!lazyLoad) { if (!lazyload()) {
return ImmutableSet.of(); return ImmutableSet.of();
} }
starsOf = StarsOf.create(accountId, starredChangesUtil.getLabels(accountId, legacyId)); starsOf = StarsOf.create(accountId, starredChangesUtil.getLabels(accountId, legacyId));
@@ -1179,7 +1208,7 @@ public class ChangeData {
public SetMultimap<NameKey, RefState> getRefStates() { public SetMultimap<NameKey, RefState> getRefStates() {
if (refStates == null) { if (refStates == null) {
if (!lazyLoad) { if (!lazyload()) {
return ImmutableSetMultimap.of(); return ImmutableSetMultimap.of();
} }