opendev/gerrit
Code Issues Proposed changes

BatchUpdate: Don't update change if there was nothing to do

Browse Source 
Committing the transaction and indexing the change can be expensive,
so allow ops to skip that if they did nothing. Similar to how
updateRepo can be a no-op if no commands were added, except we don't
communicate this through the context and instead through a return
value. This is distinct from the insert/save/deleteChange methods on
ChangeContext, which refer specifically to the Change entity; a method
could update a non-Change entity and return true without calling
saveChange.

Change-Id: Id51eec8f9dfa35c80f062a365195f25777f60006
This commit is contained in:
Dave Borowitz
2016-01-20 13:44:48 -05:00
parent 3da50441d9
commit c4365ffe8b
19 changed files with 65 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java
View File

@@ -118,7 +118,7 @@ public class Abandon implements RestModifyView<ChangeResource, AbandonInput>,
}
@Override
public void updateChange(ChangeContext ctx) throws OrmException,
public boolean updateChange(ChangeContext ctx) throws OrmException,
ResourceConflictException {
change = ctx.getChange();
PatchSet.Id psId = change.currentPatchSetId();
@@ -137,6 +137,7 @@ public class Abandon implements RestModifyView<ChangeResource, AbandonInput>,
update.setStatus(change.getStatus());
message = newMessage(ctx.getDb());
cmUtil.addChangeMessage(ctx.getDb(), update, message);
return true;
}
private ChangeMessage newMessage(ReviewDb db) throws OrmException {

3
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
View File

@@ -292,7 +292,7 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
}
@Override
public void updateChange(ChangeContext ctx) throws OrmException, IOException {
public boolean updateChange(ChangeContext ctx) throws OrmException, IOException {
change = ctx.getChange(); // Use defensive copy created by ChangeControl.
ReviewDb db = ctx.getDb();
ChangeControl ctl = ctx.getControl();
@@ -335,6 +335,7 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
changeMessage.setMessage(message);
cmUtil.addChangeMessage(db, update, changeMessage);
}
return true;
}
@Override

3
gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java
View File

@@ -104,7 +104,7 @@ public class CreateDraftComment implements RestModifyView<RevisionResource, Draf
}
@Override
public void updateChange(ChangeContext ctx)
public boolean updateChange(ChangeContext ctx)
throws ResourceNotFoundException, OrmException {
PatchSet ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
if (ps == null) {
@@ -126,6 +126,7 @@ public class CreateDraftComment implements RestModifyView<RevisionResource, Draf
comment, patchListCache, ctx.getChange(), ps);
plcUtil.insertComments(
ctx.getDb(), ctx.getUpdate(psId), Collections.singleton(comment));
return true;
}
}
}

3
gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftChangeOp.java
View File

@@ -62,7 +62,7 @@ class DeleteDraftChangeOp extends BatchUpdate.Op {
}
@Override
public void updateChange(ChangeContext ctx)
public boolean updateChange(ChangeContext ctx)
throws RestApiException, OrmException {
checkState(ctx.getOrder() == BatchUpdate.Order.DB_BEFORE_REPO,
"must use DeleteDraftChangeOp with DB_BEFORE_REPO");
@@ -100,6 +100,7 @@ class DeleteDraftChangeOp extends BatchUpdate.Op {
db.changeMessages().delete(db.changeMessages().byChange(id));
starredChangesUtil.unstarAll(id);
ctx.deleteChange();
return true;
}
@Override

5
gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftComment.java
View File

@@ -86,12 +86,12 @@ public class DeleteDraftComment
}
@Override
public void updateChange(ChangeContext ctx)
public boolean updateChange(ChangeContext ctx)
throws ResourceNotFoundException, OrmException {
Optional<PatchLineComment> maybeComment =
plcUtil.get(ctx.getDb(), ctx.getNotes(), key);
if (!maybeComment.isPresent()) {
return; // Nothing to do.
return false; // Nothing to do.
}
PatchSet.Id psId = key.getParentKey().getParentKey();
PatchSet ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
@@ -102,6 +102,7 @@ public class DeleteDraftComment
setCommentRevId(c, patchListCache, ctx.getChange(), ps);
plcUtil.deleteComments(
ctx.getDb(), ctx.getUpdate(psId), Collections.singleton(c));
return true;
}
}
}

5
gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java
View File

