Cache groups by member outside of the account cache
Originally, we cached those groups as part of AccountState in the
account cache. Since we don't want the account cache to depend on the
group index but still want any usages of the methods of
IncludingGroupMembership to be reasonably fast, we need to cache
those groups in an own cache.
Change-Id: I1617c4b0fcfa358b08d2e714499d59b1aba82550
diff --git a/Documentation/cmd-show-caches.txt b/Documentation/cmd-show-caches.txt
index e47ae81..1c9c7e9 100644
--- a/Documentation/cmd-show-caches.txt
+++ b/Documentation/cmd-show-caches.txt
@@ -62,6 +62,7 @@
changes | | 27.1ms | 0% |
groups | 5646 | 11.8ms | 97% |
groups_byinclude | 230 | 2.4ms | 62% |
+ groups_bymember | | | |
groups_byname | | | |
groups_byuuid | 5612 | 29.2ms | 99% |
groups_external | 1 | 1.5s | 98% |
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 35fa347..7a59fa1 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -852,6 +852,12 @@
Caches group inclusions in other groups. If direct updates are made
to the `account_group_includes` table, this cache should be flushed.
+cache `"groups_bymember"`::
++
+Caches the groups which contain a specific member (account). If direct
+updates are made to the `account_group_members` table, this cache should
+be flushed.
+
cache `"groups_members"`::
+
Caches subgroups. If direct updates are made to the
diff --git a/Documentation/rest-api-config.txt b/Documentation/rest-api-config.txt
index b64af53..f1e9126 100644
--- a/Documentation/rest-api-config.txt
+++ b/Documentation/rest-api-config.txt
@@ -338,6 +338,11 @@
"entries": {},
"hit_ratio": {}
},
+ "groups_bymember": {
+ "type": "MEM",
+ "entries": {},
+ "hit_ratio": {}
+ },
"groups_byname": {
"type": "MEM",
"entries": {},
@@ -468,6 +473,7 @@
"git_tags",
"groups",
"groups_byinclude",
+ "groups_bymember",
"groups_byname",
"groups_byuuid",
"groups_external",
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
index 305a2b0..3f18f64 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
@@ -46,6 +46,7 @@
import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.server.account.GroupIncludeCache;
import com.google.gerrit.server.group.Groups;
import com.google.gerrit.server.group.GroupsUpdate;
import com.google.gerrit.server.group.InternalGroup;
@@ -56,6 +57,7 @@
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Arrays;
+import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
@@ -65,6 +67,7 @@
public class GroupsIT extends AbstractDaemonTest {
@Inject @ServerInitiated private Provider<GroupsUpdate> groupsUpdateProvider;
@Inject private Groups groups;
+ @Inject private GroupIncludeCache groupIncludeCache;
@Test
public void systemGroupCanBeRetrievedFromIndex() throws Exception {
@@ -95,6 +98,26 @@
}
@Test
+ public void cachedGroupsForMemberAreUpdatedOnMemberAdditionAndRemoval() throws Exception {
+ // Fill the cache for the observed account.
+ groupIncludeCache.getGroupsWithMember(user.getId());
+ String groupName = createGroup("users");
+ AccountGroup.UUID groupUuid = new AccountGroup.UUID(gApi.groups().id(groupName).get().id);
+
+ gApi.groups().id(groupName).addMembers(user.fullName);
+
+ Collection<AccountGroup.UUID> groupsWithMemberAfterAddition =
+ groupIncludeCache.getGroupsWithMember(user.getId());
+ assertThat(groupsWithMemberAfterAddition).contains(groupUuid);
+
+ gApi.groups().id(groupName).removeMembers(user.fullName);
+
+ Collection<AccountGroup.UUID> groupsWithMemberAfterRemoval =
+ groupIncludeCache.getGroupsWithMember(user.getId());
+ assertThat(groupsWithMemberAfterRemoval).doesNotContain(groupUuid);
+ }
+
+ @Test
public void addExistingMember_OK() throws Exception {
String g = "Administrators";
assertMembers(g, admin);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCache.java
index c702aef..daa431e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCache.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCache.java
@@ -14,11 +14,21 @@
package com.google.gerrit.server.account;
+import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import java.util.Collection;
/** Tracks group inclusions in memory for efficient access. */
public interface GroupIncludeCache {
+
+ /**
+ * Returns the UUIDs of all groups of which the specified account is a direct member.
+ *
+ * @param memberId the ID of the account
+ * @return the UUIDs of all groups having the account as member
+ */
+ Collection<AccountGroup.UUID> getGroupsWithMember(Account.Id memberId);
+
/** @return groups directly a member of the passed group. */
Collection<AccountGroup.UUID> subgroupsOf(AccountGroup.UUID group);
@@ -28,6 +38,8 @@
/** @return set of any UUIDs that are not internal groups. */
Collection<AccountGroup.UUID> allExternalMembers();
+ void evictGroupsWithMember(Account.Id memberId);
+
void evictSubgroupsOf(AccountGroup.UUID groupId);
void evictParentGroupsOf(AccountGroup.UUID groupId);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java
index 2691bc1..cfb14c54 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java
@@ -15,12 +15,15 @@
package com.google.gerrit.server.account;
import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Streams;
import com.google.gerrit.common.errors.NoSuchGroupException;
+import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.cache.CacheModule;
@@ -41,6 +44,7 @@
import java.util.Collection;
import java.util.Collections;
import java.util.concurrent.ExecutionException;
+import java.util.stream.Stream;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -50,6 +54,7 @@
private static final Logger log = LoggerFactory.getLogger(GroupIncludeCacheImpl.class);
private static final String PARENT_GROUPS_NAME = "groups_byinclude";
private static final String SUBGROUPS_NAME = "groups_members";
+ private static final String GROUPS_WITH_MEMBER_NAME = "groups_bymember";
private static final String EXTERNAL_NAME = "groups_external";
public static Module module() {
@@ -57,6 +62,12 @@
@Override
protected void configure() {
cache(
+ GROUPS_WITH_MEMBER_NAME,
+ Account.Id.class,
+ new TypeLiteral<ImmutableSet<AccountGroup.UUID>>() {})
+ .loader(GroupsWithMemberLoader.class);
+
+ cache(
PARENT_GROUPS_NAME,
AccountGroup.UUID.class,
new TypeLiteral<ImmutableList<AccountGroup.UUID>>() {})
@@ -77,23 +88,37 @@
};
}
+ private final LoadingCache<Account.Id, ImmutableSet<AccountGroup.UUID>> groupsWithMember;
private final LoadingCache<AccountGroup.UUID, ImmutableList<AccountGroup.UUID>> subgroups;
private final LoadingCache<AccountGroup.UUID, ImmutableList<AccountGroup.UUID>> parentGroups;
private final LoadingCache<String, ImmutableList<AccountGroup.UUID>> external;
@Inject
GroupIncludeCacheImpl(
+ @Named(GROUPS_WITH_MEMBER_NAME)
+ LoadingCache<Account.Id, ImmutableSet<AccountGroup.UUID>> groupsWithMember,
@Named(SUBGROUPS_NAME)
LoadingCache<AccountGroup.UUID, ImmutableList<AccountGroup.UUID>> subgroups,
@Named(PARENT_GROUPS_NAME)
LoadingCache<AccountGroup.UUID, ImmutableList<AccountGroup.UUID>> parentGroups,
@Named(EXTERNAL_NAME) LoadingCache<String, ImmutableList<AccountGroup.UUID>> external) {
+ this.groupsWithMember = groupsWithMember;
this.subgroups = subgroups;
this.parentGroups = parentGroups;
this.external = external;
}
@Override
+ public Collection<AccountGroup.UUID> getGroupsWithMember(Account.Id memberId) {
+ try {
+ return groupsWithMember.get(memberId);
+ } catch (ExecutionException e) {
+ log.warn(String.format("Cannot load groups containing %d as member", memberId.get()));
+ return ImmutableSet.of();
+ }
+ }
+
+ @Override
public Collection<AccountGroup.UUID> subgroupsOf(AccountGroup.UUID groupId) {
try {
return subgroups.get(groupId);
@@ -114,6 +139,13 @@
}
@Override
+ public void evictGroupsWithMember(Account.Id memberId) {
+ if (memberId != null) {
+ groupsWithMember.invalidate(memberId);
+ }
+ }
+
+ @Override
public void evictSubgroupsOf(AccountGroup.UUID groupId) {
if (groupId != null) {
subgroups.invalidate(groupId);
@@ -141,6 +173,46 @@
}
}
+ static class GroupsWithMemberLoader
+ extends CacheLoader<Account.Id, ImmutableSet<AccountGroup.UUID>> {
+ private final SchemaFactory<ReviewDb> schema;
+ private final Provider<GroupIndex> groupIndexProvider;
+ private final Provider<InternalGroupQuery> groupQueryProvider;
+ private final GroupCache groupCache;
+
+ @Inject
+ GroupsWithMemberLoader(
+ SchemaFactory<ReviewDb> schema,
+ GroupIndexCollection groupIndexCollection,
+ Provider<InternalGroupQuery> groupQueryProvider,
+ GroupCache groupCache) {
+ this.schema = schema;
+ groupIndexProvider = groupIndexCollection::getSearchIndex;
+ this.groupQueryProvider = groupQueryProvider;
+ this.groupCache = groupCache;
+ }
+
+ @Override
+ public ImmutableSet<AccountGroup.UUID> load(Account.Id memberId)
+ throws OrmException, NoSuchGroupException {
+
+ Stream<InternalGroup> internalGroupStream;
+ GroupIndex groupIndex = groupIndexProvider.get();
+ if (groupIndex != null && groupIndex.getSchema().hasField(GroupField.MEMBER)) {
+ internalGroupStream = groupQueryProvider.get().byMember(memberId).stream();
+ } else {
+ try (ReviewDb db = schema.open()) {
+ internalGroupStream =
+ Groups.getGroupsWithMemberFromReviewDb(db, memberId)
+ .map(groupCache::get)
+ .flatMap(Streams::stream);
+ }
+ }
+
+ return internalGroupStream.map(InternalGroup::getGroupUUID).collect(toImmutableSet());
+ }
+ }
+
static class SubgroupsLoader
extends CacheLoader<AccountGroup.UUID, ImmutableList<AccountGroup.UUID>> {
private final SchemaFactory<ReviewDb> schema;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/IncludingGroupMembership.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/IncludingGroupMembership.java
index 9c05fa6..a077629 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/IncludingGroupMembership.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/IncludingGroupMembership.java
@@ -14,35 +14,21 @@
package com.google.gerrit.server.account;
-import static com.google.common.collect.ImmutableSet.toImmutableSet;
-
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
-import com.google.common.collect.Streams;
-import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
-import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.group.Groups;
import com.google.gerrit.server.group.InternalGroup;
-import com.google.gerrit.server.index.group.GroupField;
-import com.google.gerrit.server.index.group.GroupIndex;
-import com.google.gerrit.server.index.group.GroupIndexCollection;
-import com.google.gerrit.server.query.group.InternalGroupQuery;
-import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
-import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
+import java.util.Collection;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
-import java.util.stream.Stream;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
* Group membership checker for the internal group system.
@@ -57,30 +43,17 @@
IncludingGroupMembership create(IdentifiedUser user);
}
- private static final Logger log = LoggerFactory.getLogger(IncludingGroupMembership.class);
-
- private final Provider<ReviewDb> db;
private final GroupCache groupCache;
private final GroupIncludeCache includeCache;
- private final Provider<GroupIndex> groupIndexProvider;
- private final Provider<InternalGroupQuery> groupQueryProvider;
private final IdentifiedUser user;
private final Map<AccountGroup.UUID, Boolean> memberOf;
private Set<AccountGroup.UUID> knownGroups;
@Inject
IncludingGroupMembership(
- Provider<ReviewDb> db,
- GroupCache groupCache,
- GroupIncludeCache includeCache,
- GroupIndexCollection groupIndexCollection,
- Provider<InternalGroupQuery> groupQueryProvider,
- @Assisted IdentifiedUser user) {
- this.db = db;
+ GroupCache groupCache, GroupIncludeCache includeCache, @Assisted IdentifiedUser user) {
this.groupCache = groupCache;
this.includeCache = includeCache;
- groupIndexProvider = groupIndexCollection::getSearchIndex;
- this.groupQueryProvider = groupQueryProvider;
this.user = user;
memberOf = new ConcurrentHashMap<>();
}
@@ -151,15 +124,8 @@
private ImmutableSet<AccountGroup.UUID> computeKnownGroups() {
GroupMembership membership = user.getEffectiveGroups();
- Set<AccountGroup.UUID> direct;
- try {
- direct = getGroupsWithMember(db.get(), user.getAccountId());
- direct.forEach(groupUuid -> memberOf.put(groupUuid, true));
- } catch (OrmException e) {
- log.warn(
- String.format("Cannot load groups containing %d as member", user.getAccountId().get()));
- direct = ImmutableSet.of();
- }
+ Collection<AccountGroup.UUID> direct = includeCache.getGroupsWithMember(user.getAccountId());
+ direct.forEach(groupUuid -> memberOf.put(groupUuid, true));
Set<AccountGroup.UUID> r = Sets.newHashSet(direct);
r.remove(null);
@@ -182,22 +148,6 @@
return ImmutableSet.copyOf(r);
}
- private ImmutableSet<AccountGroup.UUID> getGroupsWithMember(ReviewDb db, Account.Id memberId)
- throws OrmException {
- Stream<InternalGroup> internalGroupStream;
- GroupIndex groupIndex = groupIndexProvider.get();
- if (groupIndex != null && groupIndex.getSchema().hasField(GroupField.MEMBER)) {
- internalGroupStream = groupQueryProvider.get().byMember(memberId).stream();
- } else {
- internalGroupStream =
- Groups.getGroupsWithMemberFromReviewDb(db, memberId)
- .map(groupCache::get)
- .flatMap(Streams::stream);
- }
-
- return internalGroupStream.map(InternalGroup::getGroupUUID).collect(toImmutableSet());
- }
-
@Override
public Set<AccountGroup.UUID> getKnownGroups() {
if (knownGroups == null) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsUpdate.java
index 9df5110..f31e383 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsUpdate.java
@@ -280,6 +280,9 @@
}
db.accountGroupMembers().insert(newMembers);
groupCache.evict(group.getGroupUUID(), group.getId(), group.getNameKey());
+ for (AccountGroupMember newMember : newMembers) {
+ groupIncludeCache.evictGroupsWithMember(newMember.getAccountId());
+ }
}
/**
@@ -316,6 +319,9 @@
}
db.accountGroupMembers().delete(membersToRemove);
groupCache.evict(group.getGroupUUID(), group.getId(), group.getNameKey());
+ for (AccountGroupMember member : membersToRemove) {
+ groupIncludeCache.evictGroupsWithMember(member.getAccountId());
+ }
}
/**
