opendev/gerrit
Code Issues Proposed changes

Shard refs/draft-comments/* by change instead of account ID

Browse Source 
The motivation is the same as in Ib81d462: scanning the whole
refs/draft-comments/* space (with grows without bound) when reindexing
a change is potentially too slow.

Instead of refs/draft-comments/<sharded account>/<change>, use
refs/draft-comments/<sharded change>/<account>. This is mostly
straightforward, since there are relatively few places that scan
multiple refs; most callers just use the helper method in RefNames.

Change-Id: I063e2c496751f637d968c89c1501743074fb9025
This commit is contained in:
Dave Borowitz
2016-05-02 17:44:07 -04:00
committed by Edwin Kempin
parent dba0801601
commit 66c76c9d4a
13 changed files with 114 additions and 66 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DeleteDraftPatchSetIT.java
View File

@@ -191,7 +191,7 @@ public class DeleteDraftPatchSetIT extends AbstractDaemonTest {
private Ref getDraftRef(TestAccount account, Change.Id changeId)
throws Exception {
try (Repository repo = repoManager.openRepository(allUsers)) {
return repo.exactRef(RefNames.refsDraftComments(account.id, changeId));
return repo.exactRef(RefNames.refsDraftComments(changeId, account.id));
}
}

6
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java
View File

@@ -289,14 +289,14 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
putDraft(user, id, 1, "comment by user");
ObjectId userDraftsId = getMetaRef(
allUsers, RefNames.refsDraftComments(user.getId(), id));
allUsers, RefNames.refsDraftComments(id, user.getId()));
assertThat(unwrapDb().changes().get(id).getNoteDbState()).isEqualTo(
changeMetaId.name()
+ "," + user.getId() + "=" + userDraftsId.name());
putDraft(admin, id, 2, "comment by admin");
ObjectId adminDraftsId = getMetaRef(
allUsers, RefNames.refsDraftComments(admin.getId(), id));
allUsers, RefNames.refsDraftComments(id, admin.getId()));
assertThat(admin.getId().get()).isLessThan(user.getId().get());
assertThat(unwrapDb().changes().get(id).getNoteDbState()).isEqualTo(
changeMetaId.name()
@@ -305,7 +305,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
putDraft(admin, id, 2, "revised comment by admin");
adminDraftsId = getMetaRef(
allUsers, RefNames.refsDraftComments(admin.getId(), id));
allUsers, RefNames.refsDraftComments(id, admin.getId()));
assertThat(unwrapDb().changes().get(id).getNoteDbState()).isEqualTo(
changeMetaId.name()
+ "," + admin.getId() + "=" + adminDraftsId.name()

15
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Account.java
View File

@@ -125,6 +125,21 @@ public final class Account {
Integer id = RefNames.parseShardedRefPart(name);
return id != null ? new Account.Id(id) : null;
}
/**
* Parse an Account.Id out of the last part of a ref name.
* <p>
* The input is a ref name of the form {@code ".../1234"}, where the suffix
* is a non-sharded account ID. Ref names using a sharded ID should use
* {@link #fromRefPart(String)} instead for greater safety.
*
* @param name ref name
* @return account ID, or null if not numeric.
*/
public static Id fromRefSuffix(String name) {
Integer id = RefNames.parseRefSuffix(name);
return id != null ? new Account.Id(id) : null;
}
}
@Column(id = 1)

5
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java
View File

