From 22b8412ef339d31430cb8b26f2c836f6c788e601 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 8 Feb 2017 12:04:33 -0500 Subject: [PATCH] Support disabling change ReviewDb table access entirely We already had DisabledChangesReviewDbWrapper to throw an exception indicating it's a programmer error to try to read from changes tables directly. By design, it was still possible to punch through this wrapper and get access to the ReviewDb table. We need to go deeper if we really want to shut off the changes tables. Add a new config option that adds another level of wrapping to ReviewDb, which makes all operations into complete no-ops. When this option is set, there is literally no way to write to or read from the underlying database table using the ReviewDb API. The table could go away and Gerrit would still function. Change-Id: Ia23a51449217a50eeaecd9934626270739c096e0 --- Documentation/dev-note-db.txt | 10 + ...sallowReadFromChangesReviewDbWrapper.java} | 44 +-- .../gerrit/reviewdb/server/ReviewDbUtil.java | 4 +- .../server/notedb/ConfigNotesMigration.java | 13 + .../gerrit/server/notedb/NotesMigration.java | 9 + .../schema/NoChangesReviewDbWrapper.java | 350 ++++++++++++++++++ .../schema/NotesMigrationSchemaFactory.java | 32 +- .../google/gerrit/testutil/NoteDbMode.java | 21 +- .../gerrit/testutil/TestNotesMigration.java | 21 ++ 9 files changed, 472 insertions(+), 32 deletions(-) rename gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/{DisabledChangesReviewDbWrapper.java => DisallowReadFromChangesReviewDbWrapper.java} (81%) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/schema/NoChangesReviewDbWrapper.java diff --git a/Documentation/dev-note-db.txt b/Documentation/dev-note-db.txt index 800dd2f409..ef35d7d1f6 100644 --- a/Documentation/dev-note-db.txt +++ b/Documentation/dev-note-db.txt @@ -77,6 +77,16 @@ previous options, unless otherwise noted. Due to an implementation detail, writes to Changes or related tables still result in write calls to the database layer, but they are inside a transaction that is always rolled back. +- `noteDb.changes.disableReviewDb=true`: All access to Changes or related tables + is disabled; reads return no results, and writes are no-ops. Assumes the state + of all changes in NoteDb is accurate, and so is only safe once all changes are + NoteDb primary. Otherwise, reading changes only from NoteDb might result in + inaccurate results, and writing to NoteDb would compound the problem. + + Thus it is up to an admin of a previously-ReviewDb site to ensure + MigratePrimaryStorage has been run for all changes. Note that the current + implementation of the `rebuild-note-db` program does not do this. + + In this phase, it would be possible to delete the Changes tables out from + under a running server with no effect. [[migration]] == Migration diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/DisabledChangesReviewDbWrapper.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/DisallowReadFromChangesReviewDbWrapper.java similarity index 81% rename from gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/DisabledChangesReviewDbWrapper.java rename to gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/DisallowReadFromChangesReviewDbWrapper.java index 2af8d11468..9791572875 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/DisabledChangesReviewDbWrapper.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/DisallowReadFromChangesReviewDbWrapper.java @@ -24,22 +24,22 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.ResultSet; -public class DisabledChangesReviewDbWrapper extends ReviewDbWrapper { +public class DisallowReadFromChangesReviewDbWrapper extends ReviewDbWrapper { private static final String MSG = "This table has been migrated to NoteDb"; - private final DisabledChangeAccess changes; - private final DisabledPatchSetApprovalAccess patchSetApprovals; - private final DisabledChangeMessageAccess changeMessages; - private final DisabledPatchSetAccess patchSets; - private final DisabledPatchLineCommentAccess patchComments; + private final Changes changes; + private final PatchSetApprovals patchSetApprovals; + private final ChangeMessages changeMessages; + private final PatchSets patchSets; + private final PatchLineComments patchComments; - public DisabledChangesReviewDbWrapper(ReviewDb db) { + public DisallowReadFromChangesReviewDbWrapper(ReviewDb db) { super(db); - changes = new DisabledChangeAccess(delegate.changes()); - patchSetApprovals = new DisabledPatchSetApprovalAccess(delegate.patchSetApprovals()); - changeMessages = new DisabledChangeMessageAccess(delegate.changeMessages()); - patchSets = new DisabledPatchSetAccess(delegate.patchSets()); - patchComments = new DisabledPatchLineCommentAccess(delegate.patchComments()); + changes = new Changes(delegate.changes()); + patchSetApprovals = new PatchSetApprovals(delegate.patchSetApprovals()); + changeMessages = new ChangeMessages(delegate.changeMessages()); + patchSets = new PatchSets(delegate.patchSets()); + patchComments = new PatchLineComments(delegate.patchComments()); } public ReviewDb unsafeGetDelegate() { @@ -71,9 +71,9 @@ public class DisabledChangesReviewDbWrapper extends ReviewDbWrapper { return patchComments; } - private static class DisabledChangeAccess extends ChangeAccessWrapper { + private static class Changes extends ChangeAccessWrapper { - protected DisabledChangeAccess(ChangeAccess delegate) { + protected Changes(ChangeAccess delegate) { super(delegate); } @@ -103,8 +103,8 @@ public class DisabledChangesReviewDbWrapper extends ReviewDbWrapper { } } - private static class DisabledPatchSetApprovalAccess extends PatchSetApprovalAccessWrapper { - DisabledPatchSetApprovalAccess(PatchSetApprovalAccess delegate) { + private static class PatchSetApprovals extends PatchSetApprovalAccessWrapper { + PatchSetApprovals(PatchSetApprovalAccess delegate) { super(delegate); } @@ -139,8 +139,8 @@ public class DisabledChangesReviewDbWrapper extends ReviewDbWrapper { } } - private static class DisabledChangeMessageAccess extends ChangeMessageAccessWrapper { - DisabledChangeMessageAccess(ChangeMessageAccess delegate) { + private static class ChangeMessages extends ChangeMessageAccessWrapper { + ChangeMessages(ChangeMessageAccess delegate) { super(delegate); } @@ -180,8 +180,8 @@ public class DisabledChangesReviewDbWrapper extends ReviewDbWrapper { } } - private static class DisabledPatchSetAccess extends PatchSetAccessWrapper { - DisabledPatchSetAccess(PatchSetAccess delegate) { + private static class PatchSets extends PatchSetAccessWrapper { + PatchSets(PatchSetAccess delegate) { super(delegate); } @@ -211,8 +211,8 @@ public class DisabledChangesReviewDbWrapper extends ReviewDbWrapper { } } - private static class DisabledPatchLineCommentAccess extends PatchLineCommentAccessWrapper { - DisabledPatchLineCommentAccess(PatchLineCommentAccess delegate) { + private static class PatchLineComments extends PatchLineCommentAccessWrapper { + PatchLineComments(PatchLineCommentAccess delegate) { super(delegate); } diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java index 27a8e72730..bb31b1cae0 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java @@ -42,8 +42,8 @@ public class ReviewDbUtil { } public static ReviewDb unwrapDb(ReviewDb db) { - if (db instanceof DisabledChangesReviewDbWrapper) { - return ((DisabledChangesReviewDbWrapper) db).unsafeGetDelegate(); + if (db instanceof DisallowReadFromChangesReviewDbWrapper) { + return ((DisallowReadFromChangesReviewDbWrapper) db).unsafeGetDelegate(); } return db; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ConfigNotesMigration.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ConfigNotesMigration.java index b1dd2d6c95..c0b0525dfe 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ConfigNotesMigration.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ConfigNotesMigration.java @@ -47,6 +47,7 @@ public class ConfigNotesMigration extends NotesMigration { private static final String NOTE_DB = "noteDb"; // All of these names must be reflected in the allowed set in checkConfig. + private static final String DISABLE_REVIEW_DB = "disableReviewDb"; private static final String PRIMARY_STORAGE = "primaryStorage"; private static final String READ = "read"; private static final String SEQUENCE = "sequence"; @@ -56,6 +57,7 @@ public class ConfigNotesMigration extends NotesMigration { Set keys = ImmutableSet.of(CHANGES.key()); Set allowed = ImmutableSet.of( + DISABLE_REVIEW_DB.toLowerCase(), PRIMARY_STORAGE.toLowerCase(), READ.toLowerCase(), WRITE.toLowerCase(), @@ -79,6 +81,7 @@ public class ConfigNotesMigration extends NotesMigration { private final boolean readChanges; private final boolean readChangeSequence; private final PrimaryStorage changePrimaryStorage; + private final boolean disableChangeReviewDb; @Inject ConfigNotesMigration(@GerritServerConfig Config cfg) { @@ -95,6 +98,11 @@ public class ConfigNotesMigration extends NotesMigration { changePrimaryStorage = cfg.getEnum(NOTE_DB, CHANGES.key(), PRIMARY_STORAGE, PrimaryStorage.REVIEW_DB); + disableChangeReviewDb = cfg.getBoolean(NOTE_DB, CHANGES.key(), DISABLE_REVIEW_DB, false); + + checkArgument( + !(disableChangeReviewDb && changePrimaryStorage != PrimaryStorage.NOTE_DB), + "cannot disable ReviewDb for changes if default change primary storage is ReviewDb"); } @Override @@ -116,4 +124,9 @@ public class ConfigNotesMigration extends NotesMigration { public PrimaryStorage changePrimaryStorage() { return changePrimaryStorage; } + + @Override + public boolean disableChangeReviewDb() { + return disableChangeReviewDb; + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java index 1e6d2b4fc3..c708bfecfd 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java @@ -70,6 +70,15 @@ public abstract class NotesMigration { /** @return default primary storage for new changes. */ public abstract PrimaryStorage changePrimaryStorage(); + /** + * Disable ReviewDb access for changes. + * + *

