opendev/gerrit
Code Issues Proposed changes

Add config option setting default primary storage for new changes

Browse Source 
Setting to NOTE_DB means new changes will never be written to
ReviewDb. Update ChangeNotes.Factory to properly detect the case of a
change not existing in ReviewDb but possibly still existing in NoteDb.

This config option only controls the primary storage for new changes.
Old changes (that have not been migrated, which is all of them since
this change predates the migration tools) keep their primary storage
as REVIEW_DB. This means that a single running server needs to be able
to handle a mix of NOTE_DB/REVIEW_DB changes. Thus we need to continue
using a live ReviewDb instance in the server, and just avoid writing
any NoteDb-primary changes to that instance.

The easiest way to implement this technically is to keep all the
BatchUpdate code the same but only commit the change transaction if
the change requires ReviewDb. This means we don't have to change any
BatchUpdate.Op implementations, they can continue unconditionally
writing. Rolling back the transaction is much simpler than creating
some kind of ReviewDb wrapper that drops writes on the floor, even
though it does technically create some database traffic even though no
writes are committed.

Add a new NoteDbMode to run all tests with this option enabled,
double-checking after the tests that no changes were stored in
ReviewDb. Tweak tests in various ways to work with this option
enabled, avoiding direct use of ReviewDb when the primary storage is
NoteDb.