@@ -163,6 +163,11 @@ public final class Change {
return new Change.Id(Integer.parseInt(id));
}
public static Id fromRefPart(String ref) {
Integer id = RefNames.parseShardedRefPart(ref);
return id != null ? new Change.Id(id) : null;
}
static int startIndex(String ref) {
if (ref == null || !ref.startsWith(REFS_CHANGES)) {
return -1;

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

@@ -107,15 +107,15 @@ public class RefNames {
return r.toString();
}
public static String refsDraftComments(Account.Id accountId,
Change.Id changeId) {
StringBuilder r = buildRefsPrefix(REFS_DRAFT_COMMENTS, accountId.get());
r.append(changeId.get());
public static String refsDraftComments(Change.Id changeId,
Account.Id accountId) {
StringBuilder r = buildRefsPrefix(REFS_DRAFT_COMMENTS, changeId.get());
r.append(accountId.get());
return r.toString();
}
public static String refsDraftCommentsPrefix(Account.Id accountId) {
return buildRefsPrefix(REFS_DRAFT_COMMENTS, accountId.get()).toString();
public static String refsDraftCommentsPrefix(Change.Id changeId) {
return buildRefsPrefix(REFS_DRAFT_COMMENTS, changeId.get()).toString();
}
public static String refsStarredChanges(Change.Id changeId,
@@ -217,6 +217,26 @@ public class RefNames {
return id;
}
static Integer parseRefSuffix(String name) {
if (name == null) {
return null;
}
int i = name.length();
while (i > 0) {
char c = name.charAt(i - 1);
if (c == '/') {
break;
} else if (!Character.isDigit(c)) {
return null;
}
i--;
}
if (i == 0) {
return null;
}
return Integer.valueOf(name.substring(i, name.length()));
}
private RefNames() {
}
}

7
gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/AccountTest.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.reviewdb.client;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.reviewdb.client.Account.Id.fromRef;
import static com.google.gerrit.reviewdb.client.Account.Id.fromRefPart;
import static com.google.gerrit.reviewdb.client.Account.Id.fromRefSuffix;
import org.junit.Test;
@@ -48,6 +49,12 @@ public class AccountTest {
assertThat(fromRefPart("ab/cd")).isNull();
}
@Test
public void parseRefSuffix() {
assertThat(fromRefSuffix("12/34")).isEqualTo(id(34));
assertThat(fromRefSuffix("ab/cd")).isNull();
}
private Account.Id id(int n) {
return new Account.Id(n);
}

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

@@ -16,6 +16,7 @@ package com.google.gerrit.reviewdb.client;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.reviewdb.client.RefNames.parseShardedRefPart;
import static com.google.gerrit.reviewdb.client.RefNames.parseRefSuffix;
import org.junit.Test;
@@ -39,14 +40,14 @@ public class RefNamesTest {
@Test
public void refsDraftComments() throws Exception {
assertThat(RefNames.refsDraftComments(accountId, changeId))
.isEqualTo("refs/draft-comments/23/1011123/67473");
assertThat(RefNames.refsDraftComments(changeId, accountId))
.isEqualTo("refs/draft-comments/73/67473/1011123");
}
@Test
public void refsDraftCommentsPrefix() throws Exception {
assertThat(RefNames.refsDraftCommentsPrefix(accountId))
.isEqualTo("refs/draft-comments/23/1011123/");
assertThat(RefNames.refsDraftCommentsPrefix(changeId))
.isEqualTo("refs/draft-comments/73/67473/");
}
@Test
@@ -89,4 +90,19 @@ public class RefNamesTest {
// Shard too short.
assertThat(parseShardedRefPart("1/1")).isNull();
}
@Test
public void testParseRefSuffix() throws Exception {
assertThat(parseRefSuffix("1/2/34")).isEqualTo(34);
assertThat(parseRefSuffix("/34")).isEqualTo(34);
assertThat(parseRefSuffix(null)).isNull();
assertThat(parseRefSuffix("")).isNull();
assertThat(parseRefSuffix("34")).isNull();
assertThat(parseRefSuffix("12/ab")).isNull();
assertThat(parseRefSuffix("12/a4")).isNull();
assertThat(parseRefSuffix("12/4a")).isNull();
assertThat(parseRefSuffix("a4")).isNull();
assertThat(parseRefSuffix("4a")).isNull();
}
}

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

@@ -23,7 +23,6 @@ import com.google.common.collect.ComparisonChain;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Ordering;
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.CommentInfo;
@@ -52,7 +51,6 @@ import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.NullProgressMonitor;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.RefDatabase;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.ReceiveCommand;
@@ -62,8 +60,6 @@ import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Set;
/**
* Utility functions to manipulate PatchLineComments.
@@ -169,8 +165,8 @@ public class PatchLineCommentsUtil {
}
List<PatchLineComment> comments = Lists.newArrayList();
for (String refSuffix : getDraftRefs(notes.getChangeId()).keySet()) {
Account.Id account = Account.Id.fromRefPart(refSuffix);
for (Ref ref : getDraftRefs(notes.getChangeId())) {
Account.Id account = Account.Id.fromRefSuffix(ref.getName());
if (account != null) {
comments.addAll(draftByChangeAuthor(db, notes, account));
}
@@ -199,8 +195,8 @@ public class PatchLineCommentsUtil {
List<PatchLineComment> comments = Lists.newArrayList();
comments.addAll(publishedByPatchSet(db, notes, psId));
for (String refSuffix : getDraftRefs(notes.getChangeId()).keySet()) {
Account.Id account = Account.Id.fromRefPart(refSuffix);
for (Ref ref : getDraftRefs(notes.getChangeId())) {
Account.Id account = Account.Id.fromRefSuffix(ref.getName());
if (account != null) {
comments.addAll(draftByPatchSetAuthor(db, psId, account, notes));
}
@@ -278,18 +274,25 @@ public class PatchLineCommentsUtil {
return sort(db.patchComments().draftByAuthor(author).toList());
}
Set<String> refNames =
getRefNamesAllUsers(RefNames.refsDraftCommentsPrefix(author));
List<PatchLineComment> comments = Lists.newArrayList();
for (String refName : refNames) {
Change.Id changeId = Change.Id.parse(refName);
try (Repository repo = repoManager.openRepository(allUsers)) {
for (String refName : repo.getRefDatabase()
.getRefs(RefNames.REFS_DRAFT_COMMENTS).keySet()) {
Account.Id accountId = Account.Id.fromRefSuffix(refName);
Change.Id changeId = Change.Id.fromRefPart(refName);
if (accountId == null || changeId == null) {
continue;
}
// Avoid loading notes for all affected changes just to be able to auto-
// rebuild. This is only used in a corner case in the search codepath, so
// returning slightly stale values is ok.
// rebuild. This is only used in a corner case in the search codepath,
// so returning slightly stale values is ok.
DraftCommentNotes notes =
draftFactory.createWithAutoRebuildingDisabled(changeId, author);
comments.addAll(notes.load().getComments().values());
}
} catch (IOException e) {
throw new OrmException(e);
}
return sort(comments);
}
@@ -314,7 +317,7 @@ public class PatchLineCommentsUtil {
try (Repository repo = repoManager.openRepository(allUsers);
RevWalk rw = new RevWalk(repo)) {
BatchRefUpdate bru = repo.getRefDatabase().newBatchUpdate();
for (Ref ref : getDraftRefs(repo, changeId).values()) {
for (Ref ref : getDraftRefs(repo, changeId)) {
bru.addCommand(new ReceiveCommand(
ref.getObjectId(), ObjectId.zeroId(), ref.getName()));
}
@@ -372,16 +375,7 @@ public class PatchLineCommentsUtil {
return c.getRevId();
}
private Set<String> getRefNamesAllUsers(String prefix) throws OrmException {
try (Repository repo = repoManager.openRepository(allUsers)) {
RefDatabase refDb = repo.getRefDatabase();
return refDb.getRefs(prefix).keySet();
} catch (IOException e) {
throw new OrmException(e);
}
}
public Map<String, Ref> getDraftRefs(Change.Id changeId)
public Collection<Ref> getDraftRefs(Change.Id changeId)
throws OrmException {
try (Repository repo = repoManager.openRepository(allUsers)) {
return getDraftRefs(repo, changeId);
@@ -390,17 +384,10 @@ public class PatchLineCommentsUtil {
}
}
private Map<String, Ref> getDraftRefs(Repository repo,
final Change.Id changeId) throws IOException {
final String suffix = "/" + changeId.get();
return Maps.filterKeys(
repo.getRefDatabase().getRefs(RefNames.REFS_DRAFT_COMMENTS),
new Predicate<String>() {
@Override
public boolean apply(String input) {
return input.endsWith(suffix);
}
});
private Collection<Ref> getDraftRefs(Repository repo, Change.Id changeId)
throws IOException {
return repo.getRefDatabase().getRefs(
RefNames.refsDraftCommentsPrefix(changeId)).values();
}
private static List<PatchLineComment> sort(List<PatchLineComment> comments) {

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

@@ -221,7 +221,7 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
@Override
protected String getRefName() {
return RefNames.refsDraftComments(accountId, getId());
return RefNames.refsDraftComments(getId(), accountId);
}
@Override

2
gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
View File

@@ -110,7 +110,7 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
@Override
protected String getRefName() {
return RefNames.refsDraftComments(author, getChangeId());
return RefNames.refsDraftComments(getChangeId(), author);
}
@Override

2
gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbChangeState.java
View File

@@ -177,7 +177,7 @@ public class NoteDbChangeState {
public boolean areDraftsUpToDate(Repository draftsRepo, Account.Id accountId)
throws IOException {
Ref ref = draftsRepo.exactRef(
RefNames.refsDraftComments(accountId, changeId));
RefNames.refsDraftComments(changeId, accountId));
if (ref == null) {
return !draftIds.containsKey(accountId);
}

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

@@ -258,12 +258,10 @@ public class NoteDbUpdateManager {
for (ReceiveCommand cmd : allUsersRepo.cmds.getCommands().values()) {
String r = cmd.getRefName();
if (r.startsWith(REFS_DRAFT_COMMENTS)) {
Account.Id accountId =
Account.Id.fromRefPart(r.substring(REFS_DRAFT_COMMENTS.length()));
checkDraftRef(accountId != null, r);
int s = r.lastIndexOf('/');
checkDraftRef(s >= 0 && s < r.length() - 1, r);
Change.Id changeId = Change.Id.parse(r.substring(s + 1));
Change.Id changeId =
Change.Id.fromRefPart(r.substring(REFS_DRAFT_COMMENTS.length()));
Account.Id accountId = Account.Id.fromRefSuffix(r);
checkDraftRef(accountId != null && changeId != null, r);
draftIds.put(changeId, accountId, cmd.getNewId());
}
}

6
gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
View File

@@ -1878,7 +1878,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
update.commit();
assertThat(repo.exactRef(changeMetaRef(c.getId()))).isNotNull();
String draftRef = refsDraftComments(otherUser.getAccountId(), c.getId());
String draftRef = refsDraftComments(c.getId(), otherUser.getAccountId());
assertThat(exactRefAllUsers(draftRef)).isNull();
}
@@ -1900,7 +1900,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
update.putComment(draft);
update.commit();
String draftRef = refsDraftComments(otherUser.getAccountId(), c.getId());
String draftRef = refsDraftComments(c.getId(), otherUser.getAccountId());
ObjectId old = exactRefAllUsers(draftRef);
assertThat(old).isNotNull();
@@ -2074,7 +2074,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
update.putComment(comment2);
update.commit();
String refName = refsDraftComments(otherUserId, c.getId());
String refName = refsDraftComments(c.getId(), otherUserId);
ObjectId oldDraftId = exactRefAllUsers(refName);
update = newUpdate(c, otherUser);