opendev/gerrit
Code Issues Proposed changes

Merge "ChangeData: Disallow computing submit rules when serving query results"

Browse Source
This commit is contained in:
Edwin Kempin
2021-03-19 13:22:15 +00:00
committed by Gerrit Code Review
parent eebf51b797 c774890f15
commit ca87b681a0
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();
} }