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

Documentation/cmd-show-caches.txt

@@ -62,6 +62,7 @@ Intended for interactive use only.
     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%     |

Documentation/config-gerrit.txt

@@ -852,6 +852,12 @@ cache `"groups_byinclude"`::
 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

Documentation/rest-api-config.txt

@@ -338,6 +338,11 @@ The entries in the map are sorted by cache name.
       "entries": {},
       "hit_ratio": {}
     },
+    "groups_bymember": {
+      "type": "MEM",
+      "entries": {},
+      "hit_ratio": {}
+    },
     "groups_byname": {
       "type": "MEM",
       "entries": {},
@@ -468,6 +473,7 @@ The cache names are lexicographically sorted.
     "git_tags",
     "groups",
     "groups_byinclude",
+    "groups_bymember",
     "groups_byname",
     "groups_byuuid",
     "groups_external",

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java

@@ -46,6 +46,7 @@ import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 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 com.google.inject.Provider;
 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 @@ import org.junit.Test;
 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 {
@@ -94,6 +97,26 @@ public class GroupsIT extends AbstractDaemonTest {
     assertNoMembers(g);
   }
 
+  @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";

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 @@ public interface GroupIncludeCache {
   /** @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);

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 com.google.inject.name.Named;
 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,12 +54,19 @@ public class GroupIncludeCacheImpl implements GroupIncludeCache {
   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() {
     return new CacheModule() {
       @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,
@@ -77,22 +88,36 @@ public class GroupIncludeCacheImpl implements GroupIncludeCache {
     };
   }
 
+  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 {
@@ -113,6 +138,13 @@ public class GroupIncludeCacheImpl implements GroupIncludeCache {
     }
   }
 
+  @Override
+  public void evictGroupsWithMember(Account.Id memberId) {
+    if (memberId != null) {
+      groupsWithMember.invalidate(memberId);
+    }
+  }
+
   @Override
   public void evictSubgroupsOf(AccountGroup.UUID groupId) {
     if (groupId != null) {
@@ -141,6 +173,46 @@ public class GroupIncludeCacheImpl implements GroupIncludeCache {
     }
   }
 
+  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;

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 @@ public class IncludingGroupMembership implements GroupMembership {
     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 @@ public class IncludingGroupMembership implements GroupMembership {
 
   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 @@ public class IncludingGroupMembership implements GroupMembership {
     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) {

gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsUpdate.java

@@ -280,6 +280,9 @@ public class GroupsUpdate {
     }
     db.accountGroupMembers().insert(newMembers);
     groupCache.evict(group.getGroupUUID(), group.getId(), group.getNameKey());
+    for (AccountGroupMember newMember : newMembers) {
+      groupIncludeCache.evictGroupsWithMember(newMember.getAccountId());
+    }
   }
 
   /**
@@ -316,6 +319,9 @@ public class GroupsUpdate {
     }
     db.accountGroupMembers().delete(membersToRemove);
     groupCache.evict(group.getGroupUUID(), group.getId(), group.getNameKey());
+    for (AccountGroupMember member : membersToRemove) {
+      groupIncludeCache.evictGroupsWithMember(member.getAccountId());
+    }
   }
 
   /**