opendev/gerrit
Code Issues Proposed changes

Optimize BranchesCollection.parse for large repositories

Browse Source 
Repositories with excessively large number of branches suffer from
very slow times looking up a branch in either the REST API, or the
internal GerritApi.projects().project(p).branch(b) API that may be
commonly used by plugins.

Remove BranchInfo from BranchResource; it only needs to know the name
and the revision.

Defer construction of the full BranchInfo to GetBranch.apply(), where
the action map and additional data is actually required.

During BranchesCollection.parse(), verify the branch exists using the
single-ref exactRef() operation, and check the caller has access to
the branch according to PermissionBackend.

This avoids considering every single branch in the repository when the
operation only needs information on a single branch to continue.

Change-Id: If2284dbcc45dcac8b2467426c5719e39be83e30a
This commit is contained in:
Shawn Pearce 2017-07-01 16:09:17 -07:00
parent 6467883f3a
commit 41c36201f9
6 changed files with 87 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
gerrit-server/src/main/java/com/google/gerrit/server/api/projects/BranchApiImpl.java
View File

@ -23,12 +23,14 @@ import com.google.gerrit.extensions.api.projects.ReflogEntryInfo;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.BranchResource;
import com.google.gerrit.server.project.BranchesCollection;
import com.google.gerrit.server.project.CreateBranch;
import com.google.gerrit.server.project.DeleteBranch;
import com.google.gerrit.server.project.FileResource;
import com.google.gerrit.server.project.FilesCollection;
import com.google.gerrit.server.project.GetBranch;
import com.google.gerrit.server.project.GetContent;
import com.google.gerrit.server.project.GetReflog;
import com.google.gerrit.server.project.ProjectResource;
@ -46,6 +48,7 @@ public class BranchApiImpl implements BranchApi {
private final CreateBranch.Factory createBranchFactory;
private final DeleteBranch deleteBranch;
private final FilesCollection filesCollection;
private final GetBranch getBranch;
private final GetContent getContent;
private final GetReflog getReflog;
private final String ref;
@ -57,6 +60,7 @@ public class BranchApiImpl implements BranchApi {
CreateBranch.Factory createBranchFactory,
DeleteBranch deleteBranch,
FilesCollection filesCollection,
GetBranch getBranch,
GetContent getContent,
GetReflog getReflog,
@Assisted ProjectResource project,
@ -65,6 +69,7 @@ public class BranchApiImpl implements BranchApi {
this.createBranchFactory = createBranchFactory;
this.deleteBranch = deleteBranch;
this.filesCollection = filesCollection;
this.getBranch = getBranch;
this.getContent = getContent;
this.getReflog = getReflog;
this.project = project;
@ -84,7 +89,7 @@ public class BranchApiImpl implements BranchApi {
@Override
public BranchInfo get() throws RestApiException {
try {
return resource().getBranchInfo();
return getBranch.apply(resource());
} catch (Exception e) {
throw asRestApiException("Cannot read branch", e);
}
@ -113,12 +118,13 @@ public class BranchApiImpl implements BranchApi {
public List<ReflogEntryInfo> reflog() throws RestApiException {
try {
return getReflog.apply(resource());
} catch (IOException e) {
} catch (IOException | PermissionBackendException e) {
throw new RestApiException("Cannot retrieve reflog", e);
}
}
private BranchResource resource() throws RestApiException, IOException {
private BranchResource resource()
throws RestApiException, IOException, PermissionBackendException {
return branches.parse(project, IdString.fromDecoded(ref));
}
}

20
gerrit-server/src/main/java/com/google/gerrit/server/project/BranchResource.java
View File

@ -14,37 +14,35 @@
package com.google.gerrit.server.project;
import com.google.gerrit.extensions.api.projects.BranchInfo;
import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.inject.TypeLiteral;
import org.eclipse.jgit.lib.Ref;
public class BranchResource extends RefResource {
public static final TypeLiteral<RestView<BranchResource>> BRANCH_KIND =
new TypeLiteral<RestView<BranchResource>>() {};
private final BranchInfo branchInfo;
private final String refName;
private final String revision;
public BranchResource(ProjectControl control, BranchInfo branchInfo) {
public BranchResource(ProjectControl control, Ref ref) {
super(control);
this.branchInfo = branchInfo;
}
public BranchInfo getBranchInfo() {
return branchInfo;
this.refName = ref.getName();
this.revision = ref.getObjectId() != null ? ref.getObjectId().name() : null;
}
public Branch.NameKey getBranchKey() {
return new Branch.NameKey(getNameKey(), branchInfo.ref);
return new Branch.NameKey(getNameKey(), refName);
}
@Override
public String getRef() {
return branchInfo.ref;
return refName;
}
@Override
public String getRevision() {
return branchInfo.revision;
return revision;
}
}

53
gerrit-server/src/main/java/com/google/gerrit/server/project/BranchesCollection.java
View File

@ -14,36 +14,51 @@
package com.google.gerrit.server.project;
import com.google.gerrit.extensions.api.projects.BranchInfo;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.restapi.AcceptsCreate;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ChildCollection;
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.RefPermission;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.List;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
@Singleton
public class BranchesCollection
implements ChildCollection<ProjectResource, BranchResource>, AcceptsCreate<ProjectResource> {
private final DynamicMap<RestView<BranchResource>> views;
private final Provider<ListBranches> list;
private final PermissionBackend permissionBackend;
private final Provider<CurrentUser> user;
private final GitRepositoryManager repoManager;
private final CreateBranch.Factory createBranchFactory;
@Inject
BranchesCollection(
DynamicMap<RestView<BranchResource>> views,
Provider<ListBranches> list,
PermissionBackend permissionBackend,
Provider<CurrentUser> user,
GitRepositoryManager repoManager,
CreateBranch.Factory createBranchFactory) {
this.views = views;
this.list = list;
this.permissionBackend = permissionBackend;
this.user = user;
this.repoManager = repoManager;
this.createBranchFactory = createBranchFactory;
}
@ -54,18 +69,28 @@ public class BranchesCollection
@Override
public BranchResource parse(ProjectResource parent, IdString id)
throws ResourceNotFoundException, IOException, BadRequestException {
String branchName = id.get();
if (!branchName.equals(Constants.HEAD)) {
branchName = RefNames.fullName(branchName);
}
List<BranchInfo> branches = list.get().apply(parent);
for (BranchInfo b : branches) {
if (branchName.equals(b.ref)) {
return new BranchResource(parent.getControl(), b);
throws ResourceNotFoundException, IOException, PermissionBackendException {
Project.NameKey project = parent.getNameKey();
try (Repository repo = repoManager.openRepository(project)) {
Ref ref = repo.exactRef(RefNames.fullName(id.get()));
if (ref == null) {
throw new ResourceNotFoundException(id);
}
// ListBranches checks the target of a symbolic reference to determine access
// rights on the symbolic reference itself. This check prevents seeing a hidden
// branch simply because the symbolic reference name was visible.
permissionBackend
.user(user)
.project(project)
.ref(ref.isSymbolic() ? ref.getTarget().getName() : ref.getName())
.check(RefPermission.READ);
return new BranchResource(parent.getControl(), ref);
} catch (AuthException notAllowed) {
throw new ResourceNotFoundException(id);
} catch (RepositoryNotFoundException noRepo) {
throw new ResourceNotFoundException();
}
throw new ResourceNotFoundException();
}
@Override

14
gerrit-server/src/main/java/com/google/gerrit/server/project/GetBranch.java
View File

@ -15,14 +15,24 @@
package com.google.gerrit.server.project;
import com.google.gerrit.extensions.api.projects.BranchInfo;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
@Singleton
public class GetBranch implements RestReadView<BranchResource> {
private final Provider<ListBranches> list;
@Inject
GetBranch(Provider<ListBranches> list) {
this.list = list;
}
@Override
public BranchInfo apply(BranchResource rsrc) {
return rsrc.getBranchInfo();
public BranchInfo apply(BranchResource rsrc) throws ResourceNotFoundException, IOException {
return list.get().toBranchInfo(rsrc);
}
}

18
gerrit-server/src/main/java/com/google/gerrit/server/project/ListBranches.java
View File

@ -15,6 +15,7 @@
package com.google.gerrit.server.project;
import com.google.common.collect.ComparisonChain;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Sets;
import com.google.gerrit.extensions.api.projects.BranchInfo;
import com.google.gerrit.extensions.common.ActionInfo;
@ -128,6 +129,18 @@ public class ListBranches implements RestReadView<ProjectResource> {
.filter(allBranches(rsrc));
}
BranchInfo toBranchInfo(BranchResource rsrc) throws IOException, ResourceNotFoundException {
try (Repository db = repoManager.openRepository(rsrc.getNameKey())) {
Ref r = db.exactRef(rsrc.getRef());
if (r == null) {
throw new ResourceNotFoundException();
}
return toBranchInfo(rsrc, ImmutableList.of(r)).get(0);
} catch (RepositoryNotFoundException noRepo) {
throw new ResourceNotFoundException();
}
}
private List<BranchInfo> allBranches(ProjectResource rsrc)
throws IOException, ResourceNotFoundException {
List<Ref> refs;
@ -142,7 +155,10 @@ public class ListBranches implements RestReadView<ProjectResource> {
} catch (RepositoryNotFoundException noGitRepository) {
throw new ResourceNotFoundException();
}
return toBranchInfo(rsrc, refs);
}
private List<BranchInfo> toBranchInfo(ProjectResource rsrc, List<Ref> refs) {
Set<String> targets = Sets.newHashSetWithExpectedSize(1);
for (Ref ref : refs) {
if (ref.isSymbolic()) {
@ -213,7 +229,7 @@ public class ListBranches implements RestReadView<ProjectResource> {
info.canDelete =
!targets.contains(ref.getName()) && perm.testOrFalse(RefPermission.DELETE) ? true : null;
BranchResource rsrc = new BranchResource(pctl, info);
BranchResource rsrc = new BranchResource(pctl, ref);
for (UiAction.Description d : uiActions.from(branchViews, rsrc)) {
if (info.actions == null) {
info.actions = new TreeMap<>();

3
gerrit-server/src/main/java/com/google/gerrit/server/project/PutBranch.java
View File

@ -25,7 +25,6 @@ public class PutBranch implements RestModifyView<BranchResource, BranchInput> {
@Override
public BranchInfo apply(BranchResource rsrc, BranchInput input) throws ResourceConflictException {
throw new ResourceConflictException(
"Branch \"" + rsrc.getBranchInfo().ref + "\" already exists");
throw new ResourceConflictException("Branch \"" + rsrc.getRef() + "\" already exists");
}
}