opendev/gerrit
Code Issues Proposed changes

NoteDb: Consider starred changes part of the Accounts table

Browse Source 
A StarredChange is a simple pair of (Account.Id, Change.Id), and will
ultimately be stored in NoteDb in the All-Users table. It turns out it
is not even stored under the Change entity group in the
googlesource.com backend, meaning among other things that starred
changes can't be updated in the same transaction as a Change entity.

Add a new value ACCOUNTS to the NoteDbTable enum and teach
NotesMigration to understand this table. Note that in practice we've
started migrating Accounts to All-Users without the use of
NotesMigration, which is why we haven't done this yet. It's even
possible that we will migrate StarredChanges in the same way, but
that's a decision for later. This change is the minimal work necessary
to stop considering StarredChanges part of the changes migration.

Use the new migration bits within StarredChangesUtil. Remove support
for starred changes from ChangeRebuilder (no longer applicable) and
RebuildNoteDb (not implemented yet).

Change-Id: I84fa8b78d308b7dcd6b7592cea8898a93135cd2c
This commit is contained in:
Dave Borowitz
2016-03-29 14:41:15 -04:00
parent 78fb87158d
commit f71b304786
8 changed files with 40 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
gerrit-pgm/src/main/java/com/google/gerrit/pgm/RebuildNoteDb.java
View File

