opendev/gerrit
Code Issues Proposed changes

Make NoteDbUpdateManager AutoCloseable

Browse Source 
OpenRepo instances can be auto-opened if the caller didn't call
set{Change,AllUsers}Repo. These instances were closed in a finally
block within execute() but not from stage().

Make the whole class AutoCloseable so users know they have to close it
when they're finished.

Change-Id: I247d3749008029e9969b0e10fa7f8c562cb09d7f
This commit is contained in:
Dave Borowitz
2016-07-07 13:05:05 -04:00
parent 31b09e354a
commit 17699c8bf0
7 changed files with 121 additions and 89 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -792,9 +792,10 @@ public class BatchUpdate implements AutoCloseable {
private void call(ReviewDb db, Repository repo, RevWalk rw) private void call(ReviewDb db, Repository repo, RevWalk rw)
throws Exception { throws Exception {
@SuppressWarnings("resource") // Not always opened.
NoteDbUpdateManager updateManager = null;
try { try {
ChangeContext ctx; ChangeContext ctx;
NoteDbUpdateManager updateManager = null;
db.changes().beginTransaction(id); db.changes().beginTransaction(id);
try { try {
ctx = newChangeContext(db, repo, rw, id); ctx = newChangeContext(db, repo, rw, id);
@@ -848,6 +849,10 @@ public class BatchUpdate implements AutoCloseable {
} catch (Exception e) { } catch (Exception e) {
Throwables.propagateIfPossible(e, RestApiException.class); Throwables.propagateIfPossible(e, RestApiException.class);
throw new UpdateException(e); throw new UpdateException(e);
} finally {
if (updateManager != null) {
updateManager.close();
}
} }
} }

54
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
View File

@@ -552,32 +552,34 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
Change.Id cid = getChangeId(); Change.Id cid = getChangeId();
ReviewDb db = args.db.get(); ReviewDb db = args.db.get();
ChangeRebuilder rebuilder = args.rebuilder.get(); ChangeRebuilder rebuilder = args.rebuilder.get();
NoteDbUpdateManager manager = rebuilder.stage(db, cid); NoteDbUpdateManager.Result r;
if (manager == null) { try (NoteDbUpdateManager manager = rebuilder.stage(db, cid)) {
return super.openHandle(repo, oldId); // May be null in tests. if (manager == null) {
} return super.openHandle(repo, oldId); // May be null in tests.
NoteDbUpdateManager.Result r = manager.stageAndApplyDelta(change); }
try { r = manager.stageAndApplyDelta(change);
rebuilder.execute(db, cid, manager); try {
repo.scanForRepoChanges(); rebuilder.execute(db, cid, manager);
} catch (OrmException | IOException e) { repo.scanForRepoChanges();
// Rebuilding failed. Most likely cause is contention on one or more } catch (OrmException | IOException e) {
// change refs; there are other types of errors that can happen during // Rebuilding failed. Most likely cause is contention on one or more
// rebuilding, but generally speaking they should happen during stage(), // change refs; there are other types of errors that can happen during
// not execute(). Assume that some other worker is going to successfully // rebuilding, but generally speaking they should happen during stage(),
// store the rebuilt state, which is deterministic given an input // not execute(). Assume that some other worker is going to successfully
// ChangeBundle. // store the rebuilt state, which is deterministic given an input
// // ChangeBundle.
// Parse notes from the staged result so we can return something useful //
// to the caller instead of throwing. // Parse notes from the staged result so we can return something useful
args.metrics.autoRebuildFailureCount.increment(CHANGES); // to the caller instead of throwing.
rebuildResult = checkNotNull(r); args.metrics.autoRebuildFailureCount.increment(CHANGES);
checkNotNull(r.newState()); rebuildResult = checkNotNull(r);
checkNotNull(r.staged()); checkNotNull(r.newState());
return LoadHandle.create( checkNotNull(r.staged());
ChangeNotesCommit.newStagedRevWalk( return LoadHandle.create(
repo, r.staged().changeObjects()), ChangeNotesCommit.newStagedRevWalk(
r.newState().getChangeMetaId()); repo, r.staged().changeObjects()),
r.newState().getChangeMetaId());
}
} }
return LoadHandle.create( return LoadHandle.create(
ChangeNotesCommit.newRevWalk(repo), r.newState().getChangeMetaId()); ChangeNotesCommit.newRevWalk(repo), r.newState().getChangeMetaId());

13
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java
View File

@@ -161,10 +161,11 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
if (change == null) { if (change == null) {
throw new NoSuchChangeException(changeId); throw new NoSuchChangeException(changeId);
} }
NoteDbUpdateManager manager = try (NoteDbUpdateManager manager =
updateManagerFactory.create(change.getProject()); updateManagerFactory.create(change.getProject())) {
buildUpdates(manager, ChangeBundle.fromReviewDb(db, changeId)); buildUpdates(manager, ChangeBundle.fromReviewDb(db, changeId));
return execute(db, changeId, manager); return execute(db, changeId, manager);
}
} }
private static class AbortUpdateException extends OrmRuntimeException { private static class AbortUpdateException extends OrmRuntimeException {
@@ -246,10 +247,10 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
checkArgument(allChanges.containsKey(project)); checkArgument(allChanges.containsKey(project));
boolean ok = true; boolean ok = true;
ProgressMonitor pm = new TextProgressMonitor(new PrintWriter(System.out)); ProgressMonitor pm = new TextProgressMonitor(new PrintWriter(System.out));
NoteDbUpdateManager manager = updateManagerFactory.create(project);
pm.beginTask( pm.beginTask(
FormatUtil.elide(project.get(), 50), allChanges.get(project).size()); FormatUtil.elide(project.get(), 50), allChanges.get(project).size());
try (ObjectInserter allUsersInserter = allUsersRepo.newObjectInserter(); try (NoteDbUpdateManager manager = updateManagerFactory.create(project);
ObjectInserter allUsersInserter = allUsersRepo.newObjectInserter();
RevWalk allUsersRw = new RevWalk(allUsersInserter.newReader())) { RevWalk allUsersRw = new RevWalk(allUsersInserter.newReader())) {
manager.setAllUsersRepo(allUsersRepo, allUsersRw, allUsersInserter, manager.setAllUsersRepo(allUsersRepo, allUsersRw, allUsersInserter,
new ChainedReceiveCommands(allUsersRepo)); new ChainedReceiveCommands(allUsersRepo));

11
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java
View File

@@ -218,11 +218,12 @@ public class ChangeUpdate extends AbstractChangeUpdate {
} }
public ObjectId commit() throws IOException, OrmException { public ObjectId commit() throws IOException, OrmException {
NoteDbUpdateManager updateManager = try (NoteDbUpdateManager updateManager =
updateManagerFactory.create(getProjectName()); updateManagerFactory.create(getProjectName())) {
updateManager.add(this); updateManager.add(this);
updateManager.stageAndApplyDelta(getChange()); updateManager.stageAndApplyDelta(getChange());
updateManager.execute(); updateManager.execute();
}
return getResult(); return getResult();
} }

34
gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
View File

@@ -189,22 +189,24 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
Change.Id cid = getChangeId(); Change.Id cid = getChangeId();
ReviewDb db = args.db.get(); ReviewDb db = args.db.get();
ChangeRebuilder rebuilder = args.rebuilder.get(); ChangeRebuilder rebuilder = args.rebuilder.get();
NoteDbUpdateManager manager = rebuilder.stage(db, cid); NoteDbUpdateManager.Result r;
if (manager == null) { try (NoteDbUpdateManager manager = rebuilder.stage(db, cid)) {
return super.openHandle(repo); // May be null in tests. if (manager == null) {
} return super.openHandle(repo); // May be null in tests.
NoteDbUpdateManager.Result r = manager.stageAndApplyDelta(change); }
try { r = manager.stageAndApplyDelta(change);
rebuilder.execute(db, cid, manager); try {
repo.scanForRepoChanges(); rebuilder.execute(db, cid, manager);
} catch (OrmException | IOException e) { repo.scanForRepoChanges();
// See ChangeNotes#rebuildAndOpen. } catch (OrmException | IOException e) {
args.metrics.autoRebuildFailureCount.increment(CHANGES); // See ChangeNotes#rebuildAndOpen.
checkNotNull(r.staged()); args.metrics.autoRebuildFailureCount.increment(CHANGES);
return LoadHandle.create( checkNotNull(r.staged());
ChangeNotesCommit.newStagedRevWalk( return LoadHandle.create(
repo, r.staged().allUsersObjects()), ChangeNotesCommit.newStagedRevWalk(
draftsId(r)); repo, r.staged().allUsersObjects()),
draftsId(r));
}
} }
return LoadHandle.create(ChangeNotesCommit.newRevWalk(repo), draftsId(r)); return LoadHandle.create(ChangeNotesCommit.newRevWalk(repo), draftsId(r));
} catch (NoSuchChangeException e) { } catch (NoSuchChangeException e) {

26
gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
View File

@@ -69,7 +69,7 @@ import java.util.Set;
* To see the state that would be applied prior to executing the full sequence * To see the state that would be applied prior to executing the full sequence
* of updates, use {@link #stage()}. * of updates, use {@link #stage()}.
*/ */
public class NoteDbUpdateManager { public class NoteDbUpdateManager implements AutoCloseable {
public static String CHANGES_READ_ONLY = "NoteDb changes are read-only"; public static String CHANGES_READ_ONLY = "NoteDb changes are read-only";
public interface Factory { public interface Factory {
@@ -202,6 +202,23 @@ public class NoteDbUpdateManager {
toDelete = new HashSet<>(); toDelete = new HashSet<>();
} }
@Override
public void close() {
try {
if (allUsersRepo != null) {
OpenRepo r = allUsersRepo;
allUsersRepo = null;
r.close();
}
} finally {
if (changeRepo != null) {
OpenRepo r = changeRepo;
changeRepo = null;
r.close();
}
}
}
public NoteDbUpdateManager setChangeRepo(Repository repo, RevWalk rw, public NoteDbUpdateManager setChangeRepo(Repository repo, RevWalk rw,
@Nullable ObjectInserter ins, ChainedReceiveCommands cmds) { @Nullable ObjectInserter ins, ChainedReceiveCommands cmds) {
checkState(changeRepo == null, "change repo already initialized"); checkState(changeRepo == null, "change repo already initialized");
@@ -404,12 +421,7 @@ public class NoteDbUpdateManager {
execute(changeRepo); execute(changeRepo);
execute(allUsersRepo); execute(allUsersRepo);
} finally { } finally {
if (allUsersRepo != null) { close();
allUsersRepo.close();
}
if (changeRepo != null) {
changeRepo.close();
}
} }
} }

65
gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
View File

@@ -1008,10 +1008,12 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
ChangeUpdate update2 = newUpdate(c, otherUser); ChangeUpdate update2 = newUpdate(c, otherUser);
update2.putApproval("Code-Review", (short) 2); update2.putApproval("Code-Review", (short) 2);
NoteDbUpdateManager updateManager = updateManagerFactory.create(project); try (NoteDbUpdateManager updateManager =
updateManager.add(update1); updateManagerFactory.create(project)) {
updateManager.add(update2); updateManager.add(update1);
updateManager.execute(); updateManager.add(update2);
updateManager.execute();
}
ChangeNotes notes = newNotes(c); ChangeNotes notes = newNotes(c);
List<PatchSetApproval> psas = List<PatchSetApproval> psas =
@@ -1038,20 +1040,22 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
CommentRange range1 = new CommentRange(1, 1, 2, 1); CommentRange range1 = new CommentRange(1, 1, 2, 1);
Timestamp time1 = TimeUtil.nowTs(); Timestamp time1 = TimeUtil.nowTs();
PatchSet.Id psId = c.currentPatchSetId(); PatchSet.Id psId = c.currentPatchSetId();
NoteDbUpdateManager updateManager = updateManagerFactory.create(project);
PatchLineComment comment1 = newPublishedComment(psId, "file1",
uuid1, range1, range1.getEndLine(), otherUser, null, time1, message1,
(short) 0, "abcd1234abcd1234abcd1234abcd1234abcd1234");
update1.setPatchSetId(psId);
update1.putComment(comment1);
updateManager.add(update1);
ChangeUpdate update2 = newUpdate(c, otherUser);
update2.putApproval("Code-Review", (short) 2);
updateManager.add(update2);
RevCommit tipCommit; RevCommit tipCommit;
updateManager.execute(); try (NoteDbUpdateManager updateManager =
updateManagerFactory.create(project)) {
PatchLineComment comment1 = newPublishedComment(psId, "file1",
uuid1, range1, range1.getEndLine(), otherUser, null, time1, message1,
(short) 0, "abcd1234abcd1234abcd1234abcd1234abcd1234");
update1.setPatchSetId(psId);
update1.putComment(comment1);
updateManager.add(update1);
ChangeUpdate update2 = newUpdate(c, otherUser);
update2.putApproval("Code-Review", (short) 2);
updateManager.add(update2);
updateManager.execute();
}
ChangeNotes notes = newNotes(c); ChangeNotes notes = newNotes(c);
ObjectId tip = notes.getRevision(); ObjectId tip = notes.getRevision();
@@ -1094,10 +1098,12 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
Ref initial2 = repo.exactRef(update2.getRefName()); Ref initial2 = repo.exactRef(update2.getRefName());
assertThat(initial2).isNotNull(); assertThat(initial2).isNotNull();
NoteDbUpdateManager updateManager = updateManagerFactory.create(project); try (NoteDbUpdateManager updateManager =
updateManager.add(update1); updateManagerFactory.create(project)) {
updateManager.add(update2); updateManager.add(update1);
updateManager.execute(); updateManager.add(update2);
updateManager.execute();
}
Ref ref1 = repo.exactRef(update1.getRefName()); Ref ref1 = repo.exactRef(update1.getRefName());
assertThat(ref1.getObjectId()).isEqualTo(update1.getResult()); assertThat(ref1.getObjectId()).isEqualTo(update1.getResult());
@@ -2222,9 +2228,11 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
newUpdate(c, otherUser).createDraftUpdateIfNull(); newUpdate(c, otherUser).createDraftUpdateIfNull();
comment2.setStatus(Status.DRAFT); comment2.setStatus(Status.DRAFT);
draftUpdate.putComment(comment2); draftUpdate.putComment(comment2);
NoteDbUpdateManager manager = updateManagerFactory.create(c.getProject()); try (NoteDbUpdateManager manager =
manager.add(draftUpdate); updateManagerFactory.create(c.getProject())) {
manager.execute(); manager.add(draftUpdate);
manager.execute();
}
// Looking at drafts directly shows the zombie comment. // Looking at drafts directly shows the zombie comment.
DraftCommentNotes draftNotes = draftNotesFactory.create(c, otherUserId); DraftCommentNotes draftNotes = draftNotesFactory.create(c, otherUserId);
@@ -2270,10 +2278,11 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
Status.PUBLISHED); Status.PUBLISHED);
update2.putComment(comment2); update2.putComment(comment2);
NoteDbUpdateManager manager = updateManagerFactory.create(project); try (NoteDbUpdateManager manager = updateManagerFactory.create(project)) {
manager.add(update1); manager.add(update1);
manager.add(update2); manager.add(update2);
manager.execute(); manager.execute();
}
ChangeNotes notes = newNotes(c); ChangeNotes notes = newNotes(c);
List<PatchLineComment> comments = notes.getComments().get(new RevId(rev)); List<PatchLineComment> comments = notes.getComments().get(new RevId(rev));