opendev/gerrit
Code Issues Proposed changes

NotesBranchUtil: Don't retry automatically

Browse Source 
The retry mechanism in NotesBranchUtil is inflexible, and only retries a
fixed number of times on lock failure, with a fixed wait in between, up
to 250ms total. Now that we have RetryHelper available globally, make
use of that instead. This allows us to eliminate a lot of code from
NotesBranchUtil, which is a good thing, since this extra code also has
some bugs. For example, it doesn't handle all possible
RefUpdate.Results.

This is a philosophical difference in how to approach retries, which
requires fixing downstream code, including in the reviewnotes plugin.
RetryHelper is designed to be run at the highest level, completely
reopening the repository, to ensure we get the latest ref state and
start with a fresh inserter.

Since we now expect downstream plugins to use RetryHelper, export
guava-retrying from the plugin API.

Change-Id: I8a93e20f811a7c67c5cfd9cdf630afac7327c64f
This commit is contained in:
Dave Borowitz 2017-06-01 17:37:36 -04:00
parent b58b8489a4
commit 36b9d3e991
6 changed files with 42 additions and 84 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
gerrit-plugin-api/BUILD
View File

@ -43,6 +43,7 @@ EXPORTS = [
"//lib:args4j",
"//lib:blame-cache",
"//lib:guava",
"//lib:guava-retrying",
"//lib:gson",
"//lib:gwtorm",
"//lib:icu4j",

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

@ -30,7 +30,6 @@ import java.io.IOException;
import java.util.Date;
import java.util.List;
import java.util.TimeZone;
import org.eclipse.jgit.api.errors.ConcurrentRefUpdateException;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.Constants;
@ -87,7 +86,7 @@ public class BanCommit {
public BanCommitResult ban(
ProjectControl projectControl, List<ObjectId> commitsToBan, String reason)
throws PermissionDeniedException, IOException, ConcurrentRefUpdateException {
throws PermissionDeniedException, LockFailureException, IOException {
if (!projectControl.isOwner()) {
throw new PermissionDeniedException("Not project owner: not permitted to ban commits");
}

95
gerrit-server/src/main/java/com/google/gerrit/server/git/NotesBranchUtil.java
View File

@ -14,32 +14,29 @@
package com.google.gerrit.server.git;
import static com.google.common.base.MoreObjects.firstNonNull;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.update.RefUpdateUtil;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
import org.eclipse.jgit.api.errors.ConcurrentRefUpdateException;
import org.eclipse.jgit.errors.CorruptObjectException;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.RefUpdate.Result;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.merge.MergeStrategy;
import org.eclipse.jgit.notes.Note;
import org.eclipse.jgit.notes.NoteMap;
import org.eclipse.jgit.notes.NoteMapMerger;
import org.eclipse.jgit.notes.NoteMerger;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.ReceiveCommand;
/** A utility class for updating a notes branch with automatic merge of note trees. */
public class NotesBranchUtil {
@ -47,9 +44,6 @@ public class NotesBranchUtil {
NotesBranchUtil create(Project.NameKey project, Repository db, ObjectInserter inserter);
}
private static final int MAX_LOCK_FAILURE_CALLS = 10;
private static final int SLEEP_ON_LOCK_FAILURE_MS = 25;
private final PersonIdent gerritIdent;
private final GitReferenceUpdated gitRefUpdated;
private final Project.NameKey project;
@ -86,16 +80,20 @@ public class NotesBranchUtil {
* Create a new commit in the {@code notesBranch} by updating existing or creating new notes from
* the {@code notes} map.
*
* <p>Does not retry in the case of lock failure; callers may use {@link
* com.google.gerrit.server.update.RetryHelper}.
*
* @param notes map of notes
* @param notesBranch notes branch to update
* @param commitAuthor author of the commit in the notes branch
* @param commitMessage for the commit in the notes branch
* @throws IOException
* @throws ConcurrentRefUpdateException
* @throws LockFailureException if committing the notes failed due to a lock failure on the notes
* branch
* @throws IOException if committing the notes failed for any other reason
*/
public final void commitAllNotes(
NoteMap notes, String notesBranch, PersonIdent commitAuthor, String commitMessage)
throws IOException, ConcurrentRefUpdateException {
throws IOException {
this.overwrite = true;
commitNotes(notes, notesBranch, commitAuthor, commitMessage);
}
@ -105,17 +103,21 @@ public class NotesBranchUtil {
* {@code notes} map. The notes from the {@code notes} map which already exist in the note-tree of
* the tip of the {@code notesBranch} will not be updated.
*
* <p>Does not retry in the case of lock failure; callers may use {@link
* com.google.gerrit.server.update.RetryHelper}.
*
* @param notes map of notes
* @param notesBranch notes branch to update
* @param commitAuthor author of the commit in the notes branch
* @param commitMessage for the commit in the notes branch
* @return map with those notes from the {@code notes} that were newly created
* @throws IOException
* @throws ConcurrentRefUpdateException
* @throws LockFailureException if committing the notes failed due to a lock failure on the notes
* branch
* @throws IOException if committing the notes failed for any other reason
*/
public final NoteMap commitNewNotes(
NoteMap notes, String notesBranch, PersonIdent commitAuthor, String commitMessage)
throws IOException, ConcurrentRefUpdateException {
throws IOException {
this.overwrite = false;
commitNotes(notes, notesBranch, commitAuthor, commitMessage);
NoteMap newlyCreated = NoteMap.newEmptyMap();
@ -129,7 +131,7 @@ public class NotesBranchUtil {
private void commitNotes(
NoteMap notes, String notesBranch, PersonIdent commitAuthor, String commitMessage)
throws IOException, ConcurrentRefUpdateException {
throws LockFailureException, IOException {
try {
revWalk = new RevWalk(db);
reader = db.newObjectReader();
@ -209,61 +211,16 @@ public class NotesBranchUtil {
return revWalk.parseCommit(commitId);
}
private void updateRef(String notesBranch)
throws IOException, MissingObjectException, IncorrectObjectTypeException,
CorruptObjectException, ConcurrentRefUpdateException {
private void updateRef(String notesBranch) throws LockFailureException, IOException {
if (baseCommit != null && oursCommit.getTree().equals(baseCommit.getTree())) {
// If the trees are identical, there is no change in the notes.
// Avoid saving this commit as it has no new information.
return;
}
int remainingLockFailureCalls = MAX_LOCK_FAILURE_CALLS;
RefUpdate refUpdate = createRefUpdate(notesBranch, oursCommit, baseCommit);
for (; ; ) {
Result result = refUpdate.update();
if (result == Result.LOCK_FAILURE) {
if (--remainingLockFailureCalls > 0) {
try {
Thread.sleep(SLEEP_ON_LOCK_FAILURE_MS);
} catch (InterruptedException e) {
// ignore
}
} else {
throw new ConcurrentRefUpdateException(
"Failed to lock the ref: " + notesBranch, refUpdate.getRef(), result);
}
} else if (result == Result.REJECTED) {
RevCommit theirsCommit = revWalk.parseCommit(refUpdate.getOldObjectId());
NoteMap theirs = NoteMap.read(revWalk.getObjectReader(), theirsCommit);
NoteMapMerger merger = new NoteMapMerger(db, getNoteMerger(), MergeStrategy.RESOLVE);
NoteMap merged = merger.merge(base, ours, theirs);
RevCommit mergeCommit =
createCommit(merged, gerritIdent, "Merged note commits\n", theirsCommit, oursCommit);
refUpdate = createRefUpdate(notesBranch, mergeCommit, theirsCommit);
remainingLockFailureCalls = MAX_LOCK_FAILURE_CALLS;
} else if (result == Result.IO_FAILURE) {
throw new IOException("Couldn't update " + notesBranch + ". " + result.name());
} else {
gitRefUpdated.fire(project, refUpdate, null);
break;
}
}
}
private RefUpdate createRefUpdate(
String notesBranch, ObjectId newObjectId, ObjectId expectedOldObjectId) throws IOException {
RefUpdate refUpdate = db.updateRef(notesBranch);
refUpdate.setNewObjectId(newObjectId);
if (expectedOldObjectId == null) {
refUpdate.setExpectedOldObjectId(ObjectId.zeroId());
} else {
refUpdate.setExpectedOldObjectId(expectedOldObjectId);
}
return refUpdate;
BatchRefUpdate bru = db.getRefDatabase().newBatchUpdate();
bru.addCommand(
new ReceiveCommand(firstNonNull(baseCommit, ObjectId.zeroId()), oursCommit, notesBranch));
RefUpdateUtil.executeChecked(bru, revWalk);
gitRefUpdated.fire(project, bru, null);
}
}

21
gerrit-server/src/main/java/com/google/gerrit/server/project/BanCommit.java
View File

@ -17,21 +17,24 @@ package com.google.gerrit.server.project;
import com.google.common.collect.Lists;
import com.google.gerrit.common.errors.PermissionDeniedException;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.server.git.BanCommitResult;
import com.google.gerrit.server.project.BanCommit.BanResultInfo;
import com.google.gerrit.server.project.BanCommit.Input;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.RetryingRestModifyView;
import com.google.gerrit.server.update.UpdateException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import org.eclipse.jgit.api.errors.ConcurrentRefUpdateException;
import org.eclipse.jgit.lib.ObjectId;
@Singleton
public class BanCommit implements RestModifyView<ProjectResource, Input> {
public class BanCommit extends RetryingRestModifyView<ProjectResource, Input, BanResultInfo> {
public static class Input {
public List<String> commits;
public String reason;
@ -50,13 +53,15 @@ public class BanCommit implements RestModifyView<ProjectResource, Input> {
private final com.google.gerrit.server.git.BanCommit banCommit;
@Inject
BanCommit(com.google.gerrit.server.git.BanCommit banCommit) {
BanCommit(RetryHelper retryHelper, com.google.gerrit.server.git.BanCommit banCommit) {
super(retryHelper);
this.banCommit = banCommit;
}
@Override
public BanResultInfo apply(ProjectResource rsrc, Input input)
throws UnprocessableEntityException, AuthException, ResourceConflictException, IOException {
protected BanResultInfo applyImpl(
BatchUpdate.Factory updateFactory, ProjectResource rsrc, Input input)
throws RestApiException, UpdateException, IOException {
BanResultInfo r = new BanResultInfo();
if (input != null && input.commits != null && !input.commits.isEmpty()) {
List<ObjectId> commitsToBan = new ArrayList<>(input.commits.size());
@ -75,8 +80,6 @@ public class BanCommit implements RestModifyView<ProjectResource, Input> {
r.ignored = transformCommits(result.getIgnoredObjectIds());
} catch (PermissionDeniedException e) {
throw new AuthException(e.getMessage());
} catch (ConcurrentRefUpdateException e) {
throw new ResourceConflictException(e.getMessage(), e);
}
}
return r;

4
gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/BanCommitCommand.java
View File

@ -18,7 +18,6 @@ import static com.google.gerrit.sshd.CommandMetaData.Mode.MASTER_OR_SLAVE;
import com.google.common.base.Joiner;
import com.google.common.collect.Lists;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.project.BanCommit;
import com.google.gerrit.server.project.BanCommit.BanResultInfo;
import com.google.gerrit.server.project.ProjectControl;
@ -26,7 +25,6 @@ import com.google.gerrit.server.project.ProjectResource;
import com.google.gerrit.sshd.CommandMetaData;
import com.google.gerrit.sshd.SshCommand;
import com.google.inject.Inject;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import org.eclipse.jgit.lib.ObjectId;
@ -77,7 +75,7 @@ public class BanCommitCommand extends SshCommand {
printCommits(r.newlyBanned, "The following commits were banned");
printCommits(r.alreadyBanned, "The following commits were already banned");
printCommits(r.ignored, "The following ids do not represent commits and were ignored");
} catch (RestApiException | IOException e) {
} catch (Exception e) {
throw die(e);
}
}

2
plugins/reviewnotes

@ -1 +1 @@
Subproject commit 1a64a6f6407df31c06350a5c6e9266e9ba0cf72a
Subproject commit f0930e9dfc56644105cb21e7246096097eeaa385