@@ -136,7 +136,6 @@ public class RebuildNoteDb extends SiteProgram {
try (Repository allUsersRepo =
repoManager.openMetadataRepository(allUsersName)) {
deleteRefs(RefNames.REFS_DRAFT_COMMENTS, allUsersRepo);
deleteRefs(RefNames.REFS_STARRED_CHANGES, allUsersRepo);
for (Project.NameKey project : changesByProject.keySet()) {
try {
List<ListenableFuture<?>> futures = Lists.newArrayList();

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

@@ -84,7 +84,7 @@ public class StarredChangesUtil {
dbProvider.get().starredChanges()
.insert(Collections.singleton(new StarredChange(
new StarredChange.Key(accountId, changeId))));
if (!migration.writeChanges()) {
if (!migration.writeAccounts()) {
return;
}
try (Repository repo = repoManager.openMetadataRepository(allUsers);
@@ -133,7 +133,7 @@ public class StarredChangesUtil {
dbProvider.get().starredChanges()
.delete(Collections.singleton(new StarredChange(
new StarredChange.Key(accountId, changeId))));
if (!migration.writeChanges()) {
if (!migration.writeAccounts()) {
return;
}
try (Repository repo = repoManager.openMetadataRepository(allUsers);
@@ -171,7 +171,7 @@ public class StarredChangesUtil {
public void unstarAll(Change.Id changeId) throws OrmException {
dbProvider.get().starredChanges().delete(
dbProvider.get().starredChanges().byChange(changeId));
if (!migration.writeChanges()) {
if (!migration.writeAccounts()) {
return;
}
try (Repository repo = repoManager.openMetadataRepository(allUsers);
@@ -202,7 +202,7 @@ public class StarredChangesUtil {
public Iterable<Account.Id> byChange(final Change.Id changeId)
throws OrmException {
if (!migration.readChanges()) {
if (!migration.readAccounts()) {
return FluentIterable
.from(dbProvider.get().starredChanges().byChange(changeId))
.transform(new Function<StarredChange, Account.Id>() {
@@ -229,7 +229,7 @@ public class StarredChangesUtil {
public ResultSet<Change.Id> query(Account.Id accountId) {
try {
if (!migration.readChanges()) {
if (!migration.readAccounts()) {
return new ChangeIdResultSet(
dbProvider.get().starredChanges().byAccount(accountId));
}

3
gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftChangeOp.java
View File

@@ -114,6 +114,9 @@ class DeleteDraftChangeOp extends BatchUpdate.Op {
db.patchSetApprovals().delete(db.patchSetApprovals().byChange(id));
db.patchSets().delete(db.patchSets().byChange(id));
db.changeMessages().delete(db.changeMessages().byChange(id));
// Non-atomic operation on Accounts table; not much we can do to make it
// atomic.
starredChangesUtil.unstarAll(id);
ctx.deleteChange();

24
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java
View File

@@ -40,8 +40,6 @@ import com.google.gerrit.reviewdb.client.PatchLineComment;
import com.google.gerrit.reviewdb.client.PatchLineComment.Status;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.client.StarredChange;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
@@ -60,9 +58,7 @@ import com.google.inject.util.Providers;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.InvalidObjectIdException;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
@@ -221,8 +217,6 @@ public class ChangeRebuilder {
flushEventsToDraftUpdate(db, manager, plcel, change);
}
createStarredChangesRefs(db, changeId, manager.getAllUsersCommands(),
manager.getAllUsersRepo());
manager.execute();
}
@@ -261,24 +255,6 @@ public class ChangeRebuilder {
events.clear();
}
private void createStarredChangesRefs(ReviewDb db, Change.Id changeId,
ChainedReceiveCommands allUsersCmds, Repository allUsersRepo)
throws IOException, OrmException {
ObjectId emptyTree = emptyTree(allUsersRepo);
for (StarredChange starred : db.starredChanges().byChange(changeId)) {
allUsersCmds.add(new ReceiveCommand(ObjectId.zeroId(), emptyTree,
RefNames.refsStarredChanges(starred.getAccountId(), changeId)));
}
}
private static ObjectId emptyTree(Repository repo) throws IOException {
try (ObjectInserter oi = repo.newObjectInserter()) {
ObjectId id = oi.insert(Constants.OBJ_TREE, new byte[] {});
oi.flush();
return id;
}
}
private List<HashtagsEvent> getHashtagsEvents(Change change,
NoteDbUpdateManager manager) throws IOException, ConfigInvalidException {
String refName = ChangeNoteUtil.changeRefName(change.getId());

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

@@ -15,6 +15,7 @@
package com.google.gerrit.server.notedb;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.gerrit.server.notedb.NoteDbTable.ACCOUNTS;
import static com.google.gerrit.server.notedb.NoteDbTable.CHANGES;
import com.google.gerrit.server.config.GerritServerConfig;
@@ -81,12 +82,16 @@ public class ConfigNotesMigration extends NotesMigration {
private final boolean writeChanges;
private final boolean readChanges;
private final boolean writeAccounts;
private final boolean readAccounts;
@Inject
ConfigNotesMigration(@GerritServerConfig Config cfg) {
checkConfig(cfg);
writeChanges = cfg.getBoolean(NOTE_DB, CHANGES.key(), WRITE, false);
readChanges = cfg.getBoolean(NOTE_DB, CHANGES.key(), READ, false);
writeAccounts = cfg.getBoolean(NOTE_DB, ACCOUNTS.key(), WRITE, false);
readAccounts = cfg.getBoolean(NOTE_DB, ACCOUNTS.key(), READ, false);
}
@Override
@@ -98,4 +103,14 @@ public class ConfigNotesMigration extends NotesMigration {
public boolean readChanges() {
return readChanges;
}
@Override
public boolean writeAccounts() {
return writeAccounts;
}
@Override
public boolean readAccounts() {
return readAccounts;
}
}

1
gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbTable.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.server.notedb;
enum NoteDbTable {
ACCOUNTS,
CHANGES;
String key() {

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

@@ -38,8 +38,12 @@ public abstract class NotesMigration {
public abstract boolean writeChanges();
public abstract boolean readAccounts();
public abstract boolean writeAccounts();
public boolean enabled() {
return writeChanges()
|| readChanges();
return writeChanges() || readChanges()
|| writeAccounts() || readAccounts();
}
}

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

@@ -33,6 +33,16 @@ public class TestNotesMigration extends NotesMigration {
return writeChanges;
}
@Override
public boolean readAccounts() {
return false;
}
@Override
public boolean writeAccounts() {
return false;
}
public TestNotesMigration setReadChanges(boolean readChanges) {
this.readChanges = readChanges;
return this;