Load an arbitrary version of change notes
The SHA1 of the change notes to load may be already
known to the caller of ChangeNotes.create() method hence
it can be passed as a version parameter to avoid an unneeded
ref lookup operation.
When the version passed isn't known, it can be given as
null parameter causing the lookup of the latest /meta ref
SHA1 from the repository's ref-database.
Allow skipping potentially hundreds of thousands of refs lookups
for large mono-repos when filtering refs from the repository based
on the change status.
Also document the different variants of ChangeNotes.create()
methods, highlighting their specific purpose and use-case.
This is a partial cherry-pick of Ia54acb37e1187.
Release-Notes: skip
Forward-Compatible: checked
Original-Author: Han-Wen Nienhuys <hanwen@google.com>
Change-Id: I359578a05ecad595fb64909398ea3fd069ff1a5f
diff --git a/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java b/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
index 72f1ba6..1dfb82f 100644
--- a/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
@@ -122,7 +122,11 @@
private boolean loaded;
AbstractChangeNotes(
- Args args, Change.Id changeId, @Nullable PrimaryStorage primaryStorage, boolean autoRebuild) {
+ Args args,
+ Change.Id changeId,
+ @Nullable PrimaryStorage primaryStorage,
+ boolean autoRebuild,
+ @Nullable ObjectId metaSha1) {
this.args = requireNonNull(args);
this.changeId = requireNonNull(changeId);
this.primaryStorage = primaryStorage;
@@ -130,6 +134,7 @@
primaryStorage == PrimaryStorage.REVIEW_DB
&& !args.migration.disableChangeReviewDb()
&& autoRebuild;
+ this.revision = metaSha1;
}
public Change.Id getChangeId() {
@@ -145,6 +150,18 @@
if (loaded) {
return self();
}
+ try (Repository repo = args.repoManager.openRepository(getProjectName())) {
+ load(repo);
+ return self();
+ } catch (IOException e) {
+ throw new OrmException(e);
+ }
+ }
+
+ public T load(Repository repo) throws OrmException {
+ if (loaded) {
+ return self();
+ }
boolean read = args.migration.readChanges();
if (!read && primaryStorage == PrimaryStorage.NOTE_DB) {
@@ -162,10 +179,9 @@
throw new OrmException("Reading from NoteDb is disabled");
}
try (Timer1.Context timer = args.metrics.readLatency.start(CHANGES);
- Repository repo = args.repoManager.openRepository(getProjectName());
// Call openHandle even if reading is disabled, to trigger
// auto-rebuilding before this object may get passed to a ChangeUpdate.
- LoadHandle handle = openHandle(repo)) {
+ LoadHandle handle = openHandle(repo, revision)) {
if (read) {
revision = handle.id();
onLoad(handle);
@@ -195,10 +211,18 @@
* @throws IOException a repo-level error occurred.
*/
protected LoadHandle openHandle(Repository repo) throws NoSuchChangeException, IOException {
- return openHandle(repo, readRef(repo));
+ return openHandleFromRevisionId(repo, readRef(repo));
}
- protected LoadHandle openHandle(Repository repo, ObjectId id) {
+ protected LoadHandle openHandle(Repository repo, @Nullable ObjectId revisionId)
+ throws NoSuchChangeException, IOException {
+ if (revisionId == null) {
+ revisionId = readRef(repo);
+ }
+ return openHandleFromRevisionId(repo, revisionId);
+ }
+
+ protected final LoadHandle openHandleFromRevisionId(Repository repo, ObjectId id) {
return LoadHandle.create(ChangeNotesCommit.newRevWalk(repo), id);
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index e1f415d..e2ddd87 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -124,7 +124,12 @@
return createChecked(db, c.getProject(), c.getId());
}
- public ChangeNotes createChecked(ReviewDb db, Project.NameKey project, Change.Id changeId)
+ public ChangeNotes createChecked(
+ ReviewDb db,
+ @Nullable Repository repo,
+ Project.NameKey project,
+ Change.Id changeId,
+ @Nullable ObjectId metaRevId)
throws OrmException {
Change change = readOneReviewDbChange(db, changeId);
if (change == null) {
@@ -137,7 +142,22 @@
} else if (!change.getProject().equals(project)) {
throw new NoSuchChangeException(changeId);
}
- return new ChangeNotes(args, change).load();
+ ChangeNotes cn = new ChangeNotes(args, change, null, metaRevId);
+ if (repo == null) {
+ return cn.load();
+ }
+ return cn.load(repo);
+ }
+
+ public ChangeNotes createChecked(
+ ReviewDb db, Project.NameKey project, Change.Id changeId, @Nullable ObjectId metaRevId)
+ throws OrmException {
+ return createChecked(db, null, project, changeId, metaRevId);
+ }
+
+ public ChangeNotes createChecked(ReviewDb db, Project.NameKey project, Change.Id changeId)
+ throws OrmException {
+ return createChecked(db, null, project, changeId, null);
}
public ChangeNotes createChecked(Change.Id changeId) throws OrmException {
@@ -188,7 +208,8 @@
public ChangeNotes createWithAutoRebuildingDisabled(
ReviewDb db, Project.NameKey project, Change.Id changeId) throws OrmException {
- return new ChangeNotes(args, loadChangeFromDb(db, project, changeId), true, false).load();
+ return new ChangeNotes(args, loadChangeFromDb(db, project, changeId), true, false, null, null)
+ .load();
}
/**
@@ -204,11 +225,11 @@
public ChangeNotes createForBatchUpdate(Change change, boolean shouldExist)
throws OrmException {
- return new ChangeNotes(args, change, shouldExist, false).load();
+ return new ChangeNotes(args, change, shouldExist, false, null, null).load();
}
public ChangeNotes createWithAutoRebuildingDisabled(Change change) throws OrmException {
- return new ChangeNotes(args, change, true, false).load();
+ return new ChangeNotes(args, change, true, false, null, null).load();
}
// TODO(ekempin): Remove when database backend is deleted
@@ -476,6 +497,7 @@
}
private final boolean shouldExist;
+ private final RefCache refs;
private Change change;
private ChangeNotesState state;
@@ -496,13 +518,30 @@
@VisibleForTesting
public ChangeNotes(Args args, Change change) {
- this(args, change, true, true);
+ this(args, change, true, true, null, null);
}
- private ChangeNotes(Args args, Change change, boolean shouldExist, boolean autoRebuild) {
- super(args, change.getId(), PrimaryStorage.of(change), autoRebuild);
+ private ChangeNotes(Args args, Change change, @Nullable RefCache refs) {
+ this(args, change, true, true, refs, null);
+ }
+
+ @VisibleForTesting
+ public ChangeNotes(
+ Args args, Change change, @Nullable RefCache refs, @Nullable ObjectId metaSha1) {
+ this(args, change, true, true, refs, metaSha1);
+ }
+
+ private ChangeNotes(
+ Args args,
+ Change change,
+ boolean shouldExist,
+ boolean autoRebuild,
+ @Nullable RefCache refs,
+ @Nullable ObjectId metaSha1) {
+ super(args, change.getId(), PrimaryStorage.of(change), autoRebuild, metaSha1);
this.change = new Change(change);
this.shouldExist = shouldExist;
+ this.refs = refs;
}
public Change getChange() {
@@ -728,6 +767,13 @@
@Override
protected LoadHandle openHandle(Repository repo) throws NoSuchChangeException, IOException {
+ return openHandle(repo, null);
+ }
+
+ @Override
+ protected LoadHandle openHandle(Repository repo, @Nullable ObjectId revisionId)
+ throws NoSuchChangeException, IOException {
+ ObjectId id;
if (autoRebuild) {
NoteDbChangeState state = NoteDbChangeState.parse(change);
if (args.migration.disableChangeReviewDb()) {
@@ -736,7 +782,7 @@
"shouldn't have null NoteDbChangeState when ReviewDb disabled: %s",
change);
}
- ObjectId id = readRef(repo);
+ id = revisionId == null ? readRef(repo) : revisionId;
if (id == null) {
// Meta ref doesn't exist in NoteDb.
@@ -789,7 +835,7 @@
NoteDbUpdateManager.Result r;
try (NoteDbUpdateManager manager = rebuilder.stage(db, cid)) {
if (manager == null) {
- return super.openHandle(repo, oldId); // May be null in tests.
+ return super.openHandleFromRevisionId(repo, oldId); // May be null in tests.
}
manager.setRefLogMessage("Auto-rebuilding change");
r = manager.stageAndApplyDelta(change);
@@ -819,7 +865,7 @@
}
return LoadHandle.create(ChangeNotesCommit.newRevWalk(repo), r.newState().getChangeMetaId());
} catch (NoSuchChangeException e) {
- return super.openHandle(repo, oldId);
+ return super.openHandleFromRevisionId(repo, oldId);
} catch (OrmException e) {
throw new IOException(e);
} finally {
diff --git a/java/com/google/gerrit/server/notedb/DraftCommentNotes.java b/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
index 8acc8d4..ec1cfe8 100644
--- a/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
+++ b/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
@@ -79,7 +79,7 @@
DraftCommentNotes(Args args, @Assisted Change.Id changeId, @Assisted Account.Id author) {
// PrimaryStorage is unknown; this should only called by
// PatchLineCommentsUtil#draftByAuthor, which can live with this.
- super(args, changeId, null, false);
+ super(args, changeId, null, false, null);
this.change = null;
this.author = author;
this.rebuildResult = null;
@@ -93,7 +93,7 @@
boolean autoRebuild,
@Nullable NoteDbUpdateManager.Result rebuildResult,
@Nullable Ref ref) {
- super(args, change.getId(), PrimaryStorage.of(change), autoRebuild);
+ super(args, change.getId(), PrimaryStorage.of(change), autoRebuild, null);
this.change = change;
this.author = author;
this.rebuildResult = rebuildResult;
@@ -182,6 +182,12 @@
}
@Override
+ protected LoadHandle openHandle(Repository repo, @Nullable ObjectId id)
+ throws NoSuchChangeException, IOException {
+ return openHandle(repo);
+ }
+
+ @Override
protected LoadHandle openHandle(Repository repo) throws NoSuchChangeException, IOException {
if (rebuildResult != null) {
StagedResult sr = requireNonNull(rebuildResult.staged());
diff --git a/java/com/google/gerrit/server/notedb/RobotCommentNotes.java b/java/com/google/gerrit/server/notedb/RobotCommentNotes.java
index a05e6a1..399692c 100644
--- a/java/com/google/gerrit/server/notedb/RobotCommentNotes.java
+++ b/java/com/google/gerrit/server/notedb/RobotCommentNotes.java
@@ -49,7 +49,7 @@
@Inject
RobotCommentNotes(Args args, @Assisted Change change) {
- super(args, change.getId(), PrimaryStorage.of(change), false);
+ super(args, change.getId(), PrimaryStorage.of(change), false, null);
this.change = change;
}
