Remove all unneeded checks of primary storage of a change

There is a remaining usage in ChangeUpdate which requires more work. It
already has a TODO.

Change-Id: I259f9740d1ca8c8af531b8ac61d392af98261971
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2018-12-13 11:34:30 +01:00
committed by Dave Borowitz
parent 758600436c
commit 6d82df9bbb
11 changed files with 21 additions and 234 deletions

View File

@@ -30,7 +30,6 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.change.ChangeKindCache;
import com.google.gerrit.server.change.LabelNormalizer;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
@@ -74,50 +73,6 @@ public class ApprovalCopier {
this.psUtil = psUtil;
}
/**
* Apply approval copy settings from prior PatchSets to a new PatchSet.
*
* @param db review database.
* @param notes change notes for user uploading PatchSet
* @param ps new PatchSet
* @param rw open walk that can read the patch set commit; null to open the repo on demand.
* @param repoConfig repo config used for change kind detection; null to read from repo on demand.
* @throws OrmException
*/
public void copyInReviewDb(
ReviewDb db,
ChangeNotes notes,
PatchSet ps,
@Nullable RevWalk rw,
@Nullable Config repoConfig)
throws OrmException {
copyInReviewDb(db, notes, ps, rw, repoConfig, Collections.emptyList());
}
/**
* Apply approval copy settings from prior PatchSets to a new PatchSet.
*
* @param db review database.
* @param notes change notes for user uploading PatchSet
* @param ps new PatchSet
* @param rw open walk that can read the patch set commit; null to open the repo on demand.
* @param repoConfig repo config used for change kind detection; null to read from repo on demand.
* @param dontCopy PatchSetApprovals indicating which (account, label) pairs should not be copied
* @throws OrmException
*/
public void copyInReviewDb(
ReviewDb db,
ChangeNotes notes,
PatchSet ps,
@Nullable RevWalk rw,
@Nullable Config repoConfig,
Iterable<PatchSetApproval> dontCopy)
throws OrmException {
if (PrimaryStorage.of(notes.getChange()) == PrimaryStorage.REVIEW_DB) {
db.patchSetApprovals().insert(getForPatchSet(db, notes, ps, rw, repoConfig, dontCopy));
}
}
Iterable<PatchSetApproval> getForPatchSet(
ReviewDb db,
ChangeNotes notes,

View File

@@ -31,8 +31,6 @@ import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalCopier;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.ChangeUtil;
@@ -79,7 +77,6 @@ public class PatchSetInserter implements BatchUpdateOp {
private final ProjectCache projectCache;
private final RevisionCreated revisionCreated;
private final ApprovalsUtil approvalsUtil;
private final ApprovalCopier approvalCopier;
private final ChangeMessagesUtil cmUtil;
private final PatchSetUtil psUtil;
@@ -101,7 +98,6 @@ public class PatchSetInserter implements BatchUpdateOp {
private NotifyHandling notify = NotifyHandling.ALL;
private ListMultimap<RecipientType, Account.Id> accountsToNotify = ImmutableListMultimap.of();
private boolean allowClosed;
private boolean copyApprovals = true;
// Fields set during some phase of BatchUpdate.Op.
private Change change;
@@ -114,7 +110,6 @@ public class PatchSetInserter implements BatchUpdateOp {
public PatchSetInserter(
PermissionBackend permissionBackend,
ApprovalsUtil approvalsUtil,
ApprovalCopier approvalCopier,
ChangeMessagesUtil cmUtil,
PatchSetInfoFactory patchSetInfoFactory,
CommitValidators.Factory commitValidatorsFactory,
@@ -127,7 +122,6 @@ public class PatchSetInserter implements BatchUpdateOp {
@Assisted ObjectId commitId) {
this.permissionBackend = permissionBackend;
this.approvalsUtil = approvalsUtil;
this.approvalCopier = approvalCopier;
this.cmUtil = cmUtil;
this.patchSetInfoFactory = patchSetInfoFactory;
this.commitValidatorsFactory = commitValidatorsFactory;
@@ -192,11 +186,6 @@ public class PatchSetInserter implements BatchUpdateOp {
return this;
}
public PatchSetInserter setCopyApprovals(boolean copyApprovals) {
this.copyApprovals = copyApprovals;
return this;
}
public Change getChange() {
checkState(change != null, "getChange() only valid after executing update");
return change;
@@ -218,8 +207,6 @@ public class PatchSetInserter implements BatchUpdateOp {
@Override
public boolean updateChange(ChangeContext ctx)
throws ResourceConflictException, OrmException, IOException {
ReviewDb db = ctx.getDb();
change = ctx.getChange();
ChangeUpdate update = ctx.getUpdate(psId);
update.setSubjectForCommit("Create patch set " + psId.get());
@@ -263,10 +250,6 @@ public class PatchSetInserter implements BatchUpdateOp {
change.setStatus(Change.Status.NEW);
}
change.setCurrentPatchSet(patchSetInfo);
if (copyApprovals) {
approvalCopier.copyInReviewDb(
db, ctx.getNotes(), patchSet, ctx.getRevWalk(), ctx.getRepoView().getConfig());
}
if (changeMessage != null) {
cmUtil.addChangeMessage(update, changeMessage);
}

View File

@@ -71,7 +71,6 @@ public class RebaseChangeOp implements BatchUpdateOp {
private boolean validate = true;
private boolean checkAddPatchSetPermission = true;
private boolean forceContentMerge;
private boolean copyApprovals = true;
private boolean detailedCommitMessage;
private boolean postMessage = true;
private boolean matchAuthorToCommitterDate = false;
@@ -128,11 +127,6 @@ public class RebaseChangeOp implements BatchUpdateOp {
return this;
}
public RebaseChangeOp setCopyApprovals(boolean copyApprovals) {
this.copyApprovals = copyApprovals;
return this;
}
public RebaseChangeOp setDetailedCommitMessage(boolean detailedCommitMessage) {
this.detailedCommitMessage = detailedCommitMessage;
return this;
@@ -189,7 +183,6 @@ public class RebaseChangeOp implements BatchUpdateOp {
.setDescription("Rebase")
.setNotify(NotifyHandling.NONE)
.setFireRevisionCreated(fireRevisionCreated)
.setCopyApprovals(copyApprovals)
.setCheckAddPatchSetPermission(checkAddPatchSetPermission)
.setValidate(validate);
if (postMessage) {

View File

@@ -43,7 +43,6 @@ import com.google.gerrit.reviewdb.client.Comment;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gerrit.server.ApprovalCopier;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.CommentsUtil;
@@ -116,7 +115,6 @@ public class ReplaceOp implements BatchUpdateOp {
private static final String CHANGE_IS_CLOSED = "change is closed";
private final AccountResolver accountResolver;
private final ApprovalCopier approvalCopier;
private final ApprovalsUtil approvalsUtil;
private final ChangeData.Factory changeDataFactory;
private final ChangeKindCache changeKindCache;
@@ -162,7 +160,6 @@ public class ReplaceOp implements BatchUpdateOp {
@Inject
ReplaceOp(
AccountResolver accountResolver,
ApprovalCopier approvalCopier,
ApprovalsUtil approvalsUtil,
ChangeData.Factory changeDataFactory,
ChangeKindCache changeKindCache,
@@ -190,7 +187,6 @@ public class ReplaceOp implements BatchUpdateOp {
@Assisted @Nullable MagicBranchInput magicBranch,
@Assisted @Nullable PushCertificate pushCertificate) {
this.accountResolver = accountResolver;
this.approvalCopier = approvalCopier;
this.approvalsUtil = approvalsUtil;
this.changeDataFactory = changeDataFactory;
this.changeKindCache = changeKindCache;
@@ -315,21 +311,8 @@ public class ReplaceOp implements BatchUpdateOp {
update.setPsDescription(psDescription);
MailRecipients fromFooters = getRecipientsFromFooters(accountResolver, commit.getFooterLines());
Iterable<PatchSetApproval> newApprovals =
approvalsUtil.addApprovalsForNewPatchSet(
ctx.getDb(),
update,
projectState.getLabelTypes(),
newPatchSet,
ctx.getUser(),
approvals);
approvalCopier.copyInReviewDb(
ctx.getDb(),
ctx.getNotes(),
newPatchSet,
ctx.getRevWalk(),
ctx.getRepoView().getConfig(),
newApprovals);
ctx.getDb(), update, projectState.getLabelTypes(), newPatchSet, ctx.getUser(), approvals);
reviewerAdditions =
reviewerAdder.prepare(

View File

@@ -63,7 +63,6 @@ import com.google.gerrit.server.StarredChangesUtil;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.index.change.StalenessChecker.RefStatePattern;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.notedb.RobotCommentNotes;
import com.google.gerrit.server.project.SubmitRuleOptions;
@@ -805,7 +804,6 @@ public class ChangeField {
.values()
.forEach(r -> result.add(RefState.of(r.ref()).toByteArray(allUsers(cd))));
if (PrimaryStorage.of(cd.change()) == PrimaryStorage.NOTE_DB) {
ChangeNotes notes = cd.notes();
result.add(
RefState.create(notes.getRefName(), notes.getMetaId()).toByteArray(project));
@@ -817,7 +815,6 @@ public class ChangeField {
cd.draftRefs()
.values()
.forEach(r -> result.add(RefState.of(r).toByteArray(allUsers(cd))));
}
return result;
});
@@ -842,11 +839,9 @@ public class ChangeField {
result.add(
RefStatePattern.create(RefNames.refsStarredChangesPrefix(id) + "*")
.toByteArray(allUsers(cd)));
if (PrimaryStorage.of(cd.change()) == PrimaryStorage.NOTE_DB) {
result.add(
RefStatePattern.create(RefNames.refsDraftCommentsPrefix(id) + "*")
.toByteArray(allUsers(cd)));
}
return result;
});

View File

@@ -34,7 +34,6 @@ import com.google.gerrit.server.index.IndexUtils;
import com.google.gerrit.server.logging.TraceContext;
import com.google.gerrit.server.logging.TraceContext.TraceTimer;
import com.google.gerrit.server.plugincontext.PluginSetContext;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.util.RequestContext;
import com.google.gerrit.server.util.ThreadLocalRequestContext;
@@ -419,8 +418,6 @@ public class ChangeIndexer {
indexImpl(changeDataFactory.create(db.get(), project, id));
return true;
}
} catch (NoSuchChangeException nsce) {
logger.atFine().log("Change %s was deleted, aborting reindexing the change.", id.get());
} catch (Exception e) {
if (!isCausedByRepositoryNotFoundException(e)) {
throw e;

View File

@@ -16,7 +16,6 @@ package com.google.gerrit.server.index.change;
import static com.google.common.base.Preconditions.checkArgument;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.Objects.requireNonNull;
import static java.util.stream.Collectors.joining;
import com.google.auto.value.AutoValue;
@@ -29,21 +28,15 @@ import com.google.common.collect.SetMultimap;
import com.google.common.collect.Sets;
import com.google.common.collect.Streams;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.index.RefState;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.UsedAt;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.List;
@@ -66,21 +59,16 @@ public class StalenessChecker {
private final ChangeIndexCollection indexes;
private final GitRepositoryManager repoManager;
private final IndexConfig indexConfig;
private final Provider<ReviewDb> db;
@Inject
StalenessChecker(
ChangeIndexCollection indexes,
GitRepositoryManager repoManager,
IndexConfig indexConfig,
Provider<ReviewDb> db) {
ChangeIndexCollection indexes, GitRepositoryManager repoManager, IndexConfig indexConfig) {
this.indexes = indexes;
this.repoManager = repoManager;
this.indexConfig = indexConfig;
this.db = db;
}
public boolean isStale(Change.Id id) throws IOException, OrmException {
public boolean isStale(Change.Id id) throws IOException {
ChangeIndex i = indexes.getSearchIndex();
if (i == null) {
return false; // No index; caller couldn't do anything if it is stale.
@@ -96,25 +84,16 @@ public class StalenessChecker {
return true; // Not in index, but caller wants it to be.
}
ChangeData cd = result.get();
return isStale(
repoManager,
id,
cd.change(),
ReviewDbUtil.unwrapDb(db.get()).changes().get(id),
parseStates(cd),
parsePatterns(cd));
return isStale(repoManager, id, parseStates(cd), parsePatterns(cd));
}
@UsedAt(UsedAt.Project.GOOGLE)
public static boolean isStale(
GitRepositoryManager repoManager,
Change.Id id,
Change indexChange,
@Nullable Change reviewDbChange,
SetMultimap<Project.NameKey, RefState> states,
ListMultimap<Project.NameKey, RefStatePattern> patterns) {
return reviewDbChangeIsStale(indexChange, reviewDbChange)
|| refsAreStale(repoManager, id, states, patterns);
return refsAreStale(repoManager, id, states, patterns);
}
@VisibleForTesting
@@ -134,31 +113,6 @@ public class StalenessChecker {
return false;
}
@VisibleForTesting
static boolean reviewDbChangeIsStale(Change indexChange, @Nullable Change reviewDbChange) {
requireNonNull(indexChange);
PrimaryStorage storageFromIndex = PrimaryStorage.of(indexChange);
PrimaryStorage storageFromReviewDb = PrimaryStorage.of(reviewDbChange);
if (reviewDbChange == null) {
if (storageFromIndex == PrimaryStorage.REVIEW_DB) {
return true; // Index says it should have been in ReviewDb, but it wasn't.
}
return false; // Not in ReviewDb, but that's ok.
}
checkArgument(
indexChange.getId().equals(reviewDbChange.getId()),
"mismatched change ID: %s != %s",
indexChange.getId(),
reviewDbChange.getId());
if (storageFromIndex != storageFromReviewDb) {
return true; // Primary storage differs, definitely stale.
}
if (storageFromReviewDb != PrimaryStorage.REVIEW_DB) {
return false; // Not a ReviewDb change, don't check rowVersion.
}
return reviewDbChange.getRowVersion() != indexChange.getRowVersion();
}
private SetMultimap<Project.NameKey, RefState> parseStates(ChangeData cd) {
return RefState.parseStates(cd.getRefStates());
}

View File

@@ -79,10 +79,7 @@ public class NoteDbChangeState {
}
public static PrimaryStorage of(@Nullable Change c) {
return of(NoteDbChangeState.parse(c));
}
public static PrimaryStorage of(@Nullable NoteDbChangeState s) {
NoteDbChangeState s = NoteDbChangeState.parse(c);
return s != null ? s.getPrimaryStorage() : REVIEW_DB;
}
}
@@ -270,39 +267,6 @@ public class NoteDbChangeState {
return state;
}
// TODO(dborowitz): Ugly. Refactor these static methods into a Checker class
// or something. They do not belong in NoteDbChangeState itself because:
// - need to inject Config but don't want a whole Factory
// - can't be methods on NoteDbChangeState because state is nullable (though
// we could also solve this by inventing an empty-but-non-null state)
// Also we should clean up duplicated code between static/non-static methods.
public static boolean isChangeUpToDate(
@Nullable NoteDbChangeState state, RefCache changeRepoRefs, Change.Id changeId)
throws IOException {
if (PrimaryStorage.of(state) == NOTE_DB) {
return true; // Primary storage is NoteDb, up to date by definition.
}
if (state == null) {
return !changeRepoRefs.get(changeMetaRef(changeId)).isPresent();
}
return state.isChangeUpToDate(changeRepoRefs);
}
public static boolean areDraftsUpToDate(
@Nullable NoteDbChangeState state,
RefCache draftsRepoRefs,
Change.Id changeId,
Account.Id accountId)
throws IOException {
if (PrimaryStorage.of(state) == NOTE_DB) {
return true; // Primary storage is NoteDb, up to date by definition.
}
if (state == null) {
return !draftsRepoRefs.get(refsDraftComments(changeId, accountId)).isPresent();
}
return state.areDraftsUpToDate(draftsRepoRefs, accountId);
}
public static long getReadOnlySkew(Config cfg) {
return cfg.getTimeUnit("notedb", null, "maxTimestampSkew", 1000, TimeUnit.MILLISECONDS);
}

View File

@@ -177,7 +177,6 @@ public class RebaseSubmitStrategy extends SubmitStrategy {
.setFireRevisionCreated(false)
// Bypass approval copier since SubmitStrategyOp copy all approvals
// later anyway.
.setCopyApprovals(false)
.setValidate(false)
.setCheckAddPatchSetPermission(false)
// RebaseAlways should set always modify commit message like

View File

@@ -18,7 +18,6 @@ import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.extensions.common.ProblemInfo.Status.FIXED;
import static com.google.gerrit.extensions.common.ProblemInfo.Status.FIX_FAILED;
import static com.google.gerrit.testing.TestChanges.newPatchSet;
import static java.util.Collections.singleton;
import static java.util.Objects.requireNonNull;
import com.google.gerrit.acceptance.AbstractDaemonTest;
@@ -43,7 +42,6 @@ import com.google.gerrit.server.change.ConsistencyChecker;
import com.google.gerrit.server.change.PatchSetInserter;
import com.google.gerrit.server.notedb.ChangeNoteUtil;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -804,11 +802,6 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
c.setCurrentPatchSet(psId, subject, c.getOriginalSubject());
PatchSet ps = newPatchSet(psId, rev, adminId);
if (PrimaryStorage.of(c) == PrimaryStorage.REVIEW_DB) {
db.patchSets().insert(singleton(ps));
db.changes().update(singleton(c));
}
addNoteDbCommit(
c.getId(),
"Update patch set "

View File

@@ -17,7 +17,6 @@ package com.google.gerrit.server.index.change;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assert_;
import static com.google.gerrit.server.index.change.StalenessChecker.refsAreStale;
import static com.google.gerrit.testing.TestChanges.newChange;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.stream.Collectors.toList;
@@ -25,16 +24,12 @@ import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableSetMultimap;
import com.google.common.collect.ListMultimap;
import com.google.gerrit.index.RefState;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.index.change.StalenessChecker.RefStatePattern;
import com.google.gerrit.server.notedb.NoteDbChangeState;
import com.google.gerrit.testing.GerritBaseTests;
import com.google.gerrit.testing.InMemoryRepositoryManager;
import com.google.gwtorm.protobuf.CodecFactory;
import com.google.gwtorm.protobuf.ProtobufCodec;
import java.util.stream.Stream;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.ObjectId;
@@ -51,8 +46,6 @@ public class StalenessCheckerTest extends GerritBaseTests {
private static final Change.Id C = new Change.Id(1234);
private static final ProtobufCodec<Change> CHANGE_CODEC = CodecFactory.encoder(Change.class);
private GitRepositoryManager repoManager;
private Repository r1;
private Repository r2;
@@ -316,29 +309,7 @@ public class StalenessCheckerTest extends GerritBaseTests {
.isFalse();
}
@Test
public void reviewDbChangeIsStale() throws Exception {
Change indexChange = newChange(P1, new Account.Id(1));
indexChange.setNoteDbState(SHA1);
// Change is missing from ReviewDb but present in index.
assertThat(StalenessChecker.reviewDbChangeIsStale(indexChange, null)).isTrue();
// Change differs only in primary storage.
Change noteDbPrimary = clone(indexChange);
noteDbPrimary.setNoteDbState(NoteDbChangeState.NOTE_DB_PRIMARY_STATE);
assertThat(StalenessChecker.reviewDbChangeIsStale(indexChange, noteDbPrimary)).isTrue();
assertThat(StalenessChecker.reviewDbChangeIsStale(indexChange, clone(indexChange))).isFalse();
// Can't easily change row version to check true case.
}
private static Iterable<byte[]> byteArrays(String... strs) {
return Stream.of(strs).map(s -> s != null ? s.getBytes(UTF_8) : null).collect(toList());
}
private static Change clone(Change change) {
return CHANGE_CODEC.decode(CHANGE_CODEC.encodeToByteArray(change));
}
}