From dfe73dc40810c1e3dfbf07d3459a40f9662b5cfd Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 15 Apr 2016 16:34:33 +0200 Subject: [PATCH] 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 --- .../gerrit/reviewdb/client/RefNames.java | 23 +++++++------- .../gerrit/reviewdb/client/RefNamesTest.java | 8 ++--- .../gerrit/server/StarredChangesUtil.java | 30 ++++++++++--------- 3 files changed, 31 insertions(+), 30 deletions(-) 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 d2250cf960..85ebdb137c 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 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; } 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 844f893769..6797ffb2f3 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 @@ 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 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 2c49b4c0ea..64134146b8 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 @@ 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 byChange(final Change.Id changeId) + public Iterable 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() { - @Override - public boolean apply(String refPart) { - return refPart.endsWith("/" + changeId.get()); - } - }) + return FluentIterable + .from(getRefNames(RefNames.refsStarredChangesPrefix(changeId))) .transform(new Function() { @Override public Account.Id apply(String refPart) { - return Account.Id.fromRefPart(refPart); + return Account.Id.parse(refPart); } }); } - public ResultSet query(Account.Id accountId) { + @Deprecated + public ResultSet 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() { + @Override + public boolean apply(String refPart) { + return refPart.endsWith("/" + accountId.get()); + } + }) .transform(new Function() { @Override public Change.Id apply(String changeId) {