Merge "Merge branch 'stable-3.4' into stable-3.5" into stable-3.5
diff --git a/java/com/google/gerrit/pgm/CopyApprovals.java b/java/com/google/gerrit/pgm/CopyApprovals.java
index 73b0da6..f62d60c 100644
--- a/java/com/google/gerrit/pgm/CopyApprovals.java
+++ b/java/com/google/gerrit/pgm/CopyApprovals.java
@@ -72,7 +72,7 @@
sysInjector.injectMembers(this);
try {
- recursiveApprovalCopier.persist();
+ recursiveApprovalCopier.persistStandalone();
return 0;
} catch (Exception e) {
throw die(e.getMessage(), e);
diff --git a/java/com/google/gerrit/server/approval/RecursiveApprovalCopier.java b/java/com/google/gerrit/server/approval/RecursiveApprovalCopier.java
index 0926daf..24595674 100644
--- a/java/com/google/gerrit/server/approval/RecursiveApprovalCopier.java
+++ b/java/com/google/gerrit/server/approval/RecursiveApprovalCopier.java
@@ -16,13 +16,23 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Lists;
+import com.google.common.flogger.FluentLogger;
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.ListeningExecutorService;
+import com.google.common.util.concurrent.MoreExecutors;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.git.RefUpdateUtil;
+import com.google.gerrit.server.FanOutExecutor;
import com.google.gerrit.server.InternalUser;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
@@ -31,50 +41,219 @@
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.Inject;
import java.io.IOException;
+import java.util.Collection;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Consumer;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
+import org.eclipse.jgit.internal.storage.file.RefDirectory;
+import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.RefDatabase;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.transport.ReceiveCommand;
public class RecursiveApprovalCopier {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ private static final int SLICE_MAX_REFS = 1000;
private final BatchUpdate.Factory batchUpdateFactory;
private final GitRepositoryManager repositoryManager;
private final InternalUser.Factory internalUserFactory;
private final ApprovalsUtil approvalsUtil;
+ private final ChangeNotes.Factory changeNotesFactory;
+ private final ListeningExecutorService executor;
+
+ private final ConcurrentHashMap<Project.NameKey, List<ReceiveCommand>> pendingRefUpdates =
+ new ConcurrentHashMap<>();
+
+ private volatile boolean failedForAtLeastOneProject;
+
+ private final AtomicInteger totalCopyApprovalsTasks = new AtomicInteger();
+ private final AtomicInteger finishedCopyApprovalsTasks = new AtomicInteger();
+
+ private final AtomicInteger totalRefUpdates = new AtomicInteger();
+ private final AtomicInteger finishedRefUpdates = new AtomicInteger();
@Inject
public RecursiveApprovalCopier(
BatchUpdate.Factory batchUpdateFactory,
GitRepositoryManager repositoryManager,
InternalUser.Factory internalUserFactory,
- ApprovalsUtil approvalsUtil) {
+ ApprovalsUtil approvalsUtil,
+ ChangeNotes.Factory changeNotesFactory,
+ @FanOutExecutor ExecutorService executor) {
this.batchUpdateFactory = batchUpdateFactory;
this.repositoryManager = repositoryManager;
this.internalUserFactory = internalUserFactory;
this.approvalsUtil = approvalsUtil;
+ this.changeNotesFactory = changeNotesFactory;
+ this.executor = MoreExecutors.listeningDecorator(executor);
}
- public void persist()
- throws UpdateException, RestApiException, RepositoryNotFoundException, IOException {
- for (Project.NameKey project : repositoryManager.list()) {
- persist(project, null);
+ /**
+ * This method assumes it is used as a standalone program having exclusive access to the Git
+ * repositories. Therefore, it will (safely) skip locking of the loose refs when performing batch
+ * ref-updates.
+ */
+ public void persistStandalone()
+ throws RepositoryNotFoundException, IOException, InterruptedException, ExecutionException {
+ persist(repositoryManager.list(), null, false);
+
+ if (failedForAtLeastOneProject) {
+ throw new RuntimeException("There were errors, check the logs for details");
}
}
public void persist(Project.NameKey project, @Nullable Consumer<Change> labelsCopiedListener)
- throws IOException, UpdateException, RestApiException, RepositoryNotFoundException {
- try (BatchUpdate bu =
- batchUpdateFactory.create(project, internalUserFactory.create(), TimeUtil.nowTs());
- Repository repository = repositoryManager.openRepository(project)) {
- for (Ref changeMetaRef :
+ throws IOException, RepositoryNotFoundException, InterruptedException, ExecutionException {
+ persist(ImmutableList.of(project), labelsCopiedListener, true);
+ }
+
+ private void persist(
+ Collection<Project.NameKey> projects,
+ @Nullable Consumer<Change> labelsCopiedListener,
+ boolean shouldLockLooseRefs)
+ throws InterruptedException, ExecutionException, RepositoryNotFoundException, IOException {
+ List<ListenableFuture<Void>> copyApprovalsTasks = new LinkedList<>();
+ for (Project.NameKey project : projects) {
+ copyApprovalsTasks.addAll(submitCopyApprovalsTasks(project, labelsCopiedListener));
+ }
+ Futures.successfulAsList(copyApprovalsTasks).get();
+
+ List<ListenableFuture<Void>> batchRefUpdateTasks =
+ submitBatchRefUpdateTasks(shouldLockLooseRefs);
+ Futures.successfulAsList(batchRefUpdateTasks).get();
+ }
+
+ private List<ListenableFuture<Void>> submitCopyApprovalsTasks(
+ Project.NameKey project, @Nullable Consumer<Change> labelsCopiedListener)
+ throws RepositoryNotFoundException, IOException {
+ List<ListenableFuture<Void>> futures = new LinkedList<>();
+ try (Repository repository = repositoryManager.openRepository(project)) {
+ ImmutableList<Ref> allMetaRefs =
repository.getRefDatabase().getRefsByPrefix(RefNames.REFS_CHANGES).stream()
.filter(r -> r.getName().endsWith(RefNames.META_SUFFIX))
- .collect(toImmutableList())) {
- Change.Id changeId = Change.Id.fromRef(changeMetaRef.getName());
- bu.addOp(changeId, new PersistCopiedVotesOp(approvalsUtil, labelsCopiedListener));
+ .collect(toImmutableList());
+
+ totalCopyApprovalsTasks.addAndGet(allMetaRefs.size());
+
+ for (List<Ref> slice : Lists.partition(allMetaRefs, SLICE_MAX_REFS)) {
+ futures.add(
+ executor.submit(
+ () -> {
+ copyApprovalsForSlice(project, slice, labelsCopiedListener);
+ return null;
+ }));
}
- bu.execute();
+ }
+ return futures;
+ }
+
+ private void copyApprovalsForSlice(
+ Project.NameKey project, List<Ref> slice, @Nullable Consumer<Change> labelsCopiedListener)
+ throws Exception {
+ try {
+ copyApprovalsForSlice(project, slice, labelsCopiedListener, false);
+ } catch (Exception e) {
+ failedForAtLeastOneProject = true;
+ logger.atSevere().withCause(e).log(
+ "Error in a slice of project %s, will retry and skip corrupt meta-refs", project);
+ copyApprovalsForSlice(project, slice, labelsCopiedListener, true);
+ }
+ logProgress();
+ }
+
+ private void copyApprovalsForSlice(
+ Project.NameKey project,
+ List<Ref> slice,
+ @Nullable Consumer<Change> labelsCopiedListener,
+ boolean checkForCorruptMetaRefs)
+ throws Exception {
+ logger.atInfo().log("copy-approvals for a slice of %s project", project);
+ try (BatchUpdate bu =
+ batchUpdateFactory.create(project, internalUserFactory.create(), TimeUtil.nowTs())) {
+ for (Ref metaRef : slice) {
+ Change.Id changeId = Change.Id.fromRef(metaRef.getName());
+ if (checkForCorruptMetaRefs && isCorrupt(project, changeId)) {
+ logger.atSevere().log("skipping corrupt meta-ref %s", metaRef.getName());
+ continue;
+ }
+ bu.addOp(
+ changeId,
+ new RecursiveApprovalCopier.PersistCopiedVotesOp(approvalsUtil, labelsCopiedListener));
+ }
+
+ BatchRefUpdate bru = bu.prepareRefUpdates();
+ if (bru != null) {
+ List<ReceiveCommand> cmds = bru.getCommands();
+ pendingRefUpdates.compute(
+ project,
+ (p, u) -> {
+ if (u == null) {
+ return new LinkedList<>(cmds);
+ }
+ u.addAll(cmds);
+ return u;
+ });
+ totalRefUpdates.addAndGet(cmds.size());
+ }
+
+ finishedCopyApprovalsTasks.addAndGet(slice.size());
+ }
+ }
+
+ private List<ListenableFuture<Void>> submitBatchRefUpdateTasks(boolean shouldLockLooseRefs) {
+ logger.atInfo().log("submitting batch ref-update tasks");
+ List<ListenableFuture<Void>> futures = new LinkedList<>();
+ for (Map.Entry<Project.NameKey, List<ReceiveCommand>> e : pendingRefUpdates.entrySet()) {
+ Project.NameKey project = e.getKey();
+ List<ReceiveCommand> updates = e.getValue();
+ futures.add(
+ executor.submit(
+ () -> {
+ executeRefUpdates(project, updates, shouldLockLooseRefs);
+ return null;
+ }));
+ }
+ return futures;
+ }
+
+ private void executeRefUpdates(
+ Project.NameKey project, List<ReceiveCommand> updates, boolean shouldLockLooseRefs)
+ throws RepositoryNotFoundException, IOException {
+ logger.atInfo().log(
+ "executing batch ref-update for project %s, size %d", project, updates.size());
+ try (Repository repository = repositoryManager.openRepository(project)) {
+ RefDatabase refdb = repository.getRefDatabase();
+ BatchRefUpdate bu;
+ if (refdb instanceof RefDirectory) {
+ bu = ((RefDirectory) refdb).newBatchUpdate(shouldLockLooseRefs);
+ } else {
+ bu = refdb.newBatchUpdate();
+ }
+ bu.addCommand(updates);
+ RefUpdateUtil.executeChecked(bu, repository);
+
+ finishedRefUpdates.addAndGet(updates.size());
+ logProgress();
+ }
+ }
+
+ private boolean isCorrupt(Project.NameKey project, Change.Id changeId) {
+ Change c = ChangeNotes.Factory.newChange(project, changeId);
+ try {
+ changeNotesFactory.createForBatchUpdate(c, true);
+ return false;
+ } catch (Exception e) {
+ logger.atSevere().withCause(e).log(e.getMessage());
+ return true;
}
}
@@ -88,6 +267,15 @@
}
}
+ private void logProgress() {
+ logger.atInfo().log(
+ "copy-approvals tasks done: %d/%d, ref-update tasks done: %d/%d",
+ finishedCopyApprovalsTasks.get(),
+ totalCopyApprovalsTasks.get(),
+ finishedRefUpdates.get(),
+ totalRefUpdates.get());
+ }
+
private static class PersistCopiedVotesOp implements BatchUpdateOp {
private final ApprovalsUtil approvalsUtil;
private final Consumer<Change> listener;
diff --git a/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java b/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
index 9345d98..e817fe6 100644
--- a/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
+++ b/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
@@ -306,6 +306,12 @@
}
}
+ public BatchRefUpdate prepare() throws IOException {
+ checkNotExecuted();
+ stage();
+ return prepare(changeRepo, false, pushCert);
+ }
+
@Nullable
public BatchRefUpdate execute() throws IOException {
return execute(false);
@@ -353,7 +359,7 @@
}
}
- private BatchRefUpdate execute(OpenRepo or, boolean dryrun, @Nullable PushCertificate pushCert)
+ private BatchRefUpdate prepare(OpenRepo or, boolean dryrun, @Nullable PushCertificate pushCert)
throws IOException {
if (or == null || or.cmds.isEmpty()) {
return null;
@@ -382,7 +388,13 @@
bru = listener.beforeUpdateRefs(bru);
}
- if (!dryrun) {
+ return bru;
+ }
+
+ private BatchRefUpdate execute(OpenRepo or, boolean dryrun, @Nullable PushCertificate pushCert)
+ throws IOException {
+ BatchRefUpdate bru = prepare(or, dryrun, pushCert);
+ if (bru != null && !dryrun) {
RefUpdateUtil.executeChecked(bru, or.rw);
}
return bru;
diff --git a/java/com/google/gerrit/server/update/BatchUpdate.java b/java/com/google/gerrit/server/update/BatchUpdate.java
index 232aa98..b88d0c0 100644
--- a/java/com/google/gerrit/server/update/BatchUpdate.java
+++ b/java/com/google/gerrit/server/update/BatchUpdate.java
@@ -444,6 +444,11 @@
execute(ImmutableList.of(this), ImmutableList.of(), false);
}
+ public BatchRefUpdate prepareRefUpdates() throws Exception {
+ ChangesHandle handle = executeChangeOps(ImmutableList.of(), false);
+ return handle.prepare();
+ }
+
public boolean isExecuted() {
return executed;
}
@@ -620,6 +625,10 @@
checkArgument(old == null, "result for change %s already set: %s", id, old);
}
+ public BatchRefUpdate prepare() throws IOException {
+ return manager.prepare();
+ }
+
void execute() throws IOException {
BatchUpdate.this.batchRefUpdate = manager.execute(dryrun);
BatchUpdate.this.executed = manager.isExecuted();
diff --git a/javatests/com/google/gerrit/acceptance/api/change/CopyApprovalsIT.java b/javatests/com/google/gerrit/acceptance/api/change/CopyApprovalsIT.java
index 90ee13e..c2f7771 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/CopyApprovalsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/CopyApprovalsIT.java
@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.api.change;
import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.fail;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.Iterables;
@@ -25,15 +26,20 @@
import com.google.gerrit.entities.LabelId;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.common.ApprovalInfo;
import com.google.gerrit.server.approval.RecursiveApprovalCopier;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.inject.Inject;
+import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.RefUpdate;
import org.junit.Test;
public class CopyApprovalsIT extends AbstractDaemonTest {
@@ -42,8 +48,8 @@
@Test
public void multipleProjects() throws Exception {
- projectOperations.newProject().name("secondProject").create();
- TestRepository<InMemoryRepository> secondRepo = cloneProject(project, admin);
+ Project.NameKey secondProject = projectOperations.newProject().name("secondProject").create();
+ TestRepository<InMemoryRepository> secondRepo = cloneProject(secondProject, admin);
PushOneCommit.Result change1 = createChange();
gApi.changes().id(change1.getChangeId()).current().review(ReviewInput.recommend());
@@ -66,12 +72,11 @@
// change the project config to make the vote that was not copied to be copied once we do the
// schema upgrade.
try (ProjectConfigUpdate u = updateProject(allProjects)) {
- u.getConfig()
- .updateLabelType(LabelId.CODE_REVIEW, b -> b.setCopyAnyScore(/* copyAnyScore= */ true));
+ u.getConfig().updateLabelType(LabelId.CODE_REVIEW, b -> b.setCopyAnyScore(true));
u.save();
}
- recursiveApprovalCopier.persist();
+ recursiveApprovalCopier.persistStandalone();
ApprovalInfo vote1 =
Iterables.getOnlyElement(
@@ -87,11 +92,70 @@
}
@Test
+ public void corruptChangeInOneProject_OtherProjectsProcessed() throws Exception {
+ Project.NameKey corruptProject = projectOperations.newProject().name("corruptProject").create();
+ TestRepository<InMemoryRepository> corruptRepo = cloneProject(corruptProject, admin);
+
+ PushOneCommit.Result change1 = createChange();
+ gApi.changes().id(change1.getChangeId()).current().review(ReviewInput.recommend());
+ PushOneCommit.Result change2 = createChange(corruptRepo);
+ gApi.changes().id(change2.getChangeId()).current().review(ReviewInput.dislike());
+
+ // these amends are reworks so votes will not be copied.
+ amendChange(change1.getChangeId());
+ amendChange(change2.getChangeId(), "refs/for/master", admin, corruptRepo);
+
+ // votes don't exist on the new patch-set.
+ assertThat(gApi.changes().id(change1.getChangeId()).current().votes()).isEmpty();
+ assertThat(gApi.changes().id(change2.getChangeId()).current().votes()).isEmpty();
+
+ // change the project config to make the vote that was not copied to be copied once we do the
+ // schema upgrade.
+ try (ProjectConfigUpdate u = updateProject(allProjects)) {
+ u.getConfig().updateLabelType(LabelId.CODE_REVIEW, b -> b.setCopyAnyScore(true));
+ u.save();
+ }
+
+ // make the meta-ref of change2 corrupt by updating it to the commit of the current patch-set
+ ObjectId correctMetaRefObjectId;
+ String metaRef = RefNames.changeMetaRef(change2.getChange().getId());
+ try (TestRepository<InMemoryRepository> serverSideCorruptRepo =
+ new TestRepository<>((InMemoryRepository) repoManager.openRepository(corruptProject))) {
+ RefUpdate ru = forceUpdate(serverSideCorruptRepo, metaRef, change2.getPatchSet().commitId());
+ correctMetaRefObjectId = ru.getOldObjectId();
+
+ try {
+ recursiveApprovalCopier.persistStandalone();
+ fail("Expected exception when a project contains corrupt change");
+ } catch (Exception e) {
+ assertThat(e.getMessage()).contains("check the logs");
+ } finally {
+ // fix the meta-ref by setting it back to its correct objectId
+ forceUpdate(serverSideCorruptRepo, metaRef, correctMetaRefObjectId);
+ }
+ }
+
+ ApprovalInfo vote1 =
+ Iterables.getOnlyElement(
+ gApi.changes().id(change1.getChangeId()).current().votes().values());
+ assertThat(vote1.value).isEqualTo(1);
+ assertThat(vote1._accountId).isEqualTo(admin.id().get());
+ }
+
+ private RefUpdate forceUpdate(
+ TestRepository<InMemoryRepository> repo, String ref, ObjectId newObjectId)
+ throws IOException {
+ RefUpdate ru = repo.getRepository().updateRef(ref);
+ ru.setNewObjectId(newObjectId);
+ ru.forceUpdate();
+ return ru;
+ }
+
+ @Test
public void changeWithPersistedVotesNotHarmed() throws Exception {
// change the project config to copy all votes
try (ProjectConfigUpdate u = updateProject(allProjects)) {
- u.getConfig()
- .updateLabelType(LabelId.CODE_REVIEW, b -> b.setCopyAnyScore(/* copyAnyScore= */ true));
+ u.getConfig().updateLabelType(LabelId.CODE_REVIEW, b -> b.setCopyAnyScore(true));
u.save();
}
@@ -142,8 +206,7 @@
// change the project config to make the vote that was not copied to be copied once we do the
// schema upgrade.
try (ProjectConfigUpdate u = updateProject(allProjects)) {
- u.getConfig()
- .updateLabelType(LabelId.CODE_REVIEW, b -> b.setCopyAnyScore(/* copyAnyScore= */ true));
+ u.getConfig().updateLabelType(LabelId.CODE_REVIEW, b -> b.setCopyAnyScore(true));
u.save();
}
@@ -157,4 +220,38 @@
assertThat(vote1._accountId).isEqualTo(admin.id().get());
}
}
+
+ @Test
+ public void oneCorruptChange_otherChangesProcessed() throws Exception {
+ PushOneCommit.Result good = createChange();
+ gApi.changes().id(good.getChangeId()).current().review(ReviewInput.recommend());
+ // this amend is a rework so votes will not be copied.
+ amendChange(good.getChangeId());
+
+ PushOneCommit.Result corrupt = createChange();
+
+ // change the project config to make the vote that was not copied to be copied once we do the
+ // schema upgrade.
+ try (ProjectConfigUpdate u = updateProject(allProjects)) {
+ u.getConfig().updateLabelType(LabelId.CODE_REVIEW, b -> b.setCopyAnyScore(true));
+ u.save();
+ }
+
+ // make the meta-ref corrupt by updating it to the commit of the current patch-set
+ String metaRef = RefNames.changeMetaRef(corrupt.getChange().getId());
+ try (TestRepository<InMemoryRepository> serverSideTestRepo =
+ new TestRepository<>((InMemoryRepository) repoManager.openRepository(project))) {
+ RefUpdate ru = forceUpdate(serverSideTestRepo, metaRef, corrupt.getPatchSet().commitId());
+ try {
+ recursiveApprovalCopier.persist(project, null);
+ } finally {
+ forceUpdate(serverSideTestRepo, metaRef, ru.getOldObjectId());
+ }
+ }
+
+ ApprovalInfo vote1 =
+ Iterables.getOnlyElement(gApi.changes().id(good.getChangeId()).current().votes().values());
+ assertThat(vote1.value).isEqualTo(1);
+ assertThat(vote1._accountId).isEqualTo(admin.id().get());
+ }
}
diff --git a/modules/jgit b/modules/jgit
index d013761..cb90ed0 160000
--- a/modules/jgit
+++ b/modules/jgit
@@ -1 +1 @@
-Subproject commit d01376106af8800017ac3c08d7c7ac1fd5ccc9ee
+Subproject commit cb90ed08526bd51f04e5d72e3ba3cf5bd30c11e4
