opendev/gerrit
Code Issues Proposed changes

Partially revert GroupDetail to fix Admin > Groups issues

Browse Source 
Loading Admin > Groups on a large server is horribly slow because
someone thought it was a good idea to reuse GroupDetail in
fccf928042. That caused the entire
group membership database to be downloaded to the browser when
showing just the list of groups, only to have the data discarded and
reloaded after selecting a specific group to view. If a server has
7k users and 17k groups, it could take ages to show the groups list.

Strip out GroupDetail and switch back to the more lightweight
AccountGroup type when showing the groups in a list format.

Since there are only 3 SYSTEM groups using well known names, and
everything else returned is of type INTERNAL because the LDAP type
no longer exists, drop the type column from the table, it isn't
really interesting anymore.

This change does drop the Owner group name column header. Displaying
it can really slow down on large servers when there are a lot of
groups. So skip displaying the owner name. Its more important to me
that the Admin > Groups table doesn't download the entire database
than having the owner group resolved in the list view. If someone
really cares enough, they can reattempt what fccf928042 was trying
to accomplish, without downloading the entire membership database.

Change-Id: I884a5129d238b7aaaf6db8a886e7c0a49f73cf74
This commit is contained in:
Shawn O. Pearce 2012-08-01 12:30:49 -07:00
parent 0dbb8188a2
commit 7e031f38f7
8 changed files with 34 additions and 55 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
gerrit-common/src/main/java/com/google/gerrit/common/data/AccountSecurity.java
View File

