DeleteZombieCommentsRefs: allow the program to run in logging mode We add a dryRun boolean such that clients are flexible to run the cleanup in logging mode. If set to false, detected zombie drafts will also be deleted. Change-Id: I890b6c61a4bd39902bfb4bae5ec9476596bbb0f8 Release-Notes: skip FW-COMPATIBLE: checked Google-Bug-Id: b/207614518
diff --git a/java/com/google/gerrit/server/notedb/DeleteZombieCommentsRefs.java b/java/com/google/gerrit/server/notedb/DeleteZombieCommentsRefs.java index 0d95a26..c8d93f8 100644 --- a/java/com/google/gerrit/server/notedb/DeleteZombieCommentsRefs.java +++ b/java/com/google/gerrit/server/notedb/DeleteZombieCommentsRefs.java
@@ -37,8 +37,8 @@ import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.util.time.TimeUtil; -import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; +import com.google.inject.assistedinject.AssistedInject; import java.io.IOException; import java.sql.Timestamp; import java.util.ArrayList; @@ -67,8 +67,9 @@ * refs/draft-comments/$change_id_short/$change_id/$user_id} caused some draft refs to remain * in Git and not get deleted. These refs point to an empty tree. We delete such refs. * <li>Inspecting all draft-comment refs. Check for each draft if there exists a published comment - * with the same UUID. For now this runs in logging-only mode and does not remove these zombie - * drafts. + * with the same UUID. These comments are called zombie drafts. If the program is run in + * {@link #dryRun} mode, the zombie draft IDs will only be logged for tracking, otherwise they + * will also be deleted. * </uL> */ public class DeleteZombieCommentsRefs { @@ -81,6 +82,15 @@ private final GitRepositoryManager repoManager; private final AllUsersName allUsers; private final int cleanupPercentage; + + /** + * Run the logic in dry run mode only. That is, detected zombie drafts will be logged only but not + * deleted. Creators of this class can use {@link Factory#create(int, boolean)} to specify the dry + * run mode. If {@link Factory#create(int)} is used, the dry run mode will be set to its default: + * true. + */ + private final boolean dryRun; + private final Consumer<String> uiConsumer; @Nullable private final DraftCommentNotes.Factory draftNotesFactory; @Nullable private final ChangeNotes.Factory changeNotesFactory; @@ -90,9 +100,11 @@ public interface Factory { DeleteZombieCommentsRefs create(int cleanupPercentage); + + DeleteZombieCommentsRefs create(int cleanupPercentage, boolean dryRun); } - @Inject + @AssistedInject public DeleteZombieCommentsRefs( AllUsersName allUsers, GitRepositoryManager repoManager, @@ -106,6 +118,31 @@ allUsers, repoManager, cleanupPercentage, + /* dryRun= */ true, + (msg) -> {}, + changeNotesFactory, + draftNotesFactory, + commentsUtil, + changeUpdateFactory, + userFactory); + } + + @AssistedInject + public DeleteZombieCommentsRefs( + AllUsersName allUsers, + GitRepositoryManager repoManager, + ChangeNotes.Factory changeNotesFactory, + DraftCommentNotes.Factory draftNotesFactory, + CommentsUtil commentsUtil, + ChangeUpdate.Factory changeUpdateFactory, + IdentifiedUser.GenericFactory userFactory, + @Assisted Integer cleanupPercentage, + @Assisted boolean dryRun) { + this( + allUsers, + repoManager, + cleanupPercentage, + dryRun, (msg) -> {}, changeNotesFactory, draftNotesFactory, @@ -119,13 +156,24 @@ GitRepositoryManager repoManager, Integer cleanupPercentage, Consumer<String> uiConsumer) { - this(allUsers, repoManager, cleanupPercentage, uiConsumer, null, null, null, null, null); + this( + allUsers, + repoManager, + cleanupPercentage, + /* dryRun= */ false, + uiConsumer, + null, + null, + null, + null, + null); } private DeleteZombieCommentsRefs( AllUsersName allUsers, GitRepositoryManager repoManager, Integer cleanupPercentage, + boolean dryRun, Consumer<String> uiConsumer, @Nullable ChangeNotes.Factory changeNotesFactory, @Nullable DraftCommentNotes.Factory draftNotesFactory, @@ -135,6 +183,7 @@ this.allUsers = allUsers; this.repoManager = repoManager; this.cleanupPercentage = (cleanupPercentage == null) ? 100 : cleanupPercentage; + this.dryRun = dryRun; this.uiConsumer = uiConsumer; this.draftNotesFactory = draftNotesFactory; this.changeNotesFactory = changeNotesFactory; @@ -168,6 +217,12 @@ .collect(toImmutableList()); logInfo(String.format("Number of zombie refs to be cleaned = %d", zombieRefs.size())); + if (dryRun) { + logInfo( + "Running in dry run mode. Skipping deletion of draft refs pointing to an empty tree."); + return; + } + long zombieRefsCnt = zombieRefs.size(); long deletedRefsCnt = 0; long startTime = System.currentTimeMillis(); @@ -241,7 +296,7 @@ "Draft comment with uuid '%s' of change %s, account %s, written on %s," + " is a zombie draft that is already published.", zombieDraft.key.uuid, changeId, accountId, zombieDraft.writtenOn)); - if (!zombieDrafts.isEmpty()) { + if (!zombieDrafts.isEmpty() && !dryRun) { deleteZombieComments(accountId, notes, zombieDrafts); } numZombies += zombieDrafts.size();
diff --git a/javatests/com/google/gerrit/acceptance/server/change/DeleteZombieDraftIT.java b/javatests/com/google/gerrit/acceptance/server/change/DeleteZombieDraftIT.java index 25f01e3..1eef944 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/DeleteZombieDraftIT.java +++ b/javatests/com/google/gerrit/acceptance/server/change/DeleteZombieDraftIT.java
@@ -30,10 +30,12 @@ import com.google.gerrit.extensions.common.CommentInfo; import com.google.gerrit.server.notedb.ChangeNoteJson; import com.google.gerrit.server.notedb.DeleteZombieCommentsRefs; +import com.google.gerrit.testing.ConfigSuite; import com.google.gson.JsonParser; import com.google.inject.Inject; import java.util.List; import org.apache.commons.lang3.reflect.TypeLiteral; +import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectLoader; import org.eclipse.jgit.lib.Ref; @@ -42,13 +44,35 @@ import org.eclipse.jgit.revwalk.RevTree; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.treewalk.TreeWalk; +import org.junit.Before; import org.junit.Test; /** Test for {@link com.google.gerrit.server.notedb.DeleteZombieCommentsRefs}. */ public class DeleteZombieDraftIT extends AbstractDaemonTest { + private static final String TEST_PARAMETER_MARKER = "test_only_parameter"; @Inject private DeleteZombieCommentsRefs.Factory deleteZombieDraftsFactory; @Inject private ChangeNoteJson changeNoteJson; + private boolean dryRun; + + @ConfigSuite.Default + public static Config dryRunMode() { + Config config = new Config(); + config.setBoolean(TEST_PARAMETER_MARKER, null, "dryRun", true); + return config; + } + + @ConfigSuite.Config + public static Config deleteMode() { + Config config = new Config(); + config.setBoolean(TEST_PARAMETER_MARKER, null, "dryRun", false); + return config; + } + + @Before + public void setUp() throws Exception { + dryRun = baseConfig.getBoolean(TEST_PARAMETER_MARKER, "dryRun", true); + } @Test public void draftRefWithOneZombie() throws Exception { @@ -73,9 +97,13 @@ // Run the cleanup logic. The zombie draft is cleared. The published comment is untouched. DeleteZombieCommentsRefs worker = - deleteZombieDraftsFactory.create(/* cleanupPercentage= */ 100); + deleteZombieDraftsFactory.create(/* cleanupPercentage= */ 100, dryRun); assertThat(worker.deleteDraftCommentsThatAreAlsoPublished()).isEqualTo(1); - assertThat(getDraftsByParsingDraftRef(draftRef.getName(), revId)).isEmpty(); + if (dryRun) { + assertThat(getDraftsByParsingDraftRef(draftRef.getName(), revId)).hasSize(1); + } else { + assertThat(getDraftsByParsingDraftRef(draftRef.getName(), revId)).isEmpty(); + } assertNumPublishedComments(changeId, 1); } @@ -105,17 +133,25 @@ // Run the zombie cleanup logic. Zombie draft ref for PS2 will be removed. DeleteZombieCommentsRefs worker = - deleteZombieDraftsFactory.create(/* cleanupPercentage= */ 100); + deleteZombieDraftsFactory.create(/* cleanupPercentage= */ 100, dryRun); assertThat(worker.deleteDraftCommentsThatAreAlsoPublished()).isEqualTo(1); assertThat(getDraftsByParsingDraftRef(draftRef.getName(), r1.getCommit().name())).hasSize(1); - assertThat(getDraftsByParsingDraftRef(draftRef.getName(), r2.getCommit().name())).isEmpty(); + if (dryRun) { + assertThat(getDraftsByParsingDraftRef(draftRef.getName(), r2.getCommit().name())).hasSize(1); + } else { + assertThat(getDraftsByParsingDraftRef(draftRef.getName(), r2.getCommit().name())).isEmpty(); + } assertNumPublishedComments(changeId, 1); // Re-run the worker: nothing happens. - assertThat(worker.deleteDraftCommentsThatAreAlsoPublished()).isEqualTo(0); + assertThat(worker.deleteDraftCommentsThatAreAlsoPublished()).isEqualTo(dryRun ? 1 : 0); assertNumDrafts(changeId, 1); assertThat(getDraftsByParsingDraftRef(draftRef.getName(), r1.getCommit().name())).hasSize(1); - assertThat(getDraftsByParsingDraftRef(draftRef.getName(), r2.getCommit().name())).isEmpty(); + if (dryRun) { + assertThat(getDraftsByParsingDraftRef(draftRef.getName(), r2.getCommit().name())).hasSize(1); + } else { + assertThat(getDraftsByParsingDraftRef(draftRef.getName(), r2.getCommit().name())).isEmpty(); + } assertNumPublishedComments(changeId, 1); }