opendev/gerrit
Code Issues Proposed changes

Remove NotesMigration from AbstractChangeNotes.Args

Browse Source 
Eliminate non-NoteDb conditions from subclasses. The only remaining
thing that's not quite related to the NoteDb migration is the
failOnLoadForTest mechanism, which was wedged into the NotesMigration
since it was a convenient piece of global state. Replace this with an
equally ugly piece of global state, a simple AtomicBoolean, punting for
now on a better approach.

Change-Id: I186744413ef9b263c5c6cabe19c3e5297230974f
This commit is contained in:
Dave Borowitz
2018-12-12 14:48:12 -08:00
parent 35fa54c41f
commit 300af25bef
3 changed files with 32 additions and 79 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
java/com/google/gerrit/acceptance/AbstractDaemonTest.java
View File

@@ -112,6 +112,7 @@ import com.google.gerrit.server.index.change.ChangeIndex;
import com.google.gerrit.server.index.change.ChangeIndexCollection;
import com.google.gerrit.server.index.change.ChangeIndexer;
import com.google.gerrit.server.index.group.GroupIndexer;
import com.google.gerrit.server.notedb.AbstractChangeNotes;
import com.google.gerrit.server.notedb.ChangeNoteUtil;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.MutableNotesMigration;
@@ -293,6 +294,7 @@ public abstract class AbstractDaemonTest {
@Inject private AccountIndexer accountIndexer;
@Inject private Groups groups;
@Inject private GroupIndexer groupIndexer;
@Inject private AbstractChangeNotes.Args changeNotesArgs;
private ProjectResetter resetter;
private List<Repository> toClose;
@@ -837,12 +839,12 @@ public abstract class AbstractDaemonTest {
}
protected Context disableDb() {
notesMigration.setFailOnLoadForTest(true);
changeNotesArgs.failOnLoadForTest.set(true);
return atrScope.disableDb();
}
protected void enableDb(Context preDisableContext) {
notesMigration.setFailOnLoadForTest(false);
changeNotesArgs.failOnLoadForTest.set(false);
atrScope.set(preDisableContext);
}

14
java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
View File

@@ -14,7 +14,6 @@
package com.google.gerrit.server.notedb;
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.notedb.NoteDbTable.CHANGES;
import static java.util.Objects.requireNonNull;
@@ -34,6 +33,7 @@ import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.concurrent.atomic.AtomicBoolean;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
@@ -44,8 +44,10 @@ public abstract class AbstractChangeNotes<T> {
@VisibleForTesting
@Singleton
public static class Args {
// TODO(dborowitz): Some less smelly way of disabling NoteDb in tests.
public final AtomicBoolean failOnLoadForTest;
final GitRepositoryManager repoManager;
final NotesMigration migration;
final AllUsersName allUsers;
final ChangeNoteJson changeNoteJson;
final LegacyChangeNoteRead legacyChangeNoteRead;
@@ -60,15 +62,14 @@ public abstract class AbstractChangeNotes<T> {
@Inject
Args(
GitRepositoryManager repoManager,
NotesMigration migration,
AllUsersName allUsers,
ChangeNoteJson changeNoteJson,
LegacyChangeNoteRead legacyChangeNoteRead,
NoteDbMetrics metrics,
Provider<ReviewDb> db,
Provider<ChangeNotesCache> cache) {
this.failOnLoadForTest = new AtomicBoolean();
this.repoManager = repoManager;
this.migration = migration;
this.allUsers = allUsers;
this.legacyChangeNoteRead = legacyChangeNoteRead;
this.changeNoteJson = changeNoteJson;
@@ -132,8 +133,7 @@ public abstract class AbstractChangeNotes<T> {
return self();
}
checkState(args.migration.readChanges(), "NoteDb is required to read changes");
if (args.migration.failOnLoadForTest()) {
if (args.failOnLoadForTest.get()) {
throw new OrmException("Reading from NoteDb is disabled");
}
try (Timer1.Context timer = args.metrics.readLatency.start(CHANGES);
@@ -181,8 +181,6 @@ public abstract class AbstractChangeNotes<T> {
public ObjectId loadRevision() throws OrmException {
if (loaded) {
return getRevision();
} else if (!args.migration.readChanges()) {
return null;
}
try (Repository repo = args.repoManager.openRepository(getProjectName())) {
Ref ref = repo.getRefDatabase().exactRef(getRefName());

49
java/com/google/gerrit/server/notedb/ChangeNotes.java
View File

@@ -120,9 +120,6 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
throws OrmException {
Change change = readOneReviewDbChange(db, changeId);
if (change == null) {
if (!args.migration.readChanges()) {
throw new NoSuchChangeException(changeId);
}
// Change isn't in ReviewDb, but its primary storage might be in NoteDb.
// Prepopulate the change exists with proper noteDbState field.
change = newNoteDbOnlyChange(project, changeId);
@@ -159,11 +156,8 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
Change change = readOneReviewDbChange(db, changeId);
if (change == null) {
if (args.migration.readChanges()) {
return newNoteDbOnlyChange(project, changeId);
}
throw new NoSuchChangeException(changeId);
}
checkArgument(
change.getProject().equals(project),
"passed project %s when creating ChangeNotes for %s, but actual project is %s",
@@ -198,21 +192,9 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
return new ChangeNotes(args, change, true, refs).load();
}
// TODO(ekempin): Remove when database backend is deleted
/**
* Instantiate ChangeNotes for a change that has been loaded by a batch read from the database.
*/
private ChangeNotes createFromChangeOnlyWhenNoteDbDisabled(Change change) throws OrmException {
checkState(
!args.migration.readChanges(),
"do not call createFromChangeWhenNoteDbDisabled when NoteDb is enabled");
return new ChangeNotes(args, change).load();
}
public List<ChangeNotes> create(ReviewDb db, Collection<Change.Id> changeIds)
throws OrmException {
List<ChangeNotes> notes = new ArrayList<>();
if (args.migration.readChanges()) {
for (Change.Id changeId : changeIds) {
try {
notes.add(createChecked(changeId));
@@ -223,12 +205,6 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
return notes;
}
for (Change c : ReviewDbUtil.unwrapDb(db).changes().get(changeIds)) {
notes.add(createFromChangeOnlyWhenNoteDbDisabled(c));
}
return notes;
}
public List<ChangeNotes> create(
ReviewDb db,
Project.NameKey project,
@@ -236,7 +212,6 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
Predicate<ChangeNotes> predicate)
throws OrmException {
List<ChangeNotes> notes = new ArrayList<>();
if (args.migration.readChanges()) {
for (Change.Id cid : changeIds) {
try {
ChangeNotes cn = create(db, project, cid);
@@ -252,22 +227,10 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
return notes;
}
for (Change c : ReviewDbUtil.unwrapDb(db).changes().get(changeIds)) {
if (c != null && project.equals(c.getDest().getParentKey())) {
ChangeNotes cn = createFromChangeOnlyWhenNoteDbDisabled(c);
if (predicate.test(cn)) {
notes.add(cn);
}
}
}
return notes;
}
public ListMultimap<Project.NameKey, ChangeNotes> create(
ReviewDb db, Predicate<ChangeNotes> predicate) throws IOException, OrmException {
ListMultimap<Project.NameKey, ChangeNotes> m =
MultimapBuilder.hashKeys().arrayListValues().build();
if (args.migration.readChanges()) {
for (Project.NameKey project : projectCache.all()) {
try (Repository repo = args.repoManager.openRepository(project)) {
scan(repo, project)
@@ -277,14 +240,6 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
.forEach(n -> m.put(n.getProjectName(), n));
}
}
} else {
for (Change change : ReviewDbUtil.unwrapDb(db).changes().all()) {
ChangeNotes notes = createFromChangeOnlyWhenNoteDbDisabled(change);
if (predicate.test(notes)) {
m.put(change.getProject(), notes);
}
}
}
return ImmutableListMultimap.copyOf(m);
}
@@ -594,9 +549,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
protected void onLoad(LoadHandle handle) throws NoSuchChangeException, IOException {
ObjectId rev = handle.id();
if (rev == null) {
if (args.migration.readChanges()
&& PrimaryStorage.of(change) == PrimaryStorage.NOTE_DB
&& shouldExist) {
if (PrimaryStorage.of(change) == PrimaryStorage.NOTE_DB && shouldExist) {
throw new NoSuchChangeException(getChangeId());
}
loadDefaults();