opendev/gerrit
Code Issues Proposed changes

Reduce number of valid NotesMigration permutations

Browse Source 
Having one boolean per reviewdb table being replaced is overkill; in
practice, even during the migration, even on googlesource.com, we are
never going to turn on reading from some Change sub-tables but not
others. However, we will very likely turn on writes for changes before
writes for, say, accounts, so we do need some granularity.

Explain the supported level of granularity in the NotesMigration
javadoc.

Change-Id: I98261fa888b3f6b71ed896e6c791aeee8324e898
This commit is contained in:
Dave Borowitz 2014-08-14 14:39:40 -07:00
parent 05a46d1794
commit 7f8addc4e6
13 changed files with 116 additions and 87 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/VisibleRefFilterIT.java
View File

@ -53,7 +53,7 @@ public class VisibleRefFilterIT extends AbstractDaemonTest {
@ConfigSuite.Config
public static Config noteDbWriteEnabled() {
Config cfg = new Config();
cfg.setBoolean("notedb", null, "write", true);
cfg.setBoolean("notedb", "changes", "write", true);
return cfg;
}
@ -201,7 +201,7 @@ public class VisibleRefFilterIT extends AbstractDaemonTest {
List<String> filtered = new ArrayList<>(expected.length);
for (String r : expected) {
if (notesMigration.write() || !r.endsWith(RefNames.META_SUFFIX)) {
if (notesMigration.writeChanges() || !r.endsWith(RefNames.META_SUFFIX)) {
filtered.add(r);
}
}

6
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeMessagesIT.java
View File

@ -25,6 +25,7 @@ import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.testutil.ConfigSuite;
import org.eclipse.jgit.api.errors.GitAPIException;
@ -48,10 +49,7 @@ public class ChangeMessagesIT extends AbstractDaemonTest {
@ConfigSuite.Config
public static Config noteDbEnabled() {
Config cfg = new Config();
cfg.setBoolean("notedb", null, "write", true);
cfg.setBoolean("notedb", "changeMessages", "read", true);
return cfg;
return NotesMigration.allEnabledConfig();
}
@Before

6
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java
View File

@ -26,6 +26,7 @@ import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.common.Comment;
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.testutil.ConfigSuite;
import com.google.gson.reflect.TypeToken;
@ -43,10 +44,7 @@ import java.util.Map;
public class CommentsIT extends AbstractDaemonTest {
@ConfigSuite.Config
public static Config noteDbEnabled() {
Config cfg = new Config();
cfg.setBoolean("notedb", null, "write", true);
cfg.setBoolean("notedb", "comments", "read", true);
return cfg;
return NotesMigration.allEnabledConfig();
}
@Test

6
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/LabelTypeIT.java
View File

@ -29,6 +29,7 @@ import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.testutil.ConfigSuite;
import com.google.inject.Inject;
@ -42,10 +43,7 @@ import org.junit.Test;
public class LabelTypeIT extends AbstractDaemonTest {
@ConfigSuite.Config
public static Config noteDbEnabled() {
Config cfg = new Config();
cfg.setBoolean("notedb", null, "write", true);
cfg.setBoolean("notedb", "patchSetApprovals", "read", true);
return cfg;
return NotesMigration.allEnabledConfig();
}
@Inject

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

@ -119,7 +119,7 @@ public class ApprovalsUtil {
*/
public ImmutableSetMultimap<ReviewerState, Account.Id> getReviewers(
ReviewDb db, ChangeNotes notes) throws OrmException {
if (!migration.readPatchSetApprovals()) {
if (!migration.readChanges()) {
return getReviewers(db.patchSetApprovals().byChange(notes.getChangeId()));
}
return notes.load().getReviewers();
@ -137,7 +137,7 @@ public class ApprovalsUtil {
public ImmutableSetMultimap<ReviewerState, Account.Id> getReviewers(
ChangeNotes notes, Iterable<PatchSetApproval> allApprovals)
throws OrmException {
if (!migration.readPatchSetApprovals()) {
if (!migration.readChanges()) {
return getReviewers(allApprovals);
}
return notes.load().getReviewers();
@ -269,7 +269,7 @@ public class ApprovalsUtil {
public ListMultimap<PatchSet.Id, PatchSetApproval> byChange(ReviewDb db,
ChangeNotes notes) throws OrmException {
if (!migration.readPatchSetApprovals()) {
if (!migration.readChanges()) {
ImmutableListMultimap.Builder<PatchSet.Id, PatchSetApproval> result =
ImmutableListMultimap.builder();
for (PatchSetApproval psa
@ -283,7 +283,7 @@ public class ApprovalsUtil {
public Iterable<PatchSetApproval> byPatchSet(ReviewDb db, ChangeControl ctl,
PatchSet.Id psId) throws OrmException {
if (!migration.readPatchSetApprovals()) {
if (!migration.readChanges()) {
return sortApprovals(db.patchSetApprovals().byPatchSet(psId));
}
return copier.getForPatchSet(db, ctl, psId);
@ -292,7 +292,7 @@ public class ApprovalsUtil {
public Iterable<PatchSetApproval> byPatchSetUser(ReviewDb db,
ChangeControl ctl, PatchSet.Id psId, Account.Id accountId)
throws OrmException {
if (!migration.readPatchSetApprovals()) {
if (!migration.readChanges()) {
return sortApprovals(
db.patchSetApprovals().byPatchSetUser(psId, accountId));
}

4
gerrit-server/src/main/java/com/google/gerrit/server/ChangeMessagesUtil.java
View File

@ -50,7 +50,7 @@ public class ChangeMessagesUtil {
}
public List<ChangeMessage> byChange(ReviewDb db, ChangeNotes notes) throws OrmException {
if (!migration.readChangeMessages()) {
if (!migration.readChanges()) {
return
sortChangeMessages(db.changeMessages().byChange(notes.getChangeId()));
} else {
@ -60,7 +60,7 @@ public class ChangeMessagesUtil {
public List<ChangeMessage> byPatchSet(ReviewDb db, ChangeNotes notes,
PatchSet.Id psId) throws OrmException {
if (!migration.readChangeMessages()) {
if (!migration.readChanges()) {
return sortChangeMessages(db.changeMessages().byPatchSet(psId));
}
return notes.load().getChangeMessages().get(psId);

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

@ -80,7 +80,7 @@ public class PatchLineCommentsUtil {
public Optional<PatchLineComment> get(ReviewDb db, ChangeNotes notes,
PatchLineComment.Key key) throws OrmException {
if (!migration.readComments()) {
if (!migration.readChanges()) {
return Optional.fromNullable(db.patchComments().get(key));
}
for (PatchLineComment c : publishedByChange(db, notes)) {
@ -98,7 +98,7 @@ public class PatchLineCommentsUtil {
public List<PatchLineComment> publishedByChange(ReviewDb db,
ChangeNotes notes) throws OrmException {
if (!migration.readComments()) {
if (!migration.readChanges()) {
return sort(byCommentStatus(
db.patchComments().byChange(notes.getChangeId()), Status.PUBLISHED));
}
@ -112,7 +112,7 @@ public class PatchLineCommentsUtil {
public List<PatchLineComment> draftByChange(ReviewDb db,
ChangeNotes notes) throws OrmException {
if (!migration.readComments()) {
if (!migration.readChanges()) {
return sort(byCommentStatus(
db.patchComments().byChange(notes.getChangeId()), Status.DRAFT));
}
@ -143,7 +143,7 @@ public class PatchLineCommentsUtil {
public List<PatchLineComment> byPatchSet(ReviewDb db,
ChangeNotes notes, PatchSet.Id psId) throws OrmException {
if (!migration.readComments()) {
if (!migration.readChanges()) {
return sort(db.patchComments().byPatchSet(psId).toList());
}
List<PatchLineComment> comments = Lists.newArrayList();
@ -161,7 +161,7 @@ public class PatchLineCommentsUtil {
public List<PatchLineComment> publishedByChangeFile(ReviewDb db,
ChangeNotes notes, Change.Id changeId, String file) throws OrmException {
if (!migration.readComments()) {
if (!migration.readChanges()) {
return sort(
db.patchComments().publishedByChangeFile(changeId, file).toList());
}
@ -176,7 +176,7 @@ public class PatchLineCommentsUtil {
public List<PatchLineComment> publishedByPatchSet(ReviewDb db,
ChangeNotes notes, PatchSet.Id psId) throws OrmException {
if (!migration.readComments()) {
if (!migration.readChanges()) {
return sort(
db.patchComments().publishedByPatchSet(psId).toList());
}
@ -190,7 +190,7 @@ public class PatchLineCommentsUtil {
public List<PatchLineComment> draftByPatchSetAuthor(ReviewDb db,
PatchSet.Id psId, Account.Id author, ChangeNotes notes)
throws OrmException {
if (!migration.readComments()) {
if (!migration.readChanges()) {
return sort(
db.patchComments().draftByPatchSetAuthor(psId, author).toList());
}
@ -204,7 +204,7 @@ public class PatchLineCommentsUtil {
public List<PatchLineComment> draftByChangeFileAuthor(ReviewDb db,
ChangeNotes notes, String file, Account.Id author)
throws OrmException {
if (!migration.readComments()) {
if (!migration.readChanges()) {
return sort(
db.patchComments()
.draftByChangeFileAuthor(notes.getChangeId(), file, author)
@ -221,7 +221,7 @@ public class PatchLineCommentsUtil {
public List<PatchLineComment> draftByChangeAuthor(ReviewDb db,
ChangeNotes notes, Account.Id author)
throws OrmException {
if (!migration.readComments()) {
if (!migration.readChanges()) {
return sort(db.patchComments().byChange(notes.getChangeId()).toList());
}
List<PatchLineComment> comments = Lists.newArrayList();
@ -232,7 +232,7 @@ public class PatchLineCommentsUtil {
public List<PatchLineComment> draftByAuthor(ReviewDb db,
Account.Id author) throws OrmException {
if (!migration.readComments()) {
if (!migration.readChanges()) {
return sort(db.patchComments().draftByAuthor(author).toList());
}

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

@ -92,7 +92,7 @@ public abstract class AbstractChangeUpdate extends VersionedMetaData {
}
private void load() throws IOException {
if (migration.write() && getRevision() == null) {
if (migration.writeChanges() && getRevision() == null) {
Repository repo = repoManager.openMetadataRepository(getProjectName());
try {
load(repo);
@ -119,7 +119,7 @@ public abstract class AbstractChangeUpdate extends VersionedMetaData {
public BatchMetaDataUpdate openUpdateInBatch(BatchRefUpdate bru)
throws IOException {
if (migration.write()) {
if (migration.writeChanges()) {
load();
MetaDataUpdate md =
updateFactory.create(getProjectName(), getUser(), bru);

12
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java
View File

@ -105,7 +105,7 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
verifyComment(c);
checkArgument(c.getStatus() == Status.DRAFT,
"Cannot insert a published comment into a ChangeDraftUpdate");
if (migration.readComments()) {
if (migration.readChanges()) {
checkArgument(!changeNotes.containsComment(c),
"A comment already exists with the same key,"
+ " so the following comment cannot be inserted: %s", c);
@ -131,7 +131,7 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
// the update will fail. However, this is an acceptable risk since the
// caller wanted the comment deleted anyways, so the end result will be the
// same either way.
if (migration.readComments()) {
if (migration.readChanges()) {
checkArgument(draftNotes.load().containsComment(c),
"Cannot update this comment because it didn't exist previously");
}
@ -141,12 +141,12 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
public void deleteComment(PatchLineComment c) throws OrmException {
verifyComment(c);
// See the comment above about potential race condition.
if (migration.readComments()) {
if (migration.readChanges()) {
checkArgument(draftNotes.load().containsComment(c),
"Cannot delete this comment because it didn't previously exist as a"
+ " draft");
}
if (migration.write()) {
if (migration.writeChanges()) {
if (draftNotes.load().containsComment(c)) {
deleteComments.add(c);
}
@ -170,7 +170,7 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
checkArgument(getCommentPsId(comment).equals(psId),
"Comment on %s does not match configured patch set %s",
getCommentPsId(comment), psId);
if (migration.write()) {
if (migration.writeChanges()) {
checkArgument(comment.getRevId() != null);
}
checkArgument(comment.getAuthor().equals(accountId),
@ -276,7 +276,7 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
public void writeCommit(BatchMetaDataUpdate batch)
throws OrmException, IOException {
CommitBuilder builder = new CommitBuilder();
if (migration.write()) {
if (migration.writeChanges()) {
AtomicBoolean removedAllComments = new AtomicBoolean();
ObjectId treeId = storeCommentsInNotes(removedAllComments);
if (treeId != null) {

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

@ -239,7 +239,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
if (notes == null) {
notes = getChangeNotes().load();
}
if (migration.readComments()) {
if (migration.readChanges()) {
checkArgument(!notes.containsComment(c),
"A comment already exists with the same key as the following comment,"
+ " so we cannot insert this comment: %s", c);
@ -265,7 +265,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
// is on and migration.readComments is off because we will not be able to
// verify that the comment didn't already exist as a published comment
// since we don't have a ReviewDb.
if (migration.readComments()) {
if (migration.readChanges()) {
checkArgument(!notes.containsCommentPublished(c),
"Cannot update a comment that has already been published and saved");
}
@ -288,7 +288,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
}
// See comment above in upsertPublishedComment() about potential risk with
// this check.
if (migration.readComments()) {
if (migration.readChanges()) {
checkArgument(!notes.containsCommentPublished(c),
"Cannot update a comment that has already been published and saved");
}
@ -406,7 +406,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
public void writeCommit(BatchMetaDataUpdate batch) throws OrmException,
IOException {
CommitBuilder builder = new CommitBuilder();
if (migration.write()) {
if (migration.writeChanges()) {
ObjectId treeId = storeCommentsInNotes();
if (treeId != null) {
builder.setTreeId(treeId);

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

@ -14,66 +14,103 @@
package com.google.gerrit.server.notedb;
import static com.google.common.base.Preconditions.checkArgument;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import org.eclipse.jgit.lib.Config;
import java.util.HashSet;
import java.util.Set;
/**
* Holds the current state of the notes DB migration.
* Holds the current state of the NoteDb migration.
* <p>
* During a transitional period, different subsets of the former gwtorm DB
* functionality may be enabled on the site, possibly only for reading or
* writing.
* The migration will proceed one root entity type at a time. A <em>root
* entity</em> is an entity stored in ReviewDb whose key's
* {@code getParentKey()} method returns null. For an example of the entity
* hierarchy rooted at Change, see the diagram in
* {@code com.google.gerrit.reviewdb.client.Change}.
* <p>
* During a transitional period, each root entity group from ReviewDb may be
* either <em>written to</em> or <em>both written to and read from</em> NoteDb.
* <p>
* This class controls the state of the migration according to options in
* {@code gerrit.config}. In general, any changes to these options should only
* be made by adventurous administrators, who know what they're doing, on
* non-production data, for the purposes of testing the NoteDb implementation.
* Changing options quite likely requires re-running {@code RebuildNoteDb}. For
* these reasons, the options remain undocumented.
*/
@Singleton
public class NotesMigration {
public static NotesMigration allEnabled() {
Config cfg = new Config();
cfg.setBoolean("notedb", null, "write", true);
cfg.setBoolean("notedb", "patchSetApprovals", "read", true);
cfg.setBoolean("notedb", "changeMessages", "read", true);
cfg.setBoolean("notedb", "comments", "read", true);
return new NotesMigration(cfg);
private static enum Table {
CHANGES;
private String key() {
return name().toLowerCase();
}
}
private final boolean write;
private final boolean readPatchSetApprovals;
private final boolean readChangeMessages;
private final boolean readComments;
private static final String NOTEDB = "notedb";
private static final String READ = "read";
private static final String WRITE = "write";
private static void checkConfig(Config cfg) {
Set<String> keys = new HashSet<>();
for (Table t : Table.values()) {
keys.add(t.key());
}
for (String t : cfg.getSubsections(NOTEDB)) {
checkArgument(keys.contains(t.toLowerCase()),
"invalid notedb table: %s", t);
for (String key : cfg.getNames(NOTEDB, t)) {
String lk = key.toLowerCase();
checkArgument(lk.equals(WRITE) || lk.equals(READ),
"invalid notedb key: %s.%s", t, key);
}
boolean write = cfg.getBoolean(NOTEDB, t, WRITE, false);
boolean read = cfg.getBoolean(NOTEDB, t, READ, false);
checkArgument(!(read && !write),
"must have write enabled when read enabled: %s", t);
}
}
public static NotesMigration allEnabled() {
return new NotesMigration(allEnabledConfig());
}
public static Config allEnabledConfig() {
Config cfg = new Config();
for (Table t : Table.values()) {
cfg.setBoolean(NOTEDB, t.key(), WRITE, true);
cfg.setBoolean(NOTEDB, t.key(), READ, true);
}
return cfg;
}
private final boolean writeChanges;
private final boolean readChanges;
@Inject
NotesMigration(@GerritServerConfig Config cfg) {
write = cfg.getBoolean("notedb", null, "write", false);
readPatchSetApprovals =
cfg.getBoolean("notedb", "patchSetApprovals", "read", false);
readChangeMessages =
cfg.getBoolean("notedb", "changeMessages", "read", false);
readComments =
cfg.getBoolean("notedb", "comments", "read", false);
checkConfig(cfg);
writeChanges = cfg.getBoolean(NOTEDB, Table.CHANGES.key(), WRITE, false);
readChanges = cfg.getBoolean(NOTEDB, Table.CHANGES.key(), READ, false);
}
public boolean enabled() {
return readChangeMessages()
|| readComments()
|| readPatchSetApprovals()
|| write();
return writeChanges()
|| readChanges();
}
public boolean write() {
return write;
public boolean writeChanges() {
return writeChanges;
}
public boolean readPatchSetApprovals() {
return readPatchSetApprovals;
}
public boolean readChangeMessages() {
return readChangeMessages;
}
public boolean readComments() {
return readComments;
public boolean readChanges() {
return readChanges;
}
}

4
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
View File

@ -79,7 +79,7 @@ public class ChangeData {
}
if (!missing.isEmpty()) {
ChangeData first = missing.values().iterator().next();
if (!first.notesMigration.readPatchSetApprovals()) {
if (!first.notesMigration.readChanges()) {
ReviewDb db = missing.values().iterator().next().db;
for (Change change : db.changes().get(missing.keySet())) {
missing.get(change.getId()).change = change;
@ -120,7 +120,7 @@ public class ChangeData {
throws OrmException {
List<ResultSet<PatchSetApproval>> pending = Lists.newArrayList();
for (ChangeData cd : changes) {
if (!cd.notesMigration.readPatchSetApprovals()) {
if (!cd.notesMigration.readChanges()) {
if (cd.currentApprovals == null) {
pending.add(cd.db.patchSetApprovals()
.byPatchSet(cd.change().currentPatchSetId()));

6
gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java
View File

@ -61,6 +61,7 @@ import com.google.gerrit.server.git.GitModule;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.util.TimeUtil;
@ -104,10 +105,7 @@ public class CommentsTest {
@ConfigSuite.Config
public static @GerritServerConfig Config noteDbEnabled() {
@GerritServerConfig Config cfg = new Config();
cfg.setBoolean("notedb", null, "write", true);
cfg.setBoolean("notedb", "comments", "read", true);
return cfg;
return NotesMigration.allEnabledConfig();
}
private Injector injector;