Partially revert GroupDetail to fix Admin > Groups issues
Loading Admin > Groups on a large server is horribly slow because
someone thought it was a good idea to reuse GroupDetail in
fccf928042d372f379204fcef61038f94c5b0845. 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
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/AccountSecurity.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/AccountSecurity.java
index bd92c79..aa212f9 100644
--- a/gerrit-common/src/main/java/com/google/gerrit/common/data/AccountSecurity.java
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/AccountSecurity.java
@@ -18,13 +18,14 @@
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 @@
void myExternalIds(AsyncCallback<List<AccountExternalId>> callback);
@SignInRequired
- void myGroups(AsyncCallback<List<GroupDetail>> callback);
+ void myGroups(AsyncCallback<List<AccountGroup>> callback);
@Audit
@SignInRequired
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupList.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupList.java
index 6352461..b3095cd 100644
--- a/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupList.java
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupList.java
@@ -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;
}
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyGroupsScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyGroupsScreen.java
index 719c395..6cb749d 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyGroupsScreen.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyGroupsScreen.java
@@ -16,7 +16,7 @@
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 @@
@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);
}
});
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/GroupTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/GroupTable.java
index 7f73226..6818841 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/GroupTable.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/GroupTable.java
@@ -18,7 +18,6 @@
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 @@
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 @@
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 @@
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 @@
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();
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/AccountSecurityImpl.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/AccountSecurityImpl.java
index 8cdccac..b62a10b 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/AccountSecurityImpl.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/AccountSecurityImpl.java
@@ -17,7 +17,6 @@
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 @@
}
@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();
}
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/MyGroupsFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/MyGroupsFactory.java
index fd274fd..33ce371 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/MyGroupsFactory.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/MyGroupsFactory.java
@@ -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 java.util.List;
-class MyGroupsFactory extends Handler<List<GroupDetail>> {
+class MyGroupsFactory extends Handler<List<AccountGroup>> {
interface Factory {
MyGroupsFactory create();
}
@@ -39,7 +39,7 @@
}
@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();
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/VisibleGroups.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/VisibleGroups.java
index 3112ed4..4037266 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/VisibleGroups.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/VisibleGroups.java
@@ -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 @@
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 @@
@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 @@
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 @@
* 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 @@
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());
}
}
diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ListGroupsCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ListGroupsCommand.java
index aa439c6..f8856f2 100644
--- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ListGroupsCommand.java
+++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ListGroupsCommand.java
@@ -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.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 @@
}
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 @@
formatter.nextLine();
}
formatter.finish();
- } catch (OrmException e) {
- throw die(e);
} catch (NoSuchGroupException e) {
throw die(e);
}
