opendev/gerrit
Code Issues Proposed changes

Rewrite AbstractChangeUpdate to not extend MetaDataUpdate

Browse Source 
This was never a particularly good fit for ChangeUpdate, as evidenced
by all the methods we had to override to throw exceptions to say
"don't use this method, use this other method." Plus, the
BatchMetaDataUpdate interface was confusing, and moreover didn't allow
us to group draft updates into a single BatchRefUpdate.

Reduce the methods in AbstractChangeUpdate to what is actually
required. Add a new class, the somewhat clumsily named
NoteDbUpdateManager, that understands enough about ChangeUpdates to
batch both the change repo and All-Users repo changes together.

This allows us to replace:

 BatchMetaDataUpdate batch = update1.openUpdate();
 try {
   update1.writeCommit(batch);
   update2.writeCommit(batch);
   batch.commit();
 } finally {
   batch.close();
 }

With:

 NoteDbUpdateManager manager = updateManagerFactory.create(project);
 manager.add(update1);
 manager.add(update2);
 manager.execute();

The explicit creation of the update manager without having to call a
method on one of the update instances reduces one possibility of
error.

The changes to ChangeRebuilder are just enough to get it to compile;
there are still problems with this class that I identified when trying
to write tests, so a further rewrite (including tests) will be coming.

It is not a coincidence that the structure and API of
NoteDbUpdateManager are reminiscent of BatchUpdate. This will allow us
to eventually use a single NoteDbUpdateManager per BatchUpdate and
even update change and meta refs at the same time.

Change-Id: Ib163d2092b76066203b8bfd78ddc69376e0e441b
This commit is contained in:
Dave Borowitz
2016-02-18 09:49:19 -05:00
parent 2ce83d538f
commit 0e8077dc8d
12 changed files with 505 additions and 358 deletions
Download Patch File Download Diff File Expand all files Collapse all files

72
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.server.notedb;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.notedb.CommentsInNotesUtil.addCommentToMap;
@@ -23,7 +24,6 @@ import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.PatchLineComment;
import com.google.gerrit.reviewdb.client.PatchLineComment.Status;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.client.RevId;
@@ -32,18 +32,16 @@ import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.AnonymousCowardName;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gwtorm.server.OrmException;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.notes.NoteMap;
import org.eclipse.jgit.revwalk.RevCommit;
import java.io.IOException;
import java.util.ArrayList;
@@ -83,14 +81,12 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
@AnonymousCowardName String anonymousCowardName,
GitRepositoryManager repoManager,
NotesMigration migration,
MetaDataUpdate.User updateFactory,
DraftCommentNotes.Factory draftNotesFactory,
AllUsersName allUsers,
CommentsInNotesUtil commentsUtil,
@Assisted ChangeControl ctl,
@Assisted Date when) throws OrmException {
super(migration, repoManager, updateFactory, ctl, serverIdent,
anonymousCowardName, when);
super(migration, repoManager, ctl, serverIdent, anonymousCowardName, when);
this.draftsProject = allUsers;
this.commentsUtil = commentsUtil;
checkState(ctl.getUser().isIdentifiedUser(),
@@ -107,7 +103,7 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
public void insertComment(PatchLineComment c) throws OrmException {
verifyComment(c);
checkArgument(c.getStatus() == Status.DRAFT,
checkArgument(c.getStatus() == PatchLineComment.Status.DRAFT,
"Cannot insert a published comment into a ChangeDraftUpdate");
if (migration.readChanges()) {
checkArgument(!changeNotes.containsComment(c),
@@ -119,14 +115,14 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
public void upsertComment(PatchLineComment c) {
verifyComment(c);
checkArgument(c.getStatus() == Status.DRAFT,
checkArgument(c.getStatus() == PatchLineComment.Status.DRAFT,
"Cannot upsert a published comment into a ChangeDraftUpdate");
upsertComments.add(c);
}
public void updateComment(PatchLineComment c) throws OrmException {
verifyComment(c);
checkArgument(c.getStatus() == Status.DRAFT,
checkArgument(c.getStatus() == PatchLineComment.Status.DRAFT,
"Cannot update a published comment into a ChangeDraftUpdate");
// Here, we check to see if this comment existed previously as a draft.
// However, this could cause a race condition if there is a delete and an
@@ -178,12 +174,8 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
}
/** @return the tree id for the updated tree */
private ObjectId storeCommentsInNotes(AtomicBoolean removedAllComments)
throws OrmException, IOException {
if (isEmpty()) {
return null;
}
private ObjectId storeCommentsInNotes(ObjectInserter inserter,
AtomicBoolean removedAllComments) throws OrmException, IOException {
NoteMap noteMap = draftNotes.load().getNoteMap();
if (noteMap == null) {
noteMap = NoteMap.newEmptyMap();
@@ -230,7 +222,7 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
// for the caller to delete the entire ref.
boolean touchedAllRevs = updatedRevs.equals(existing.keySet());
if (touchedAllRevs && !hasComments) {
removedAllComments.set(touchedAllRevs && !hasComments);
removedAllComments.set(true);
return null;
}
@@ -238,32 +230,20 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
return noteMap.writeTree(inserter);
}
public RevCommit commit() throws IOException {
BatchMetaDataUpdate batch = openUpdate();
try {
writeCommit(batch);
return batch.commit();
} catch (OrmException e) {
throw new IOException(e);
} finally {
batch.close();
}
}
@Override
public void writeCommit(BatchMetaDataUpdate batch)
protected CommitBuilder applyImpl(ObjectInserter ins)
throws OrmException, IOException {
CommitBuilder builder = new CommitBuilder();
if (migration.writeChanges()) {
AtomicBoolean removedAllComments = new AtomicBoolean();
ObjectId treeId = storeCommentsInNotes(removedAllComments);
if (removedAllComments.get()) {
batch.removeRef(getRefName());
} else if (treeId != null) {
builder.setTreeId(treeId);
batch.write(builder);
}
CommitBuilder cb = new CommitBuilder();
cb.setAuthor(newIdent(getUser().getAccount(), when));
cb.setCommitter(new PersonIdent(serverIdent, when));
cb.setMessage("Update draft comments");
AtomicBoolean removedAllComments = new AtomicBoolean();
ObjectId treeId = storeCommentsInNotes(ins, removedAllComments);
if (removedAllComments.get()) {
return null; // Delete ref.
}
cb.setTreeId(checkNotNull(treeId));
return cb;
}
@Override
@@ -276,18 +256,6 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
return RefNames.refsDraftComments(accountId, ctl.getId());
}
@Override
protected boolean onSave(CommitBuilder commit) throws IOException,
ConfigInvalidException {
if (isEmpty()) {
return false;
}
commit.setAuthor(newIdent(getUser().getAccount(), when));
commit.setCommitter(new PersonIdent(serverIdent, when));
commit.setMessage("Update draft comments");
return true;
}
@Override
public boolean isEmpty() {
return deleteComments.isEmpty()