Shard refs/starred-changes refs by change ID instead of account ID 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>
diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java index d2250cf..85ebdb1 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java
@@ -95,37 +95,36 @@ 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; }
diff --git a/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java b/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java index 844f893..6797ffb 100644 --- a/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java +++ b/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java
@@ -50,14 +50,14 @@ @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
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/StarredChangesUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/StarredChangesUtil.java index 2c49b4c..6413414 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/StarredChangesUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/StarredChangesUtil.java
@@ -90,7 +90,7 @@ 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 @@ 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 @@ 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 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 @@ } }); } - 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 @@ } 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) {