@ -18,13 +18,14 @@ import com.google.gerrit.common.audit.Audit;
import com.google.gerrit.common.auth.SignInRequired;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountExternalId;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.AccountSshKey;
import com.google.gerrit.reviewdb.client.ContactInformation;
import com.google.gwtjsonrpc.common.AsyncCallback;
import com.google.gwtjsonrpc.common.RemoteJsonService;
import com.google.gwtjsonrpc.common.RpcImpl;
import com.google.gwtjsonrpc.common.VoidResult;
import com.google.gwtjsonrpc.common.RpcImpl.Version;
import com.google.gwtjsonrpc.common.VoidResult;
import java.util.List;
import java.util.Set;
@ -61,7 +62,7 @@ public interface AccountSecurity extends RemoteJsonService {
void myExternalIds(AsyncCallback<List<AccountExternalId>> callback);
@SignInRequired
void myGroups(AsyncCallback<List<GroupDetail>> callback);
void myGroups(AsyncCallback<List<AccountGroup>> callback);
@Audit
@SignInRequired

10
gerrit-common/src/main/java/com/google/gerrit/common/data/GroupList.java
View File

@ -14,25 +14,27 @@
package com.google.gerrit.common.data;
import com.google.gerrit.reviewdb.client.AccountGroup;
import java.util.List;
public class GroupList {
protected List<GroupDetail> groups;
protected List<AccountGroup> groups;
protected boolean canCreateGroup;
protected GroupList() {
}
public GroupList(final List<GroupDetail> groups, final boolean canCreateGroup) {
public GroupList(final List<AccountGroup> groups, final boolean canCreateGroup) {
this.groups = groups;
this.canCreateGroup = canCreateGroup;
}
public List<GroupDetail> getGroups() {
public List<AccountGroup> getGroups() {
return groups;
}
public void setGroups(List<GroupDetail> groups) {
public void setGroups(List<AccountGroup> groups) {
this.groups = groups;
}

7
gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyGroupsScreen.java
View File

@ -16,7 +16,7 @@ package com.google.gerrit.client.account;
import com.google.gerrit.client.admin.GroupTable;
import com.google.gerrit.client.rpc.ScreenLoadCallback;
import com.google.gerrit.common.data.GroupDetail;
import com.google.gerrit.reviewdb.client.AccountGroup;
import java.util.List;
@ -33,8 +33,9 @@ public class MyGroupsScreen extends SettingsScreen {
@Override
protected void onLoad() {
super.onLoad();
Util.ACCOUNT_SEC.myGroups(new ScreenLoadCallback<List<GroupDetail>>(this) {
public void preDisplay(final List<GroupDetail> result) {
Util.ACCOUNT_SEC.myGroups(new ScreenLoadCallback<List<AccountGroup>>(this) {
@Override
public void preDisplay(final List<AccountGroup> result) {
groups.display(result);
}
});

20
gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/GroupTable.java
View File

@ -18,7 +18,6 @@ import com.google.gerrit.client.Dispatcher;
import com.google.gerrit.client.Gerrit;
import com.google.gerrit.client.ui.Hyperlink;
import com.google.gerrit.client.ui.NavigationTable;
import com.google.gerrit.common.data.GroupDetail;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gwt.event.dom.client.ClickEvent;
import com.google.gwt.event.dom.client.ClickHandler;
@ -32,7 +31,7 @@ import java.util.List;
public class GroupTable extends NavigationTable<AccountGroup> {
private static final int NUM_COLS = 5;
private static final int NUM_COLS = 3;
private final boolean enableLink;
@ -52,9 +51,7 @@ public class GroupTable extends NavigationTable<AccountGroup> {
table.setText(0, 1, Util.C.columnGroupName());
table.setText(0, 2, Util.C.columnGroupDescription());
table.setText(0, 3, Util.C.headingOwner());
table.setText(0, 4, Util.C.columnGroupType());
table.setText(0, 5, Util.C.columnGroupVisibleToAll());
table.setText(0, 3, Util.C.columnGroupVisibleToAll());
table.addClickHandler(new ClickHandler() {
@Override
public void onClick(ClickEvent event) {
@ -82,20 +79,19 @@ public class GroupTable extends NavigationTable<AccountGroup> {
History.newItem(Dispatcher.toGroup(getRowItem(row).getId()));
}
public void display(final List<GroupDetail> result) {
public void display(final List<AccountGroup> result) {
while (1 < table.getRowCount())
table.removeRow(table.getRowCount() - 1);
for(GroupDetail detail : result) {
for(AccountGroup group : result) {
final int row = table.getRowCount();
table.insertRow(row);
applyDataRowStyle(row);
populate(row, detail);
populate(row, group);
}
}
void populate(final int row, final GroupDetail detail) {
AccountGroup k = detail.group;
void populate(final int row, final AccountGroup k) {
if (enableLink) {
table.setWidget(row, 1, new Hyperlink(k.getName(),
Dispatcher.toGroup(k.getId())));
@ -103,10 +99,8 @@ public class GroupTable extends NavigationTable<AccountGroup> {
table.setText(row, 1, k.getName());
}
table.setText(row, 2, k.getDescription());
table.setText(row, 3, detail.ownerGroup.getName());
table.setText(row, 4, k.getType().toString());
if (k.isVisibleToAll()) {
table.setWidget(row, 5, new Image(Gerrit.RESOURCES.greenCheck()));
table.setWidget(row, 3, new Image(Gerrit.RESOURCES.greenCheck()));
}
final FlexCellFormatter fmt = table.getFlexCellFormatter();

7
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/AccountSecurityImpl.java
View File

@ -17,7 +17,6 @@ package com.google.gerrit.httpd.rpc.account;
import com.google.gerrit.common.ChangeHooks;
import com.google.gerrit.common.data.AccountSecurity;
import com.google.gerrit.common.data.ContributorAgreement;
import com.google.gerrit.common.data.GroupDetail;
import com.google.gerrit.common.errors.ContactInformationStoreException;
import com.google.gerrit.common.errors.InvalidSshKeyException;
import com.google.gerrit.common.errors.NameAlreadyUsedException;
@ -213,9 +212,9 @@ class AccountSecurityImpl extends BaseServiceImplementation implements
}
@Override
public void myGroups(final AsyncCallback<List<GroupDetail>> callback) {
run(callback, new Action<List<GroupDetail>>() {
public List<GroupDetail> run(final ReviewDb db) throws OrmException,
public void myGroups(final AsyncCallback<List<AccountGroup>> callback) {
run(callback, new Action<List<AccountGroup>>() {
public List<AccountGroup> run(final ReviewDb db) throws OrmException,
NoSuchGroupException, Failure {
return myGroupsFactory.create().call();
}

6
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/MyGroupsFactory.java
View File

@ -14,9 +14,9 @@
package com.google.gerrit.httpd.rpc.account;
import com.google.gerrit.common.data.GroupDetail;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.httpd.rpc.Handler;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.VisibleGroups;
import com.google.gwtorm.server.OrmException;
@ -24,7 +24,7 @@ import com.google.inject.Inject;
import java.util.List;
class MyGroupsFactory extends Handler<List<GroupDetail>> {
class MyGroupsFactory extends Handler<List<AccountGroup>> {
interface Factory {
MyGroupsFactory create();
}
@ -39,7 +39,7 @@ class MyGroupsFactory extends Handler<List<GroupDetail>> {
}
@Override
public List<GroupDetail> call() throws OrmException, NoSuchGroupException {
public List<AccountGroup> call() throws OrmException, NoSuchGroupException {
final VisibleGroups visibleGroups = visibleGroupsFactory.create();
return visibleGroups.get(user).getGroups();
}

27
gerrit-server/src/main/java/com/google/gerrit/server/account/VisibleGroups.java
View File

@ -14,18 +14,15 @@
package com.google.gerrit.server.account;
import com.google.gerrit.common.data.GroupDetail;
import com.google.gerrit.common.data.GroupList;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.util.ArrayList;
import java.util.Collection;
import java.util.LinkedList;
import java.util.List;
@ -41,7 +38,6 @@ public class VisibleGroups {
private final Provider<IdentifiedUser> identifiedUser;
private final GroupCache groupCache;
private final GroupControl.Factory groupControlFactory;
private final GroupDetailFactory.Factory groupDetailFactory;
private boolean onlyVisibleToAll;
private AccountGroup.Type groupType;
@ -49,12 +45,10 @@ public class VisibleGroups {
@Inject
VisibleGroups(final Provider<IdentifiedUser> currentUser,
final GroupCache groupCache,
final GroupControl.Factory groupControlFactory,
final GroupDetailFactory.Factory groupDetailFactory) {
final GroupControl.Factory groupControlFactory) {
this.identifiedUser = currentUser;
this.groupCache = groupCache;
this.groupControlFactory = groupControlFactory;
this.groupDetailFactory = groupDetailFactory;
}
public void setOnlyVisibleToAll(final boolean onlyVisibleToAll) {
@ -65,13 +59,12 @@ public class VisibleGroups {
this.groupType = groupType;
}
public GroupList get() throws OrmException, NoSuchGroupException {
final Iterable<AccountGroup> groups = groupCache.all();
return createGroupList(filterGroups(groups));
public GroupList get() {
return createGroupList(filterGroups(groupCache.all()));
}
public GroupList get(final Collection<ProjectControl> projects)
throws OrmException, NoSuchGroupException {
throws NoSuchGroupException {
final Set<AccountGroup> groups =
new TreeSet<AccountGroup>(new GroupComparator());
for (final ProjectControl projectControl : projects) {
@ -93,8 +86,7 @@ public class VisibleGroups {
* groups.
* @See GroupMembership#getKnownGroups()
*/
public GroupList get(final IdentifiedUser user) throws OrmException,
NoSuchGroupException {
public GroupList get(final IdentifiedUser user) throws NoSuchGroupException {
if (identifiedUser.get().getAccountId().equals(user.getAccountId())
|| identifiedUser.get().getCapabilities().canAdministrateServer()) {
final Set<AccountGroup.UUID> effective =
@ -134,13 +126,8 @@ public class VisibleGroups {
return filteredGroups;
}
private GroupList createGroupList(final List<AccountGroup> groups)
throws OrmException, NoSuchGroupException {
final List<GroupDetail> groupDetailList = new ArrayList<GroupDetail>();
for (final AccountGroup group : groups) {
groupDetailList.add(groupDetailFactory.create(group.getId()).call());
}
return new GroupList(groupDetailList, identifiedUser.get()
private GroupList createGroupList(final List<AccountGroup> groups) {
return new GroupList(groups, identifiedUser.get()
.getCapabilities().canCreateGroup());
}
}

7
gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ListGroupsCommand.java
View File

@ -14,7 +14,6 @@
package com.google.gerrit.sshd.commands;
import com.google.gerrit.common.data.GroupDetail;
import com.google.gerrit.common.data.GroupList;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.reviewdb.client.Account;
@ -26,7 +25,6 @@ import com.google.gerrit.server.ioutil.ColumnFormatter;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.sshd.SshCommand;
import com.google.gwtorm.client.KeyUtil;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import org.kohsuke.args4j.Option;
@ -84,8 +82,7 @@ public class ListGroupsCommand extends SshCommand {
}
final ColumnFormatter formatter = new ColumnFormatter(stdout, '\t');
for (final GroupDetail groupDetail : groupList.getGroups()) {
final AccountGroup g = groupDetail.group;
for (final AccountGroup g : groupList.getGroups()) {
formatter.addColumn(g.getName());
if (verboseOutput) {
formatter.addColumn(KeyUtil.decode(g.getGroupUUID().toString()));
@ -102,8 +99,6 @@ public class ListGroupsCommand extends SshCommand {
formatter.nextLine();
}
formatter.finish();
} catch (OrmException e) {
throw die(e);
} catch (NoSuchGroupException e) {
throw die(e);
}