@@ -100,11 +100,11 @@ public class DeleteDraftPatchSet implements RestModifyView<RevisionResource, Inp
}
@Override
public void updateChange(ChangeContext ctx)
public boolean updateChange(ChangeContext ctx)
throws RestApiException, OrmException, IOException {
patchSet = psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
if (patchSet == null) {
return; // Nothing to do.
return false; // Nothing to do.
}
if (!patchSet.isDraft()) {
throw new ResourceConflictException("Patch set is not a draft");
@@ -118,6 +118,7 @@ public class DeleteDraftPatchSet implements RestModifyView<RevisionResource, Inp
deleteDraftPatchSet(patchSet, ctx);
deleteOrUpdateDraftChange(ctx);
return true;
}
@Override

3
gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java
View File

@@ -91,7 +91,7 @@ public class DeleteVote implements RestModifyView<VoteResource, Input> {
}
@Override
public void updateChange(ChangeContext ctx)
public boolean updateChange(ChangeContext ctx)
throws OrmException, AuthException, ResourceNotFoundException {
IdentifiedUser user = ctx.getUser().asIdentifiedUser();
Change change = ctx.getChange();
@@ -135,6 +135,7 @@ public class DeleteVote implements RestModifyView<VoteResource, Input> {
cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(psId),
changeMessage);
}
return true;
}
}

3
gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
View File

@@ -204,7 +204,7 @@ public class PatchSetInserter extends BatchUpdate.Op {
}
@Override
public void updateChange(ChangeContext ctx) throws OrmException,
public boolean updateChange(ChangeContext ctx) throws OrmException,
InvalidChangeOperationException, IOException {
ChangeControl ctl = ctx.getControl();
@@ -249,6 +249,7 @@ public class PatchSetInserter extends BatchUpdate.Op {
if (changeMessage != null) {
cmUtil.addChangeMessage(db, update, changeMessage);
}
return true;
}
@Override

3
gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
View File

@@ -354,7 +354,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
@Override
public void updateChange(ChangeContext ctx)
public boolean updateChange(ChangeContext ctx)
throws OrmException, ResourceConflictException {
user = ctx.getUser().asIdentifiedUser();
change = ctx.getChange();
@@ -369,6 +369,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
if (dirty) {
ctx.saveChange();
}
return dirty;
}
@Override

3
gerrit-server/src/main/java/com/google/gerrit/server/change/PublishDraftPatchSet.java
View File

@@ -168,7 +168,7 @@ public class PublishDraftPatchSet implements RestModifyView<RevisionResource, In
}
@Override
public void updateChange(ChangeContext ctx)
public boolean updateChange(ChangeContext ctx)
throws RestApiException, OrmException, IOException {
if (!ctx.getControl().canPublish(ctx.getDb())) {
throw new AuthException("Cannot publish this draft patch set");
@@ -182,6 +182,7 @@ public class PublishDraftPatchSet implements RestModifyView<RevisionResource, In
saveChange(ctx);
savePatchSet(ctx);
addReviewers(ctx);
return true;
}
private void saveChange(ChangeContext ctx) {

3
gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java
View File

@@ -109,7 +109,7 @@ public class PutDraftComment implements RestModifyView<DraftCommentResource, Dra
}
@Override
public void updateChange(ChangeContext ctx)
public boolean updateChange(ChangeContext ctx)
throws ResourceNotFoundException, OrmException {
Optional<PatchLineComment> maybeComment =
plcUtil.get(ctx.getDb(), ctx.getNotes(), key);
@@ -152,6 +152,7 @@ public class PutDraftComment implements RestModifyView<DraftCommentResource, Dra
plcUtil.updateComments(ctx.getDb(), update,
Collections.singleton(update(comment, in)));
}
return true;
}
}

5
gerrit-server/src/main/java/com/google/gerrit/server/change/PutTopic.java
View File

@@ -98,13 +98,13 @@ public class PutTopic implements RestModifyView<ChangeResource, Input>,
}
@Override
public void updateChange(ChangeContext ctx) throws OrmException {
public boolean updateChange(ChangeContext ctx) throws OrmException {
change = ctx.getChange();
ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId());
newTopicName = Strings.nullToEmpty(input.topic);
oldTopicName = Strings.nullToEmpty(change.getTopic());
if (oldTopicName.equals(newTopicName)) {
return;
return false;
}
String summary;
if (oldTopicName.isEmpty()) {
@@ -128,6 +128,7 @@ public class PutTopic implements RestModifyView<ChangeResource, Input>,
change.currentPatchSetId());
cmsg.setMessage(summary);
cmUtil.addChangeMessage(ctx.getDb(), update, cmsg);
return true;
}
@Override

5
gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java
View File

@@ -152,10 +152,11 @@ public class RebaseChangeOp extends BatchUpdate.Op {
}
@Override
public void updateChange(ChangeContext ctx)
public boolean updateChange(ChangeContext ctx)
throws OrmException, InvalidChangeOperationException, IOException {
patchSetInserter.updateChange(ctx);
boolean ret = patchSetInserter.updateChange(ctx);
rebasedPatchSet = patchSetInserter.getPatchSet();
return ret;
}
@Override

3
gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java
View File