Change-Id: I9caf13192f955c4ec90409da32609d0a6f496d96
This commit is contained in:
Dave Borowitz
2016-12-19 14:50:54 -05:00
parent 3dbcb6ea48
commit fc9d1ae55f
14 changed files with 247 additions and 56 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -1049,18 +1049,45 @@ public class BatchUpdate implements AutoCloseable {
private ChangeContext newChangeContext(ReviewDb db, Repository repo,
RevWalk rw, Change.Id id) throws OrmException {
Change c = newChanges.get(id);
if (c == null) {
boolean isNew = c != null;
PrimaryStorage defaultStorage = notesMigration.changePrimaryStorage();
if (isNew) {
// New change: populate noteDbState.
checkState(c.getNoteDbState() == null,
"noteDbState should not be filled in by callers");
if (defaultStorage == PrimaryStorage.NOTE_DB) {
c.setNoteDbState(NoteDbChangeState.NOTE_DB_PRIMARY_STATE);
}
} else {
// Existing change.
c = ChangeNotes.readOneReviewDbChange(db, id);
if (c == null) {
logDebug("Failed to get change {} from unwrapped db", id);
throw new NoSuchChangeException(id);
if (defaultStorage == PrimaryStorage.REVIEW_DB) {
logDebug("Failed to get change {} from unwrapped db", id);
throw new NoSuchChangeException(id);
}
// Not in ReviewDb, but new changes are created with default primary
// storage as NOTE_DB, so we can assume that a missing change is
// NoteDb primary. Pass a synthetic change into ChangeNotes.Factory,
// which lets ChangeNotes take care of the existence check.
//
// TODO(dborowitz): This assumption is potentially risky, because
// it means once we turn this option on and start creating changes
// without writing anything to ReviewDb, we can't turn this option
// back off without making those changes inaccessible. The problem
// is we have no way of distinguishing a change that only exists in
// NoteDb because it only ever existed in NoteDb, from a change that
// only exists in NoteDb because it used to exist in ReviewDb and
// deleting from ReviewDb succeeded but deleting from NoteDb failed.
//
// TODO(dborowitz): We actually still have that problem anyway. Maybe
// we need a cutoff timestamp? Or maybe we need to start leaving
// tombstones in ReviewDb?
c = ChangeNotes.Factory.newNoteDbOnlyChange(project, id);
}
NoteDbChangeState.checkNotReadOnly(c, skewMs);
}
// Pass in preloaded change to controlFor, to avoid:
// - reading from a db that does not belong to this update
// - attempting to read a change that doesn't exist yet
ChangeNotes notes = changeNotesFactory.createForBatchUpdate(c);
ChangeNotes notes = changeNotesFactory.createForBatchUpdate(c, !isNew);
ChangeControl ctl = changeControlFactory.controlFor(notes, user);
return new ChangeContext(ctl, new BatchUpdateReviewDb(db), repo, rw);
}

20
gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
View File

@@ -29,6 +29,7 @@ import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -174,7 +175,20 @@ public abstract class AbstractChangeNotes<T> {
return ref != null ? ref.getObjectId() : null;
}
protected LoadHandle openHandle(Repository repo) throws IOException {
/**
* Open a handle for reading this entity from a repository.
* <p>
* Implementations may override this method to provide auto-rebuilding
* behavior.
*
* @param repo open repository.
* @return handle for reading the entity.
*
* @throws NoSuchChangeException change does not exist.
* @throws IOException a repo-level error occurred.
*/
protected LoadHandle openHandle(Repository repo)
throws NoSuchChangeException, IOException {
return openHandle(repo, readRef(repo));
}
@@ -182,7 +196,7 @@ public abstract class AbstractChangeNotes<T> {
return LoadHandle.create(ChangeNotesCommit.newRevWalk(repo), id);
}
public T reload() throws OrmException {
public T reload() throws NoSuchChangeException, OrmException {
loaded = false;
return load();
}
@@ -215,7 +229,7 @@ public abstract class AbstractChangeNotes<T> {
/** Set up the metadata, parsing any state from the loaded revision. */
protected abstract void onLoad(LoadHandle handle)
throws IOException, ConfigInvalidException;
throws NoSuchChangeException, IOException, ConfigInvalidException;
@SuppressWarnings("unchecked")
protected final T self() {

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

@@ -36,6 +36,7 @@ import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.metrics.Timer1;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.Comment;
@@ -124,7 +125,14 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
public ChangeNotes createChecked(ReviewDb db, Project.NameKey project,
Change.Id changeId) throws OrmException {
Change change = readOneReviewDbChange(db, changeId);
if (change == null || !change.getProject().equals(project)) {
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);
} else if (!change.getProject().equals(project)) {
throw new NoSuchChangeException(changeId);
}
return new ChangeNotes(args, change).load();
@@ -144,16 +152,33 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
return changes.get(0).notes();
}
public static Change newNoteDbOnlyChange(
Project.NameKey project, Change.Id changeId) {
Change change = new Change(
null, changeId, null,
new Branch.NameKey(project, "INVALID_NOTE_DB_ONLY"),
null);
change.setNoteDbState(NoteDbChangeState.NOTE_DB_PRIMARY_STATE);
return change;
}
private Change loadChangeFromDb(ReviewDb db, Project.NameKey project,
Change.Id changeId) throws OrmException {
Change change = readOneReviewDbChange(db, changeId);
checkArgument(project != null, "project is required");
checkNotNull(change,
"change %s not found in ReviewDb", changeId);
checkArgument(change.getProject().equals(project),
"passed project %s when creating ChangeNotes for %s, but actual"
+ " project is %s",
project, changeId, change.getProject());
Change change = readOneReviewDbChange(db, changeId);
if (change == null && args.migration.readChanges()) {
// 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);
} else {
checkNotNull(change, "change %s not found in ReviewDb", changeId);
checkArgument(change.getProject().equals(project),
"passed project %s when creating ChangeNotes for %s, but actual"
+ " project is %s",
project, changeId, change.getProject());
}
// TODO: Throw NoSuchChangeException when the change is not found in the
// database
return change;
@@ -168,7 +193,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
public ChangeNotes createWithAutoRebuildingDisabled(ReviewDb db,
Project.NameKey project, Change.Id changeId) throws OrmException {
return new ChangeNotes(
args, loadChangeFromDb(db, project, changeId), false, null).load();
args, loadChangeFromDb(db, project, changeId), true, false, null).load();
}
/**
@@ -183,13 +208,14 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
return new ChangeNotes(args, change);
}
public ChangeNotes createForBatchUpdate(Change change) throws OrmException {
return new ChangeNotes(args, change, false, null).load();
public ChangeNotes createForBatchUpdate(Change change, boolean shouldExist)
throws OrmException {
return new ChangeNotes(args, change, shouldExist, false, null).load();
}
public ChangeNotes createWithAutoRebuildingDisabled(Change change,
RefCache refs) throws OrmException {
return new ChangeNotes(args, change, false, refs).load();
return new ChangeNotes(args, change, true, false, refs).load();
}
// TODO(ekempin): Remove when database backend is deleted
@@ -226,7 +252,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
public List<ChangeNotes> create(ReviewDb db, Project.NameKey project,
Collection<Change.Id> changeIds, Predicate<ChangeNotes> predicate)
throws OrmException {
throws OrmException {
List<ChangeNotes> notes = new ArrayList<>();
if (args.migration.enabled()) {
for (Change.Id cid : changeIds) {
@@ -302,13 +328,18 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
Project.NameKey project) throws OrmException, IOException {
Set<Change.Id> ids = scan(repo);
List<ChangeNotes> changeNotes = new ArrayList<>(ids.size());
PrimaryStorage defaultStorage = args.migration.changePrimaryStorage();
for (Change.Id id : ids) {
Change change = readOneReviewDbChange(db, id);
if (change == null) {
log.warn("skipping change {} found in project {} " +
"but not in ReviewDb",
id, project);
continue;
if (defaultStorage == PrimaryStorage.REVIEW_DB) {
log.warn("skipping change {} found in project {} " +
"but not in ReviewDb",
id, project);
continue;
}
// TODO(dborowitz): See discussion in BatchUpdate#newChangeContext.
change = newNoteDbOnlyChange(project, id);
} else if (!change.getProject().equals(project)) {
log.error(
"skipping change {} found in project {} " +
@@ -337,6 +368,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
}
}
private final boolean shouldExist;
private final RefCache refs;
private Change change;
@@ -358,13 +390,14 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
@VisibleForTesting
public ChangeNotes(Args args, Change change) {
this(args, change, true, null);
this(args, change, true, true, null);
}
private ChangeNotes(Args args, Change change, boolean autoRebuild,
@Nullable RefCache refs) {
private ChangeNotes(Args args, Change change, boolean shouldExist,
boolean autoRebuild, @Nullable RefCache refs) {
super(args, change.getId(), PrimaryStorage.of(change), autoRebuild);
this.change = new Change(change);
this.shouldExist = shouldExist;
this.refs = refs;
}
@@ -555,9 +588,14 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
@Override
protected void onLoad(LoadHandle handle)
throws IOException, ConfigInvalidException {
throws NoSuchChangeException, IOException, ConfigInvalidException {
ObjectId rev = handle.id();
if (rev == null) {
if (args.migration.readChanges()
&& PrimaryStorage.of(change) == PrimaryStorage.NOTE_DB
&& shouldExist) {
throw new NoSuchChangeException(getChangeId());
}
loadDefaults();
return;
}
@@ -587,12 +625,17 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
}
@Override
protected LoadHandle openHandle(Repository repo) throws IOException {
protected LoadHandle openHandle(Repository repo)
throws NoSuchChangeException, IOException {
if (autoRebuild) {
NoteDbChangeState state = NoteDbChangeState.parse(change);
ObjectId id = readRef(repo);
if (state == null && id == null) {
return super.openHandle(repo, id);
if (id == null) {
if (state == null) {
return super.openHandle(repo, id);
} else if (shouldExist) {
throw new NoSuchChangeException(getChangeId());
}
}
RefCache refs = this.refs != null ? this.refs : new RepoRefCache(repo);
if (!NoteDbChangeState.isChangeUpToDate(state, refs, getChangeId())) {

14
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ConfigNotesMigration.java
View File

@@ -20,6 +20,7 @@ import static com.google.gerrit.server.notedb.NoteDbTable.CHANGES;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -49,9 +50,11 @@ public class ConfigNotesMigration extends NotesMigration {
}
private static final String NOTE_DB = "noteDb";
private static final String PRIMARY_STORAGE = "primaryStorage";
private static final String READ = "read";
private static final String WRITE = "write";
private static final String SEQUENCE = "sequence";
private static final String WRITE = "write";
private static void checkConfig(Config cfg) {
Set<String> keys = new HashSet<>();
@@ -81,6 +84,7 @@ public class ConfigNotesMigration extends NotesMigration {
private final boolean writeChanges;
private final boolean readChanges;
private final boolean readChangeSequence;
private final PrimaryStorage changePrimaryStorage;
private final boolean writeAccounts;
private final boolean readAccounts;
@@ -98,6 +102,9 @@ public class ConfigNotesMigration extends NotesMigration {
// NoteDb. This decision for the default may be reevaluated later.
readChangeSequence = cfg.getBoolean(NOTE_DB, CHANGES.key(), SEQUENCE, false);
changePrimaryStorage = cfg.getEnum(
NOTE_DB, CHANGES.key(), PRIMARY_STORAGE, PrimaryStorage.REVIEW_DB);
writeAccounts = cfg.getBoolean(NOTE_DB, ACCOUNTS.key(), WRITE, false);
readAccounts = cfg.getBoolean(NOTE_DB, ACCOUNTS.key(), READ, false);
}
@@ -117,6 +124,11 @@ public class ConfigNotesMigration extends NotesMigration {
return readChangeSequence;
}
@Override
public PrimaryStorage changePrimaryStorage() {
return changePrimaryStorage;
}
@Override
public boolean writeAccounts() {
return writeAccounts;

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

@@ -188,7 +188,8 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
}
@Override
protected LoadHandle openHandle(Repository repo) throws IOException {
protected LoadHandle openHandle(Repository repo)
throws NoSuchChangeException, IOException {
if (rebuildResult != null) {
StagedResult sr = checkNotNull(rebuildResult.staged());
return LoadHandle.create(
@@ -216,7 +217,8 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
return null;
}
private LoadHandle rebuildAndOpen(Repository repo) throws IOException {
private LoadHandle rebuildAndOpen(Repository repo)
throws NoSuchChangeException, IOException {
Timer1.Context timer = args.metrics.autoRebuildLatency.start(CHANGES);
try {
Change.Id cid = getChangeId();

5
gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java
View File

@@ -14,6 +14,8 @@
package com.google.gerrit.server.notedb;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
/**
* Holds the current state of the NoteDb migration.
* <p>
@@ -71,6 +73,9 @@ public abstract class NotesMigration {
*/
public abstract boolean readChangeSequence();
/** @return default primary storage for new changes. */
public abstract PrimaryStorage changePrimaryStorage();
public abstract boolean readAccounts();
public abstract boolean writeAccounts();