Merge "Add maxRefsInBatch and maxRefsToUpdate options"
diff --git a/java/com/google/gerrit/server/notedb/CommitRewriter.java b/java/com/google/gerrit/server/notedb/CommitRewriter.java
index e940b1e..eabee65 100644
--- a/java/com/google/gerrit/server/notedb/CommitRewriter.java
+++ b/java/com/google/gerrit/server/notedb/CommitRewriter.java
@@ -30,6 +30,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.UsedAt;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
@@ -117,6 +118,16 @@
public boolean verifyCommits = true;
/** Whether to compute and output the diff of the commit history for the backfilled refs. */
public boolean outputDiff = true;
+
+ /** Max number of refs to update in a single {@link BatchRefUpdate}. */
+ public int maxRefsInBatch = 10000;
+ /**
+ * Max number of refs to fix by a single {@link RefsUpdate#backfillProject} run. Since second
+ * run on the same set of refs is a no-op, running with this option in a loop will eventually
+ * fix all refs. Number of executed {@link BatchRefUpdate} depends on {@link #maxRefsInBatch}
+ * option.
+ */
+ public int maxRefsToUpdate = 50000;
}
/** Result of the backfill run for a project. */
@@ -239,15 +250,24 @@
*/
public BackfillResult backfillProject(
Project.NameKey project, Repository repo, RunOptions options) {
+
+ checkState(
+ options.maxRefsInBatch > 0 && options.maxRefsToUpdate > 0,
+ "Expected maxRefsInBatch>0 && <= maxRefsToUpdate>0");
+ checkState(
+ options.maxRefsInBatch <= options.maxRefsToUpdate,
+ "Expected maxRefsInBatch(%s) <= maxRefsToUpdate(%s)",
+ options.maxRefsInBatch,
+ options.maxRefsToUpdate);
BackfillResult result = new BackfillResult();
result.ok = true;
- try (RevWalk revWalk = new RevWalk(repo);
- ObjectInserter ins = newPackInserter(repo)) {
- BatchRefUpdate bru = repo.getRefDatabase().newBatchUpdate();
- bru.setForceRefLog(true);
- bru.setRefLogMessage(CommitRewriter.class.getName(), false);
- bru.setAllowNonFastForwards(true);
+ int refsInUpdate = 0;
+ RefsUpdate refsUpdate = null;
+ try {
for (Ref ref : repo.getRefDatabase().getRefsByPrefix(RefNames.REFS_CHANGES)) {
+ if (result.fixedRefDiff.size() >= options.maxRefsToUpdate) {
+ return result;
+ }
Change.Id changeId = Change.Id.fromRef(ref.getName());
if (changeId == null || !ref.getName().equals(RefNames.changeMetaRef(changeId))) {
continue;
@@ -262,14 +282,26 @@
logger.atWarning().withCause(e).log("Failed to run verification on ref %s", ref);
}
}
+ if (refsUpdate == null) {
+ refsUpdate = RefsUpdate.create(repo);
+ }
ChangeFixProgress changeFixProgress =
- backfillChange(revWalk, ins, ref, accountsInChange, options);
+ backfillChange(refsUpdate, ref, accountsInChange, options);
if (changeFixProgress.anyFixesApplied) {
- bru.addCommand(
- new ReceiveCommand(ref.getObjectId(), changeFixProgress.newTipId, ref.getName()));
+ refsInUpdate++;
+ refsUpdate
+ .batchRefUpdate()
+ .addCommand(
+ new ReceiveCommand(
+ ref.getObjectId(), changeFixProgress.newTipId, ref.getName()));
result.fixedRefDiff.put(ref.getName(), changeFixProgress.commitDiffs);
}
-
+ if (refsInUpdate >= options.maxRefsInBatch
+ || result.fixedRefDiff.size() >= options.maxRefsToUpdate) {
+ processUpdate(options, refsUpdate);
+ refsUpdate = null;
+ refsInUpdate = 0;
+ }
if (!changeFixProgress.isValidAfterFix) {
result.refsStillInvalidAfterFix.add(ref.getName());
}
@@ -278,21 +310,34 @@
result.refsFailedToFix.add(ref.getName());
}
}
-
- if (!bru.getCommands().isEmpty()) {
- if (!options.dryRun) {
- ins.flush();
- RefUpdateUtil.executeChecked(bru, revWalk);
- }
- }
+ processUpdate(options, refsUpdate);
} catch (IOException e) {
- logger.atWarning().withCause(e).log("Failed to fix project %s", project.get());
+ logger.atWarning().log("Failed to fix project %s. Reason: %s", project.get(), e.getMessage());
result.ok = false;
+ } finally {
+ if (refsUpdate != null) {
+ refsUpdate.close();
+ }
}
return result;
}
+ /** Executes a single {@link RefsUpdate#batchRefUpdate}. */
+ private void processUpdate(RunOptions options, @Nullable RefsUpdate refsUpdate)
+ throws IOException {
+ if (refsUpdate == null) {
+ return;
+ }
+ if (!refsUpdate.batchRefUpdate().getCommands().isEmpty()) {
+ if (!options.dryRun) {
+ refsUpdate.inserter().flush();
+ RefUpdateUtil.executeChecked(refsUpdate.batchRefUpdate(), refsUpdate.revWalk());
+ }
+ }
+ refsUpdate.close();
+ }
+
/**
* Retrieves accounts, that are associated with a change (e.g. reviewers, commenters, etc.). These
* accounts are used to verify that commits do not contain user data. See {@link #verifyCommit}
@@ -376,8 +421,7 @@
* ChangeFixProgress#newTipId}.
*/
public ChangeFixProgress backfillChange(
- RevWalk revWalk,
- ObjectInserter inserter,
+ RefsUpdate refsUpdate,
Ref ref,
ImmutableSet<AccountState> accountsInChange,
RunOptions options)
@@ -385,17 +429,17 @@
ObjectId oldTip = ref.getObjectId();
// Walk from the first commit of the branch.
- revWalk.reset();
- revWalk.markStart(revWalk.parseCommit(oldTip));
- revWalk.sort(RevSort.TOPO);
+ refsUpdate.revWalk().reset();
+ refsUpdate.revWalk().markStart(refsUpdate.revWalk().parseCommit(oldTip));
+ refsUpdate.revWalk().sort(RevSort.TOPO);
- revWalk.sort(RevSort.REVERSE);
+ refsUpdate.revWalk().sort(RevSort.REVERSE);
RevCommit originalCommit;
boolean rewriteStarted = false;
ChangeFixProgress changeFixProgress = new ChangeFixProgress(ref.getName());
- while ((originalCommit = revWalk.next()) != null) {
+ while ((originalCommit = refsUpdate.revWalk().next()) != null) {
changeFixProgress.updateAuthorId =
parseIdent(changeFixProgress, originalCommit.getAuthorIdent());
@@ -453,7 +497,8 @@
cb.setEncoding(originalCommit.getEncoding());
byte[] newCommitContent = cb.build();
checkCommitModification(originalCommit, newCommitContent);
- changeFixProgress.newTipId = inserter.insert(Constants.OBJ_COMMIT, newCommitContent);
+ changeFixProgress.newTipId =
+ refsUpdate.inserter().insert(Constants.OBJ_COMMIT, newCommitContent);
// Only compute diff if the content of the commit was actually changed.
if (options.outputDiff && needsFix) {
String diff = computeDiff(originalCommit.getRawBuffer(), newCommitContent);
@@ -1283,4 +1328,33 @@
abstract Optional<String> email();
}
+
+ /**
+ * Objects, needed to fix Refs in a single {@link BatchRefUpdate}. Number of changes in a batch
+ * are limited by {@link RunOptions#maxRefsInBatch}.
+ */
+ @AutoValue
+ abstract static class RefsUpdate implements AutoCloseable {
+ static RefsUpdate create(Repository repo) {
+ RevWalk revWalk = new RevWalk(repo);
+ ObjectInserter inserter = newPackInserter(repo);
+ BatchRefUpdate bru = repo.getRefDatabase().newBatchUpdate();
+ bru.setForceRefLog(true);
+ bru.setRefLogMessage(CommitRewriter.class.getName(), false);
+ bru.setAllowNonFastForwards(true);
+ return new AutoValue_CommitRewriter_RefsUpdate(bru, revWalk, inserter);
+ }
+
+ @Override
+ public void close() {
+ inserter().close();
+ revWalk().close();
+ }
+
+ abstract BatchRefUpdate batchRefUpdate();
+
+ abstract RevWalk revWalk();
+
+ abstract ObjectInserter inserter();
+ }
}
diff --git a/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java b/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
index 056c7dc..98721fd 100644
--- a/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
+++ b/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
@@ -24,6 +24,7 @@
import static java.util.Objects.requireNonNull;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AttentionSetUpdate;
import com.google.gerrit.entities.AttentionSetUpdate.Operation;
@@ -33,6 +34,7 @@
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.entities.SubmitRecord;
+import com.google.gerrit.git.RefUpdateUtil;
import com.google.gerrit.json.OutputFormat;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
@@ -48,7 +50,9 @@
import java.sql.Timestamp;
import java.util.Arrays;
import java.util.List;
+import java.util.Map;
import java.util.stream.IntStream;
+import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
@@ -56,6 +60,8 @@
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevSort;
import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.transport.ReceiveCommand;
+import org.junit.After;
import org.junit.Before;
import org.junit.Test;
@@ -70,6 +76,21 @@
@Before
public void setUp() throws Exception {}
+ @After
+ public void cleanUp() throws Exception {
+ BatchRefUpdate bru = repo.getRefDatabase().newBatchUpdate();
+ bru.setAllowNonFastForwards(true);
+ for (Ref ref : repo.getRefDatabase().getRefsByPrefix(RefNames.REFS_CHANGES)) {
+ Change.Id changeId = Change.Id.fromRef(ref.getName());
+ if (changeId == null || !ref.getName().equals(RefNames.changeMetaRef(changeId))) {
+ continue;
+ }
+ bru.addCommand(new ReceiveCommand(ref.getObjectId(), ObjectId.zeroId(), ref.getName()));
+ }
+
+ RefUpdateUtil.executeChecked(bru, repo);
+ }
+
@Test
public void validHistoryNoOp() throws Exception {
String tag = "jenkins";
@@ -157,6 +178,138 @@
}
@Test
+ public void numRefs_greater_maxRefsToUpdate_allFixed() throws Exception {
+ int numberOfChanges = 12;
+ ImmutableMap.Builder<String, Ref> refsToOldMetaBuilder = new ImmutableMap.Builder<>();
+ for (int i = 0; i < numberOfChanges; i++) {
+ Change c = newChange();
+ ChangeUpdate update = newUpdate(c, changeOwner);
+ update.setChangeMessage("Change has been successfully merged by " + changeOwner.getName());
+ update.commit();
+ ChangeUpdate updateWithSubject = newUpdate(c, changeOwner);
+ updateWithSubject.setSubjectForCommit("Update with subject");
+ updateWithSubject.commit();
+ String refName = RefNames.changeMetaRef(c.getId());
+ Ref metaRefBeforeRewrite = repo.exactRef(refName);
+ refsToOldMetaBuilder.put(refName, metaRefBeforeRewrite);
+ }
+ ImmutableMap<String, Ref> refsToOldMeta = refsToOldMetaBuilder.build();
+
+ RunOptions options = new RunOptions();
+ options.dryRun = false;
+ options.outputDiff = false;
+ options.verifyCommits = false;
+ options.maxRefsInBatch = 10;
+ options.maxRefsToUpdate = 12;
+ BackfillResult backfillResult = rewriter.backfillProject(project, repo, options);
+ assertThat(backfillResult.fixedRefDiff.keySet()).isEqualTo(refsToOldMeta.keySet());
+ for (Map.Entry<String, Ref> refEntry : refsToOldMeta.entrySet()) {
+ Ref metaRefAfterRewrite = repo.exactRef(refEntry.getKey());
+ assertThat(refEntry.getValue()).isNotEqualTo(metaRefAfterRewrite);
+ }
+ }
+
+ @Test
+ public void maxRefsToUpdate_coversAllInvalid_inMultipleBatches() throws Exception {
+ testMaxRefsToUpdate(
+ /*numberOfInvalidChanges=*/ 11,
+ /*numberOfValidChanges=*/ 9,
+ /*maxRefsToUpdate=*/ 12,
+ /*maxRefsInBatch=*/ 2);
+ }
+
+ @Test
+ public void maxRefsToUpdate_coversAllInvalid_inSingleBatch() throws Exception {
+ testMaxRefsToUpdate(
+ /*numberOfInvalidChanges=*/ 11,
+ /*numberOfValidChanges=*/ 9,
+ /*maxRefsToUpdate=*/ 12,
+ /*maxRefsInBatch=*/ 12);
+ }
+
+ @Test
+ public void moreInvalidRefs_thenMaxRefsToUpdate_inMultipleBatches() throws Exception {
+ testMaxRefsToUpdate(
+ /*numberOfInvalidChanges=*/ 11,
+ /*numberOfValidChanges=*/ 9,
+ /*maxRefsToUpdate=*/ 10,
+ /*maxRefsInBatch=*/ 2);
+ }
+
+ @Test
+ public void moreInvalidRefs_thenMaxRefsToUpdate_inSingleBatch() throws Exception {
+ testMaxRefsToUpdate(
+ /*numberOfInvalidChanges=*/ 11,
+ /*numberOfValidChanges=*/ 9,
+ /*maxRefsToUpdate=*/ 10,
+ /*maxRefsInBatch=*/ 10);
+ }
+
+ private void testMaxRefsToUpdate(
+ int numberOfInvalidChanges, int numberOfValidChanges, int maxRefsToUpdate, int maxRefsInBatch)
+ throws Exception {
+ ImmutableMap.Builder<String, ObjectId> expectedFixedRefsToOldMetaBuilder =
+ new ImmutableMap.Builder<>();
+ ImmutableMap.Builder<String, ObjectId> expectedSkippedRefsToOldMetaBuilder =
+ new ImmutableMap.Builder<>();
+ for (int i = 0; i < numberOfValidChanges; i++) {
+ Change c = newChange();
+ ChangeUpdate updateWithSubject = newUpdate(c, changeOwner);
+ updateWithSubject.setSubjectForCommit("Update with subject");
+ updateWithSubject.commit();
+ String refName = RefNames.changeMetaRef(c.getId());
+ Ref metaRefBeforeRewrite = repo.exactRef(refName);
+ expectedSkippedRefsToOldMetaBuilder.put(refName, metaRefBeforeRewrite.getObjectId());
+ }
+ for (int i = 0; i < numberOfInvalidChanges; i++) {
+ Change c = newChange();
+ ChangeUpdate update = newUpdate(c, changeOwner);
+ update.setChangeMessage("Change has been successfully merged by " + changeOwner.getName());
+ update.commit();
+ ChangeUpdate updateWithSubject = newUpdate(c, changeOwner);
+ updateWithSubject.setSubjectForCommit("Update with subject");
+ updateWithSubject.commit();
+ String refName = RefNames.changeMetaRef(c.getId());
+ Ref metaRefBeforeRewrite = repo.exactRef(refName);
+ if (i < maxRefsToUpdate) {
+ expectedFixedRefsToOldMetaBuilder.put(refName, metaRefBeforeRewrite.getObjectId());
+ } else {
+ expectedSkippedRefsToOldMetaBuilder.put(refName, metaRefBeforeRewrite.getObjectId());
+ }
+ }
+ ImmutableMap<String, ObjectId> expectedFixedRefsToOldMeta =
+ expectedFixedRefsToOldMetaBuilder.build();
+ ImmutableMap<String, ObjectId> expectedSkippedRefsToOldMeta =
+ expectedSkippedRefsToOldMetaBuilder.build();
+ RunOptions options = new RunOptions();
+ options.dryRun = false;
+ options.outputDiff = false;
+ options.verifyCommits = false;
+ options.maxRefsInBatch = maxRefsInBatch;
+ options.maxRefsToUpdate = maxRefsToUpdate;
+ BackfillResult backfillResult = rewriter.backfillProject(project, repo, options);
+ assertThat(backfillResult.fixedRefDiff.keySet()).isEqualTo(expectedFixedRefsToOldMeta.keySet());
+ for (Map.Entry<String, ObjectId> refEntry : expectedFixedRefsToOldMeta.entrySet()) {
+ Ref metaRefAfterRewrite = repo.exactRef(refEntry.getKey());
+ assertThat(refEntry.getValue()).isNotEqualTo(metaRefAfterRewrite.getObjectId());
+ }
+ for (Map.Entry<String, ObjectId> refEntry : expectedSkippedRefsToOldMeta.entrySet()) {
+ Ref metaRefAfterRewrite = repo.exactRef(refEntry.getKey());
+ assertThat(refEntry.getValue()).isEqualTo(metaRefAfterRewrite.getObjectId());
+ }
+ RunOptions secondRunOptions = new RunOptions();
+ secondRunOptions.dryRun = false;
+ secondRunOptions.outputDiff = false;
+ secondRunOptions.verifyCommits = false;
+ secondRunOptions.maxRefsInBatch = maxRefsInBatch;
+ secondRunOptions.maxRefsToUpdate = numberOfInvalidChanges + numberOfValidChanges;
+ BackfillResult secondRunResult = rewriter.backfillProject(project, repo, options);
+ int expectedSecondRunResult =
+ numberOfInvalidChanges > maxRefsToUpdate ? numberOfInvalidChanges - maxRefsToUpdate : 0;
+ assertThat(secondRunResult.fixedRefDiff.keySet().size()).isEqualTo(expectedSecondRunResult);
+ }
+
+ @Test
public void fixAuthorIdent() throws Exception {
Change c = newChange();
Timestamp when = TimeUtil.nowTs();
