opendev/gerrit
Code Issues Proposed changes

Shard refs/starred-changes refs by change ID instead of account ID

Browse Source 
If the ref is sharded by change ID, the lookup by change ID is
cheaper. This is because now we can list all refs that start with
refs/starred-changes/ plus sharded change ID and looking up refs with
a prefix that end on '/' is optimized in jgit.

Finding all changes that were starred by a user gets more expensive
since now we need to scan the complete refs/starred-changes/ namespace
for this.

The ref format is changed, because we plan to store the users that
have starred a change in the change index, and this means that we need
to look them up each time a change is reindex. If reindexing a change
would require a complete scan over the refs/starred-changes/ namespace
this would result in performance issues.

On the other hand looking up the changes that were starred by a user
can be done via the change index so that we don't need to scan the
refs/starred-changes/ namespace in this case either.

Change-Id: Ib81d46283035ef753d35fbc78684ded9f074bd1f
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2016-04-15 16:34:33 +02:00
parent 82d3bfa84c
commit dfe73dc408
3 changed files with 31 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files

23
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java
View File

@@ -95,37 +95,36 @@ public class RefNames {
public static String refsDraftComments(Account.Id accountId,
Change.Id changeId) {
StringBuilder r = buildRefsPrefix(REFS_DRAFT_COMMENTS, accountId);
StringBuilder r = buildRefsPrefix(REFS_DRAFT_COMMENTS, accountId.get());
r.append(changeId.get());
return r.toString();
}
public static String refsDraftCommentsPrefix(Account.Id accountId) {
return buildRefsPrefix(REFS_DRAFT_COMMENTS, accountId).toString();
return buildRefsPrefix(REFS_DRAFT_COMMENTS, accountId.get()).toString();
}
public static String refsStarredChanges(Account.Id accountId,
Change.Id changeId) {
StringBuilder r = buildRefsPrefix(REFS_STARRED_CHANGES, accountId);
r.append(changeId.get());
public static String refsStarredChanges(Change.Id changeId,
Account.Id accountId) {
StringBuilder r = buildRefsPrefix(REFS_STARRED_CHANGES, changeId.get());
r.append(accountId.get());
return r.toString();
}
public static String refsStarredChangesPrefix(Account.Id accountId) {
return buildRefsPrefix(REFS_STARRED_CHANGES, accountId).toString();
public static String refsStarredChangesPrefix(Change.Id changeId) {
return buildRefsPrefix(REFS_STARRED_CHANGES, changeId.get()).toString();
}
private static StringBuilder buildRefsPrefix(String prefix,
Account.Id accountId) {
private static StringBuilder buildRefsPrefix(String prefix, int id) {
StringBuilder r = new StringBuilder();
r.append(prefix);
int n = accountId.get() % 100;
int n = id % 100;
if (n < 10) {
r.append('0');
}
r.append(n);
r.append('/');
r.append(accountId.get());
r.append(id);
r.append('/');
return r;
}

8
gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java
View File

@@ -50,14 +50,14 @@ public class RefNamesTest {
@Test
public void refsStarredChanges() throws Exception {
assertThat(RefNames.refsStarredChanges(accountId, changeId))
.isEqualTo("refs/starred-changes/23/1011123/67473");
assertThat(RefNames.refsStarredChanges(changeId, accountId))
.isEqualTo("refs/starred-changes/73/67473/1011123");
}
@Test
public void refsStarredChangesPrefix() throws Exception {
assertThat(RefNames.refsStarredChangesPrefix(accountId))
.isEqualTo("refs/starred-changes/23/1011123/");
assertThat(RefNames.refsStarredChangesPrefix(changeId))
.isEqualTo("refs/starred-changes/73/67473/");
}
@Test

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

@@ -90,7 +90,7 @@ public class StarredChangesUtil {
try (Repository repo = repoManager.openRepository(allUsers);
RevWalk rw = new RevWalk(repo)) {
RefUpdate u = repo.updateRef(
RefNames.refsStarredChanges(accountId, changeId));
RefNames.refsStarredChanges(changeId, accountId));
u.setExpectedOldObjectId(ObjectId.zeroId());
u.setNewObjectId(emptyTree(repo));
u.setRefLogIdent(serverIdent);
@@ -139,7 +139,7 @@ public class StarredChangesUtil {
try (Repository repo = repoManager.openRepository(allUsers);
RevWalk rw = new RevWalk(repo)) {
RefUpdate u = repo.updateRef(
RefNames.refsStarredChanges(accountId, changeId));
RefNames.refsStarredChanges(changeId, accountId));
u.setForceUpdate(true);
u.setRefLogIdent(serverIdent);
u.setRefLogMessage("Unstar change " + changeId.get(), true);
@@ -181,7 +181,7 @@ public class StarredChangesUtil {
batchUpdate.setRefLogIdent(serverIdent);
batchUpdate.setRefLogMessage("Unstar change " + changeId.get(), true);
for (Account.Id accountId : byChange(changeId)) {
String refName = RefNames.refsStarredChanges(accountId, changeId);
String refName = RefNames.refsStarredChanges(changeId, accountId);
Ref ref = repo.getRefDatabase().getRef(refName);
batchUpdate.addCommand(new ReceiveCommand(ref.getObjectId(),
ObjectId.zeroId(), refName));
@@ -200,7 +200,7 @@ public class StarredChangesUtil {
}
}
public Iterable<Account.Id> byChange(final Change.Id changeId)
public Iterable<Account.Id> byChange(Change.Id changeId)
throws OrmException {
if (!migration.readAccounts()) {
return FluentIterable
@@ -212,22 +212,18 @@ public class StarredChangesUtil {
}
});
}
return FluentIterable.from(getRefNames(RefNames.REFS_STARRED_CHANGES))
.filter(new Predicate<String>() {
@Override
public boolean apply(String refPart) {
return refPart.endsWith("/" + changeId.get());
}
})
return FluentIterable
.from(getRefNames(RefNames.refsStarredChangesPrefix(changeId)))
.transform(new Function<String, Account.Id>() {
@Override
public Account.Id apply(String refPart) {
return Account.Id.fromRefPart(refPart);
return Account.Id.parse(refPart);
}
});
}
public ResultSet<Change.Id> query(Account.Id accountId) {
@Deprecated
public ResultSet<Change.Id> query(final Account.Id accountId) {
try {
if (!migration.readAccounts()) {
return new ChangeIdResultSet(
@@ -235,7 +231,13 @@ public class StarredChangesUtil {
}
return new ListResultSet<>(FluentIterable
.from(getRefNames(RefNames.refsStarredChangesPrefix(accountId)))
.from(getRefNames(RefNames.REFS_STARRED_CHANGES))
.filter(new Predicate<String>() {
@Override
public boolean apply(String refPart) {
return refPart.endsWith("/" + accountId.get());
}
})
.transform(new Function<String, Change.Id>() {
@Override
public Change.Id apply(String changeId) {