opendev/gerrit
Code Issues Proposed changes

Support read-only mode for NoteDb changes

Browse Source 
Offline or batch programs might want to read NoteDb changes from the
repo but without any chance of writing to the repo and incurring
contention. Support this by allowing NotesMigration to specify
readChanges() = true and writeChanges() = false, which was previously
disallowed. Now the behavior of writeChanges() = false is either a
no-op or a hard failure, depending on whether reading is enabled.

Change-Id: I3255a3380c50dcc3e52028f146b1d36d83bfdd41
This commit is contained in:
Dave Borowitz
2016-06-24 13:30:11 -04:00
parent f6e5149bbf
commit 7ce3e1755e
10 changed files with 150 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

69
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java
View File

@@ -20,6 +20,7 @@ import static com.google.gerrit.reviewdb.client.RefNames.changeMetaRef;
import static com.google.gerrit.reviewdb.client.RefNames.refsDraftComments;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
import static org.junit.Assert.fail;
import com.google.common.base.Function;
import com.google.common.collect.ImmutableList;
@@ -34,6 +35,7 @@ import com.google.gerrit.extensions.api.changes.DraftInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
@@ -52,9 +54,11 @@ import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.git.BatchUpdate;
import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
import com.google.gerrit.server.git.RepoRefCache;
import com.google.gerrit.server.git.UpdateException;
import com.google.gerrit.server.notedb.ChangeBundle;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.NoteDbChangeState;
import com.google.gerrit.server.notedb.NoteDbUpdateManager;
import com.google.gerrit.server.notedb.TestChangeRebuilderWrapper;
import com.google.gerrit.server.schema.DisabledChangesReviewDbWrapper;
import com.google.gerrit.testutil.ConfigSuite;
@@ -880,6 +884,71 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
}
}
@Test
public void failWhenWritesDisabled() throws Exception {
setNotesMigration(true, true);
PushOneCommit.Result r = createChange();
Change.Id id = r.getPatchSetId().getParentKey();
assertChangeUpToDate(true, id);
assertThat(gApi.changes().id(id.get()).info().topic).isNull();
// Turning off writes causes failure.
setNotesMigration(false, true);
try {
gApi.changes().id(id.get()).topic(name("a-topic"));
fail("Expected write to fail");
} catch (RestApiException e) {
assertChangesReadOnly(e);
}
// Update was not written.
assertThat(gApi.changes().id(id.get()).info().topic).isNull();
assertChangeUpToDate(true, id);
}
@Test
public void rebuildWhenWritesDisabledWorksButDoesNotWrite() throws Exception {
setNotesMigration(true, true);
PushOneCommit.Result r = createChange();
Change.Id id = r.getPatchSetId().getParentKey();
assertChangeUpToDate(true, id);
// Make a ReviewDb change behind NoteDb's back and ensure it's detected.
setNotesMigration(false, false);
gApi.changes().id(id.get()).topic(name("a-topic"));
setInvalidNoteDbState(id);
assertChangeUpToDate(false, id);
// On next NoteDb read, change is rebuilt in-memory but not stored.
setNotesMigration(false, true);
assertThat(gApi.changes().id(id.get()).info().topic)
.isEqualTo(name("a-topic"));
assertChangeUpToDate(false, id);
// Attempting to write directly causes failure.
try {
gApi.changes().id(id.get()).topic(name("other-topic"));
fail("Expected write to fail");
} catch (RestApiException e) {
assertChangesReadOnly(e);
}
// Update was not written.
assertThat(gApi.changes().id(id.get()).info().topic)
.isEqualTo(name("a-topic"));
assertChangeUpToDate(false, id);
}
private void assertChangesReadOnly(RestApiException e) throws Exception {
Throwable cause = e.getCause();
assertThat(cause).isInstanceOf(UpdateException.class);
assertThat(cause.getCause()).isInstanceOf(OrmException.class);
assertThat(cause.getCause())
.hasMessage(NoteDbUpdateManager.CHANGES_READ_ONLY);
}
private void setInvalidNoteDbState(Change.Id id) throws Exception {
ReviewDb db = unwrapDb();
Change c = db.changes().get(id);

2
gerrit-server/src/main/java/com/google/gerrit/server/change/Rebuild.java
View File

@@ -54,7 +54,7 @@ public class Rebuild implements RestModifyView<ChangeResource, Input> {
public Response<?> apply(ChangeResource rsrc, Input input)
throws ResourceNotFoundException, IOException, OrmException,
ConfigInvalidException {
if (!migration.writeChanges()) {
if (!migration.commitChangeWrites()) {
throw new ResourceNotFoundException();
}
try {

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

@@ -636,6 +636,11 @@ public class BatchUpdate implements AutoCloseable {
List<ChangeTask> tasks = new ArrayList<>(ops.keySet().size());
try {
if (!ops.isEmpty() && notesMigration.failChangeWrites()) {
// Fail fast before attempting any writes if changes are read-only, as
// this is a programmer error.
throw new OrmException(NoteDbUpdateManager.CHANGES_READ_ONLY);
}
List<ListenableFuture<?>> futures = new ArrayList<>(ops.keySet().size());
for (Map.Entry<Change.Id, Collection<Op>> e : ops.asMap().entrySet()) {
ChangeTask task =
@@ -645,13 +650,15 @@ public class BatchUpdate implements AutoCloseable {
}
Futures.allAsList(futures).get();
if (notesMigration.writeChanges()) {
if (notesMigration.commitChangeWrites()) {
executeNoteDbUpdates(tasks);
}
} catch (ExecutionException | InterruptedException e) {
Throwables.propagateIfInstanceOf(e.getCause(), UpdateException.class);
Throwables.propagateIfInstanceOf(e.getCause(), RestApiException.class);
throw new UpdateException(e);
} catch (OrmException e) {
throw new UpdateException(e);
}
// Reindex changes.
@@ -802,7 +809,7 @@ public class BatchUpdate implements AutoCloseable {
deleted = ctx.deleted;
// Stage the NoteDb update and store its state in the Change.
if (notesMigration.writeChanges()) {
if (notesMigration.commitChangeWrites()) {
updateManager = stageNoteDbUpdate(ctx, deleted);
}
@@ -821,7 +828,7 @@ public class BatchUpdate implements AutoCloseable {
db.rollback();
}
if (notesMigration.writeChanges()) {
if (notesMigration.commitChangeWrites()) {
try {
// Do not execute the NoteDbUpdateManager, as we don't want too much
// contention on the underlying repo, and we would rather use a

4
gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java
View File

@@ -204,9 +204,7 @@ public class ChangeIndexer {
public void index(ReviewDb db, Change change)
throws IOException, OrmException {
ChangeData cd;
if (notesMigration.readChanges()) {
cd = changeDataFactory.create(db, change);
} else if (notesMigration.writeChanges()) {
if (notesMigration.commitChangeWrites()) {
// Auto-rebuilding when NoteDb reads are disabled just increases
// contention on the meta ref from a background indexing thread with
// little benefit. The next actual write to the entity may still incur a

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

@@ -192,6 +192,11 @@ public abstract class AbstractChangeUpdate {
if (isEmpty()) {
return null;
}
// Allow this method to proceed even if migration.failChangeWrites() = true.
// This may be used by an auto-rebuilding step that the caller does not plan
// to actually store.
checkArgument(rw.getObjectReader().getCreatedFromInserter() == ins);
ObjectId z = ObjectId.zeroId();
CommitBuilder cb = applyImpl(rw, ins, curr);

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

@@ -122,6 +122,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
private final ChangeNoteUtil changeNoteUtil;
private final ChangeUpdate.Factory updateFactory;
private final NoteDbUpdateManager.Factory updateManagerFactory;
private final NotesMigration migration;
private final PatchListCache patchListCache;
private final PersonIdent serverIdent;
private final ProjectCache projectCache;
@@ -134,6 +135,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
ChangeNoteUtil changeNoteUtil,
ChangeUpdate.Factory updateFactory,
NoteDbUpdateManager.Factory updateManagerFactory,
NotesMigration migration,
PatchListCache patchListCache,
@GerritPersonIdent PersonIdent serverIdent,
@Nullable ProjectCache projectCache,
@@ -144,6 +146,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
this.changeNoteUtil = changeNoteUtil;
this.updateFactory = updateFactory;
this.updateManagerFactory = updateManagerFactory;
this.migration = migration;
this.patchListCache = patchListCache;
this.serverIdent = serverIdent;
this.projectCache = projectCache;
@@ -221,7 +224,14 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
return change;
}
});
if (!migration.failChangeWrites()) {
manager.execute();
} else {
// Don't even attempt to execute if read-only, it would fail anyway. But
// do throw an exception to the caller so they know to use the staged
// results instead of reading from the repo.
throw new OrmException(NoteDbUpdateManager.CHANGES_READ_ONLY);
}
} catch (AbortUpdateException e) {
// Drop this rebuild; another thread completed it.
}

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

@@ -64,10 +64,6 @@ public class ConfigNotesMigration extends NotesMigration {
checkArgument(lk.equals(WRITE) || lk.equals(READ),
"invalid NoteDb key: %s.%s", t, key);
}
boolean write = cfg.getBoolean(NOTE_DB, t, WRITE, false);
boolean read = cfg.getBoolean(NOTE_DB, t, READ, false);
checkArgument(!(read && !write),
"must have write enabled when read enabled: %s", t);
}
}
@@ -95,7 +91,7 @@ public class ConfigNotesMigration extends NotesMigration {
}
@Override
public boolean writeChanges() {
protected boolean writeChanges() {
return writeChanges;
}

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

@@ -70,6 +70,8 @@ import java.util.Set;
* of updates, use {@link #stage()}.
*/
public class NoteDbUpdateManager {
public static String CHANGES_READ_ONLY = "NoteDb changes are read-only";
public interface Factory {
NoteDbUpdateManager create(Project.NameKey projectName);
}
@@ -249,7 +251,7 @@ public class NoteDbUpdateManager {
}
private boolean isEmpty() {
if (!migration.writeChanges()) {
if (!migration.commitChangeWrites()) {
return true;
}
return changeUpdates.isEmpty()
@@ -382,6 +384,10 @@ public class NoteDbUpdateManager {
}
public void execute() throws OrmException, IOException {
// Check before even inspecting the list, as this is a programmer error.
if (migration.failChangeWrites()) {
throw new OrmException(CHANGES_READ_ONLY);
}
if (isEmpty()) {
return;
}

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

@@ -34,9 +34,33 @@ package com.google.gerrit.server.notedb;
* these reasons, the options remain undocumented.
*/
public abstract class NotesMigration {
/**
* Read changes from NoteDb.
* <p>
* Change data is read from NoteDb refs, but ReviewDb is still the source of
* truth. If the loader determines NoteDb is out of date, the change data in
* NoteDb will be transparently rebuilt. This means that some code paths that
* look read-only may in fact attempt to write.
* <p>
* If true and {@code writeChanges() = false}, changes can still be read from
* NoteDb, but any attempts to write will generate an error.
*/
public abstract boolean readChanges();
public abstract boolean writeChanges();
/**
* Write changes to NoteDb.
* <p>
* Updates to change data are written to NoteDb refs, but ReviewDb is still
* the source of truth. Change data will not be written unless the NoteDb refs
* are already up to date, and the write path will attempt to rebuild the
* change if not.
* <p>
* If false, the behavior when attempting to write depends on
* {@code readChanges()}. If {@code readChanges() = false}, writes to NoteDb
* are simply ignored; if {@code true}, any attempts to write will generate an
* error.
*/
protected abstract boolean writeChanges();
public abstract boolean readAccounts();
@@ -51,6 +75,24 @@ public abstract class NotesMigration {
return false;
}
public boolean commitChangeWrites() {
// It may seem odd that readChanges() without writeChanges() means we should
// attempt to commit writes. However, this method is used by callers to know
// whether or not they should short-circuit and skip attempting to read or
// write NoteDb refs.
//
// It is possible for commitChangeWrites() to return true and
// failChangeWrites() to also return true, causing an error later in the
// same codepath. This specific condition is used by the auto-rebuilding
// path to rebuild a change and stage the results, but not commit them due
// to failChangeWrites().
return writeChanges() || readChanges();
}
public boolean failChangeWrites() {
return !writeChanges() && readChanges();
}
public boolean enabled() {
return writeChanges() || readChanges()
|| writeAccounts() || readAccounts();

2
gerrit-server/src/test/java/com/google/gerrit/testutil/TestNotesMigration.java
View File

@@ -29,6 +29,8 @@ public class TestNotesMigration extends NotesMigration {
return readChanges;
}
// Increase visbility from superclass, as tests may want to check whether
// NoteDb data is written in specific migration scenarios.
@Override
public boolean writeChanges() {
return writeChanges;