When set, ReviewDb operations involving the Changes table become no-ops. Lookups return no + * results; updates do nothing, as does opening, committing, or rolling back a transaction on the + * Changes table. + */ + public abstract boolean disableChangeReviewDb(); + /** * Whether to fail when reading any data from NoteDb. * diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/NoChangesReviewDbWrapper.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/NoChangesReviewDbWrapper.java new file mode 100644 index 0000000000..43e9a3a49c --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/NoChangesReviewDbWrapper.java @@ -0,0 +1,350 @@ +// Copyright (C) 2017 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.schema; + +import static com.google.common.base.Preconditions.checkState; + +import com.google.common.collect.ImmutableList; +import com.google.common.util.concurrent.CheckedFuture; +import com.google.common.util.concurrent.Futures; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.ChangeMessage; +import com.google.gerrit.reviewdb.client.PatchLineComment; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.client.PatchSetApproval; +import com.google.gerrit.reviewdb.server.ChangeAccess; +import com.google.gerrit.reviewdb.server.ChangeMessageAccess; +import com.google.gerrit.reviewdb.server.PatchLineCommentAccess; +import com.google.gerrit.reviewdb.server.PatchSetAccess; +import com.google.gerrit.reviewdb.server.PatchSetApprovalAccess; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.reviewdb.server.ReviewDbWrapper; +import com.google.gwtorm.client.Key; +import com.google.gwtorm.server.Access; +import com.google.gwtorm.server.AtomicUpdate; +import com.google.gwtorm.server.ListResultSet; +import com.google.gwtorm.server.OrmException; +import com.google.gwtorm.server.ResultSet; +import java.util.Map; +import java.util.function.Function; + +/** + * Wrapper for ReviewDb that never calls the underlying change tables. + * + *

See {@link NotesMigrationSchemaFactory} for discussion. + */ +class NoChangesReviewDbWrapper extends ReviewDbWrapper { + private static ResultSet empty() { + return new ListResultSet<>(ImmutableList.of()); + } + + private static > CheckedFuture emptyFuture() { + return Futures.immediateCheckedFuture(null); + } + + private final ChangeAccess changes; + private final PatchSetApprovalAccess patchSetApprovals; + private final ChangeMessageAccess changeMessages; + private final PatchSetAccess patchSets; + private final PatchLineCommentAccess patchComments; + + private boolean inTransaction; + + NoChangesReviewDbWrapper(ReviewDb db) { + super(db); + changes = new Changes(this, delegate); + patchSetApprovals = new PatchSetApprovals(this, delegate); + changeMessages = new ChangeMessages(this, delegate); + patchSets = new PatchSets(this, delegate); + patchComments = new PatchLineComments(this, delegate); + } + + @Override + public ChangeAccess changes() { + return changes; + } + + @Override + public PatchSetApprovalAccess patchSetApprovals() { + return patchSetApprovals; + } + + @Override + public ChangeMessageAccess changeMessages() { + return changeMessages; + } + + @Override + public PatchSetAccess patchSets() { + return patchSets; + } + + @Override + public PatchLineCommentAccess patchComments() { + return patchComments; + } + + @Override + public void commit() throws OrmException { + if (!inTransaction) { + // This reads a little weird, we're not in a transaction, so why are we calling commit? + // Because we want to let the underlying ReviewDb do its normal thing in this case (which may + // be throwing an exception, or not, depending on implementation). + delegate.commit(); + } + } + + @Override + public void rollback() throws OrmException { + if (inTransaction) { + inTransaction = false; + } else { + // See comment in commit(): we want to let the underlying ReviewDb do its thing. + delegate.rollback(); + } + } + + private abstract static class AbstractDisabledAccess> + implements Access { + // Don't even hold a reference to delegate, so it's not possible to use it accidentally. + private final NoChangesReviewDbWrapper wrapper; + private final String relationName; + private final int relationId; + private final Function primaryKey; + private final Function, Map> toMap; + + private AbstractDisabledAccess(NoChangesReviewDbWrapper wrapper, Access delegate) { + this.wrapper = wrapper; + this.relationName = delegate.getRelationName(); + this.relationId = delegate.getRelationID(); + this.primaryKey = delegate::primaryKey; + this.toMap = delegate::toMap; + } + + @Override + public final int getRelationID() { + return relationId; + } + + @Override + public final String getRelationName() { + return relationName; + } + + @Override + public final K primaryKey(T entity) { + return primaryKey.apply(entity); + } + + @Override + public final Map toMap(Iterable iterable) { + return toMap.apply(iterable); + } + + @Override + public final ResultSet iterateAllEntities() { + return empty(); + } + + @Override + public final CheckedFuture getAsync(K key) { + return emptyFuture(); + } + + @Override + public final ResultSet get(Iterable keys) { + return empty(); + } + + @Override + public final void insert(Iterable instances) { + // Do nothing. + } + + @Override + public final void update(Iterable instances) { + // Do nothing. + } + + @Override + public final void upsert(Iterable instances) { + // Do nothing. + } + + @Override + public final void deleteKeys(Iterable keys) { + // Do nothing. + } + + @Override + public final void delete(Iterable instances) { + // Do nothing. + } + + @Override + public final void beginTransaction(K key) { + // Keep track of when we've started a transaction so that we can avoid calling commit/rollback + // on the underlying ReviewDb. This is just a simple arm's-length approach, and may produce + // slightly different results from a native ReviewDb in corner cases like: + // * beginning transactions on different tables simultaneously + // * doing work between commit and rollback + // These kinds of things are already misuses of ReviewDb, and shouldn't be happening in + // current code anyway. + checkState(!wrapper.inTransaction, "already in transaction"); + wrapper.inTransaction = true; + } + + @Override + public final T atomicUpdate(K key, AtomicUpdate update) { + return null; + } + + @Override + public final T get(K id) { + return null; + } + } + + private static class Changes extends AbstractDisabledAccess + implements ChangeAccess { + private Changes(NoChangesReviewDbWrapper wrapper, ReviewDb db) { + super(wrapper, db.changes()); + } + + @Override + public ResultSet all() { + return empty(); + } + } + + private static class ChangeMessages + extends AbstractDisabledAccess + implements ChangeMessageAccess { + private ChangeMessages(NoChangesReviewDbWrapper wrapper, ReviewDb db) { + super(wrapper, db.changeMessages()); + } + + @Override + public ResultSet byChange(Change.Id id) throws OrmException { + return empty(); + } + + @Override + public ResultSet byPatchSet(PatchSet.Id id) throws OrmException { + return empty(); + } + + @Override + public ResultSet all() throws OrmException { + return empty(); + } + } + + private static class PatchSets extends AbstractDisabledAccess + implements PatchSetAccess { + private PatchSets(NoChangesReviewDbWrapper wrapper, ReviewDb db) { + super(wrapper, db.patchSets()); + } + + @Override + public ResultSet byChange(Change.Id id) { + return empty(); + } + + @Override + public ResultSet all() { + return empty(); + } + } + + private static class PatchSetApprovals + extends AbstractDisabledAccess + implements PatchSetApprovalAccess { + private PatchSetApprovals(NoChangesReviewDbWrapper wrapper, ReviewDb db) { + super(wrapper, db.patchSetApprovals()); + } + + @Override + public ResultSet byChange(Change.Id id) { + return empty(); + } + + @Override + public ResultSet byPatchSet(PatchSet.Id id) { + return empty(); + } + + @Override + public ResultSet byPatchSetUser(PatchSet.Id patchSet, Account.Id account) { + return empty(); + } + + @Override + public ResultSet all() { + return empty(); + } + } + + private static class PatchLineComments + extends AbstractDisabledAccess + implements PatchLineCommentAccess { + private PatchLineComments(NoChangesReviewDbWrapper wrapper, ReviewDb db) { + super(wrapper, db.patchComments()); + } + + @Override + public ResultSet byChange(Change.Id id) { + return empty(); + } + + @Override + public ResultSet byPatchSet(PatchSet.Id id) { + return empty(); + } + + @Override + public ResultSet publishedByChangeFile(Change.Id id, String file) { + return empty(); + } + + @Override + public ResultSet publishedByPatchSet(PatchSet.Id patchset) { + return empty(); + } + + @Override + public ResultSet draftByPatchSetAuthor( + PatchSet.Id patchset, Account.Id author) { + return empty(); + } + + @Override + public ResultSet draftByChangeFileAuthor( + Change.Id id, String file, Account.Id author) { + return empty(); + } + + @Override + public ResultSet draftByAuthor(Account.Id author) { + return empty(); + } + + @Override + public ResultSet all() { + return empty(); + } + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/NotesMigrationSchemaFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/NotesMigrationSchemaFactory.java index 2e4065a16e..d73a5f4461 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/NotesMigrationSchemaFactory.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/NotesMigrationSchemaFactory.java @@ -14,7 +14,7 @@ package com.google.gerrit.server.schema; -import com.google.gerrit.reviewdb.server.DisabledChangesReviewDbWrapper; +import com.google.gerrit.reviewdb.server.DisallowReadFromChangesReviewDbWrapper; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gwtorm.server.OrmException; @@ -40,6 +40,34 @@ public class NotesMigrationSchemaFactory implements SchemaFactory { if (!migration.readChanges()) { return db; } - return new DisabledChangesReviewDbWrapper(db); + + // There are two levels at which this class disables access to Changes and related tables, + // corresponding to two phases of the NoteDb migration: + // + // 1. When changes are read from NoteDb but some changes might still have their primary storage + // in ReviewDb, it is generally programmer error to read changes from ReviewDb. However, + // since ReviewDb is still the primary storage for most or all changes, we still need to + // support writing to ReviewDb. This behavior is accomplished by wrapping in a + // DisallowReadFromChangesReviewDbWrapper. + // + // Some codepaths might need to be able to read from ReviewDb if they really need to, because + // they need to operate on the underlying source of truth, for example when reading a change + // to determine its primary storage. To support this, ReviewDbUtil#unwrapDb can detect and + // unwrap databases of this type. + // + // 2. After all changes have their primary storage in NoteDb, we can completely shut off access + // to the change tables. At this point in the migration, we are by definition not using the + // ReviewDb tables at all; we could even delete the tables at this point, and Gerrit would + // continue to function. + // + // This is accomplished by setting the delegate ReviewDb *underneath* DisallowReadFromChanges + // to be a complete no-op, with NoChangesReviewDbWrapper. With this wrapper, all read + // operations return no results, and write operations silently do nothing. This wrapper is + // not a public class and nobody should ever attempt to unwrap it. + + if (migration.disableChangeReviewDb()) { + db = new NoChangesReviewDbWrapper(db); + } + return new DisallowReadFromChangesReviewDbWrapper(db); } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbMode.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbMode.java index 093812f346..552f6f105f 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbMode.java +++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbMode.java @@ -21,22 +21,25 @@ import com.google.common.base.Strings; public enum NoteDbMode { /** NoteDb is disabled. */ - OFF, + OFF(false), /** Writing data to NoteDb is enabled. */ - WRITE, + WRITE(false), /** Reading and writing all data to NoteDb is enabled. */ - READ_WRITE, + READ_WRITE(true), /** Changes are created with their primary storage as NoteDb. */ - PRIMARY, + PRIMARY(true), + + /** All change tables are entirely disabled. */ + DISABLE_CHANGE_REVIEW_DB(true), /** * Run tests with NoteDb disabled, then convert ReviewDb to NoteDb and check that the results * match. */ - CHECK; + CHECK(false); private static final String ENV_VAR = "GERRIT_NOTEDB"; private static final String SYS_PROP = "gerrit.notedb"; @@ -65,6 +68,12 @@ public enum NoteDbMode { } public static boolean readWrite() { - return get() == READ_WRITE || get() == PRIMARY; + return get().readWrite; + } + + private final boolean readWrite; + + private NoteDbMode(boolean readWrite) { + this.readWrite = readWrite; } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/TestNotesMigration.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/TestNotesMigration.java index 2bab037e0d..e6a72fce43 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/testutil/TestNotesMigration.java +++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/TestNotesMigration.java @@ -26,6 +26,7 @@ public class TestNotesMigration extends NotesMigration { private volatile boolean readChanges; private volatile boolean writeChanges; private volatile PrimaryStorage changePrimaryStorage = PrimaryStorage.REVIEW_DB; + private volatile boolean disableChangeReviewDb; private volatile boolean failOnLoad; @Override @@ -45,6 +46,11 @@ public class TestNotesMigration extends NotesMigration { return changePrimaryStorage; } + @Override + public boolean disableChangeReviewDb() { + return disableChangeReviewDb; + } + // Increase visbility from superclass, as tests may want to check whether // NoteDb data is written in specific migration scenarios. @Override @@ -72,6 +78,11 @@ public class TestNotesMigration extends NotesMigration { return this; } + public TestNotesMigration setDisableChangeReviewDb(boolean disableChangeReviewDb) { + this.disableChangeReviewDb = disableChangeReviewDb; + return this; + } + public TestNotesMigration setFailOnLoad(boolean failOnLoad) { this.failOnLoad = failOnLoad; return this; @@ -87,16 +98,25 @@ public class TestNotesMigration extends NotesMigration { setWriteChanges(true); setReadChanges(true); setChangePrimaryStorage(PrimaryStorage.REVIEW_DB); + setDisableChangeReviewDb(false); break; case WRITE: setWriteChanges(true); setReadChanges(false); setChangePrimaryStorage(PrimaryStorage.REVIEW_DB); + setDisableChangeReviewDb(false); break; case PRIMARY: setWriteChanges(true); setReadChanges(true); setChangePrimaryStorage(PrimaryStorage.NOTE_DB); + setDisableChangeReviewDb(false); + break; + case DISABLE_CHANGE_REVIEW_DB: + setWriteChanges(true); + setReadChanges(true); + setChangePrimaryStorage(PrimaryStorage.NOTE_DB); + setDisableChangeReviewDb(true); break; case CHECK: case OFF: @@ -104,6 +124,7 @@ public class TestNotesMigration extends NotesMigration { setWriteChanges(false); setReadChanges(false); setChangePrimaryStorage(PrimaryStorage.REVIEW_DB); + setDisableChangeReviewDb(false); break; } return this;