@@ -108,7 +108,7 @@ public class Restore implements RestModifyView<ChangeResource, RestoreInput>,
}
@Override
public void updateChange(ChangeContext ctx) throws OrmException,
public boolean updateChange(ChangeContext ctx) throws OrmException,
ResourceConflictException {
caller = ctx.getUser().asIdentifiedUser();
change = ctx.getChange();
@@ -125,6 +125,7 @@ public class Restore implements RestModifyView<ChangeResource, RestoreInput>,
message = newMessage(ctx.getDb());
cmUtil.addChangeMessage(ctx.getDb(), update, message);
return true;
}
private ChangeMessage newMessage(ReviewDb db) throws OrmException {

5
gerrit-server/src/main/java/com/google/gerrit/server/change/SetHashtagsOp.java
View File

@@ -73,12 +73,12 @@ public class SetHashtagsOp extends BatchUpdate.Op {
}
@Override
public void updateChange(ChangeContext ctx)
public boolean updateChange(ChangeContext ctx)
throws AuthException, BadRequestException, OrmException, IOException {
if (input == null
|| (input.add == null && input.remove == null)) {
updatedHashtags = ImmutableSortedSet.of();
return;
return false;
}
if (!ctx.getControl().canEditHashtags()) {
throw new AuthException("Editing hashtags not permitted");
@@ -112,6 +112,7 @@ public class SetHashtagsOp extends BatchUpdate.Op {
}
updatedHashtags = ImmutableSortedSet.copyOf(updated);
return true;
}
@Override

38
gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java
View File

@@ -221,7 +221,8 @@ public class BatchUpdate implements AutoCloseable {
}
@SuppressWarnings("unused")
public void updateChange(ChangeContext ctx) throws Exception {
public boolean updateChange(ChangeContext ctx) throws Exception {
return false;
}
// TODO(dborowitz): Support async operations?
@@ -542,10 +543,11 @@ public class BatchUpdate implements AutoCloseable {
Change.Id id = e.getKey();
db.changes().beginTransaction(id);
ChangeContext ctx;
boolean dirty = false;
try {
ctx = newChangeContext(id);
for (Op op : e.getValue()) {
op.updateChange(ctx);
dirty |= op.updateChange(ctx);
}
Iterable<Change> changes = Collections.singleton(ctx.getChange());
if (newChanges.containsKey(id)) {
@@ -555,26 +557,30 @@ public class BatchUpdate implements AutoCloseable {
} else if (ctx.deleted) {
db.changes().delete(changes);
}
db.commit();
if (dirty) {
db.commit();
}
} finally {
db.rollback();
}
BatchMetaDataUpdate bmdu = null;
for (ChangeUpdate u : ctx.updates.values()) {
if (bmdu == null) {
bmdu = u.openUpdate();
if (dirty) {
BatchMetaDataUpdate bmdu = null;
for (ChangeUpdate u : ctx.updates.values()) {
if (bmdu == null) {
bmdu = u.openUpdate();
}
u.writeCommit(bmdu);
}
if (bmdu != null) {
bmdu.commit();
}
u.writeCommit(bmdu);
}
if (bmdu != null) {
bmdu.commit();
}
if (ctx.deleted) {
indexFutures.add(indexer.deleteAsync(id));
} else {
indexFutures.add(indexer.indexAsync(id));
if (ctx.deleted) {
indexFutures.add(indexer.deleteAsync(id));
} else {
indexFutures.add(indexer.indexAsync(id));
}
}
}
} catch (Exception e) {

3
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@@ -1810,8 +1810,9 @@ public class ReceiveCommits {
changeId,
new BatchUpdate.Op() {
@Override
public void updateChange(ChangeContext ctx) throws Exception {
public boolean updateChange(ChangeContext ctx) {
ctx.getUpdate(psId).setTopic(magicBranch.topic);
return true;
}
});
}

5
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
View File

@@ -162,11 +162,11 @@ public class CherryPick extends SubmitStrategy {
}
@Override
public void updateChange(ChangeContext ctx) throws OrmException,
public boolean updateChange(ChangeContext ctx) throws OrmException,
NoSuchChangeException {
if (newCommit == null) {
// Merge conflict; don't update change.
return;
return false;
}
PatchSet prevPs = args.psUtil.current(ctx.getDb(), ctx.getNotes());
@@ -185,6 +185,7 @@ public class CherryPick extends SubmitStrategy {
newCommit.setControl(
args.changeControlFactory.controlFor(toMerge.change(), args.caller));
return true;
}
}

5
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
View File

@@ -170,11 +170,11 @@ public class RebaseIfNecessary extends SubmitStrategy {
}
@Override
public void updateChange(ChangeContext ctx) throws NoSuchChangeException,
public boolean updateChange(ChangeContext ctx) throws NoSuchChangeException,
InvalidChangeOperationException, OrmException, IOException {
if (rebaseOp == null) {
// Took the fast-forward option, nothing to do.
return;
return false;
}
rebaseOp.updateChange(ctx);
@@ -191,6 +191,7 @@ public class RebaseIfNecessary extends SubmitStrategy {
newPatchSetId));
newCommit.setControl(
args.changeControlFactory.controlFor(toMerge.change(), args.caller));
return true;
}
@Override