opendev/gerrit
Code Issues Proposed changes

ChangeUtil: Delete draft change inside database transaction

Browse Source 
Deletion of draft change involves database and git operations. Gerrit
is using optimistic transaction locking strategy. With this strategy
it's expected that other client may modify the same database record,
bump row version number and the modification will fail with concurrent
database modification exception. When this happens, some git refs were
already deleted, so that the database and git are out of sync. As the
consequence manual intervention is needed to clean this up.

Delete change related database records in transaction boundary, and
remember the refs during deletion, and only delete the git refs, when
database deletion was sucessful, otherwise, rollback the database
mutation, so that the user can retry the deletion later.

As a side effect of this change, the refs deletion is optimized as it
is done in batch update.

Bug: Issue 3355
Change-Id: I0191f664c779c079352037e2a28dd0c7be12a0b1
This commit is contained in:
David Ostrovsky
2015-06-09 08:27:45 +02:00
committed by David Pursehouse
parent d6f7d2de81
commit 4a0c8c0e13
1 changed files with 53 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

70
gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java
View File

@@ -57,7 +57,9 @@ import com.google.inject.Singleton;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.NullProgressMonitor;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.PersonIdent;
@@ -78,6 +80,7 @@ import java.text.MessageFormat;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
@@ -422,15 +425,39 @@ public class ChangeUtil {
}
ReviewDb db = this.db.get();
for (PatchSet ps : db.patchSets().byChange(changeId)) {
// These should all be draft patch sets.
deleteOnlyDraftPatchSet(ps, change);
}
db.changes().beginTransaction(change.getId());
try {
Map<RevId, String> refsToDelete = new HashMap<>();
for (PatchSet ps : db.patchSets().byChange(changeId)) {
// These should all be draft patch sets.
deleteOnlyDraftPatchSetPreserveRef(db, ps);
refsToDelete.put(ps.getRevision(), ps.getRefName());
}
db.changeMessages().delete(db.changeMessages().byChange(changeId));
db.starredChanges().delete(db.starredChanges().byChange(changeId));
db.changes().delete(Collections.singleton(change));
db.changeMessages().delete(db.changeMessages().byChange(changeId));
db.starredChanges().delete(db.starredChanges().byChange(changeId));
db.changes().delete(Collections.singleton(change));
indexer.delete(change.getId());
// Delete all refs at once
try (Repository repo = gitManager.openRepository(change.getProject());
RevWalk rw = new RevWalk(repo)) {
BatchRefUpdate ru = repo.getRefDatabase().newBatchUpdate();
for (Map.Entry<RevId, String> e : refsToDelete.entrySet()) {
ru.addCommand(new ReceiveCommand(ObjectId.fromString(e.getKey().get()),
ObjectId.zeroId(), e.getValue()));
}
ru.execute(rw, NullProgressMonitor.INSTANCE);
for (ReceiveCommand cmd : ru.getCommands()) {
if (cmd.getResult() != ReceiveCommand.Result.OK) {
throw new IOException("failed: " + cmd);
}
}
}
db.commit();
indexer.delete(change.getId());
} finally {
db.rollback();
}
}
public void deleteOnlyDraftPatchSet(PatchSet patch, Change change)
@@ -461,15 +488,7 @@ public class ChangeUtil {
repo.close();
}
ReviewDb db = this.db.get();
db.accountPatchReviews().delete(db.accountPatchReviews().byPatchSet(patchSetId));
db.changeMessages().delete(db.changeMessages().byPatchSet(patchSetId));
// No need to delete from notedb; draft patch sets will be filtered out.
db.patchComments().delete(db.patchComments().byPatchSet(patchSetId));
db.patchSetApprovals().delete(db.patchSetApprovals().byPatchSet(patchSetId));
db.patchSetAncestors().delete(db.patchSetAncestors().byPatchSet(patchSetId));
db.patchSets().delete(Collections.singleton(patch));
deleteOnlyDraftPatchSetPreserveRef(this.db.get(), patch);
}
/**
@@ -511,6 +530,23 @@ public class ChangeUtil {
return (IdentifiedUser) userProvider.get();
}
private static void deleteOnlyDraftPatchSetPreserveRef(ReviewDb db,
PatchSet patch) throws NoSuchChangeException, OrmException {
PatchSet.Id patchSetId = patch.getId();
if (!patch.isDraft()) {
throw new NoSuchChangeException(patchSetId.getParentKey());
}
db.accountPatchReviews().delete(db.accountPatchReviews().byPatchSet(patchSetId));
db.changeMessages().delete(db.changeMessages().byPatchSet(patchSetId));
// No need to delete from notedb; draft patch sets will be filtered out.
db.patchComments().delete(db.patchComments().byPatchSet(patchSetId));
db.patchSetApprovals().delete(db.patchSetApprovals().byPatchSet(patchSetId));
db.patchSetAncestors().delete(db.patchSetAncestors().byPatchSet(patchSetId));
db.patchSets().delete(Collections.singleton(patch));
}
public static PatchSet.Id nextPatchSetId(PatchSet.Id id) {
return new PatchSet.Id(id.getParentKey(), id.get() + 1);
}