Merge "groups_bysubgroups: Batch loading values"
diff --git a/java/com/google/gerrit/server/account/GroupIncludeCache.java b/java/com/google/gerrit/server/account/GroupIncludeCache.java
index d92d9fc..266f858 100644
--- a/java/com/google/gerrit/server/account/GroupIncludeCache.java
+++ b/java/com/google/gerrit/server/account/GroupIncludeCache.java
@@ -16,7 +16,9 @@
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AccountGroup;
+import com.google.gerrit.entities.AccountGroup.UUID;
import java.util.Collection;
+import java.util.Set;
/** Tracks group inclusions in memory for efficient access. */
public interface GroupIncludeCache {
@@ -37,6 +39,14 @@
*/
Collection<AccountGroup.UUID> parentGroupsOf(AccountGroup.UUID groupId);
+ /**
+ * Returns the parent groups of the provided subgroups.
+ *
+ * @param groupId the UUID of the subgroup
+ * @return the UUIDs of all direct parent groups
+ */
+ Collection<AccountGroup.UUID> parentGroupsOf(Set<UUID> groupId);
+
/** Returns set of any UUIDs that are not internal groups. */
Collection<AccountGroup.UUID> allExternalMembers();
diff --git a/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java b/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java
index f203240..fc6087b 100644
--- a/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java
+++ b/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java
@@ -22,6 +22,8 @@
import com.google.common.cache.LoadingCache;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Maps;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AccountGroup;
@@ -45,6 +47,9 @@
import com.google.inject.name.Named;
import java.util.Collection;
import java.util.Collections;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
import java.util.concurrent.ExecutionException;
/** Tracks group inclusions in memory for efficient access. */
@@ -70,7 +75,7 @@
cache(
PARENT_GROUPS_NAME,
AccountGroup.UUID.class,
- new TypeLiteral<ImmutableList<AccountGroup.UUID>>() {})
+ new TypeLiteral<ImmutableSet<AccountGroup.UUID>>() {})
.loader(ParentGroupsLoader.class);
/**
@@ -101,7 +106,7 @@
}
private final LoadingCache<Account.Id, ImmutableSet<AccountGroup.UUID>> groupsWithMember;
- private final LoadingCache<AccountGroup.UUID, ImmutableList<AccountGroup.UUID>> parentGroups;
+ private final LoadingCache<AccountGroup.UUID, ImmutableSet<AccountGroup.UUID>> parentGroups;
private final LoadingCache<String, ImmutableList<AccountGroup.UUID>> external;
@Inject
@@ -109,7 +114,7 @@
@Named(GROUPS_WITH_MEMBER_NAME)
LoadingCache<Account.Id, ImmutableSet<AccountGroup.UUID>> groupsWithMember,
@Named(PARENT_GROUPS_NAME)
- LoadingCache<AccountGroup.UUID, ImmutableList<AccountGroup.UUID>> parentGroups,
+ LoadingCache<AccountGroup.UUID, ImmutableSet<AccountGroup.UUID>> parentGroups,
@Named(EXTERNAL_NAME) LoadingCache<String, ImmutableList<AccountGroup.UUID>> external) {
this.groupsWithMember = groupsWithMember;
this.parentGroups = parentGroups;
@@ -137,6 +142,18 @@
}
@Override
+ public Collection<AccountGroup.UUID> parentGroupsOf(Set<AccountGroup.UUID> groupIds) {
+ try {
+ Set<AccountGroup.UUID> parents = new HashSet<>();
+ parentGroups.getAll(groupIds).values().forEach(p -> parents.addAll(p));
+ return parents;
+ } catch (ExecutionException e) {
+ logger.atWarning().withCause(e).log("Cannot load included groups");
+ return Collections.emptySet();
+ }
+ }
+
+ @Override
public void evictGroupsWithMember(Account.Id memberId) {
if (memberId != null) {
logger.atFine().log("Evict groups with member %d", memberId.get());
@@ -194,7 +211,10 @@
}
static class ParentGroupsLoader
- extends CacheLoader<AccountGroup.UUID, ImmutableList<AccountGroup.UUID>> {
+ extends CacheLoader<AccountGroup.UUID, ImmutableSet<AccountGroup.UUID>> {
+ // Be conservative with batching: We don't want to exhaust the number of
+ // results per page and maximum terms per query. Both are usually 1000+.
+ private static final int MAX_BATCH_SIZE = 100;
private final Provider<InternalGroupQuery> groupQueryProvider;
@Inject
@@ -203,13 +223,26 @@
}
@Override
- public ImmutableList<AccountGroup.UUID> load(AccountGroup.UUID key) {
+ public ImmutableSet<AccountGroup.UUID> load(AccountGroup.UUID key) {
try (TraceTimer timer =
TraceContext.newTimer(
"Loading parent groups", Metadata.builder().groupUuid(key.get()).build())) {
- return groupQueryProvider.get().bySubgroup(key).stream()
- .map(InternalGroup::getGroupUUID)
- .collect(toImmutableList());
+ return loadAll(ImmutableList.of(key)).get(key);
+ }
+ }
+
+ @Override
+ public Map<AccountGroup.UUID, ImmutableSet<AccountGroup.UUID>> loadAll(
+ Iterable<? extends AccountGroup.UUID> keys) {
+ int numKeys = Iterables.size(keys);
+ Map<AccountGroup.UUID, ImmutableSet<AccountGroup.UUID>> result =
+ Maps.newHashMapWithExpectedSize(numKeys);
+ try (TraceTimer timer = TraceContext.newTimer("Loading " + numKeys + " parent groups")) {
+ Iterables.partition(keys, MAX_BATCH_SIZE)
+ .forEach(
+ keyPartition ->
+ result.putAll(groupQueryProvider.get().bySubgroups(ImmutableSet.copyOf(keys))));
+ return result;
}
}
}
diff --git a/java/com/google/gerrit/server/account/IncludingGroupMembership.java b/java/com/google/gerrit/server/account/IncludingGroupMembership.java
index 8cec8bf..e1edf10 100644
--- a/java/com/google/gerrit/server/account/IncludingGroupMembership.java
+++ b/java/com/google/gerrit/server/account/IncludingGroupMembership.java
@@ -16,7 +16,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.InternalGroup;
@@ -25,7 +24,6 @@
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.Set;
import java.util.concurrent.ConcurrentHashMap;
@@ -135,7 +133,7 @@
Set<AccountGroup.UUID> r = Sets.newHashSet(direct);
r.remove(null);
- List<AccountGroup.UUID> q = Lists.newArrayList(r);
+ Set<AccountGroup.UUID> q = Sets.newHashSet(r);
for (AccountGroup.UUID g : membership.intersection(includeCache.allExternalMembers())) {
if (g != null && r.add(g)) {
q.add(g);
@@ -143,9 +141,10 @@
}
while (!q.isEmpty()) {
- AccountGroup.UUID id = q.remove(q.size() - 1);
- for (AccountGroup.UUID g : includeCache.parentGroupsOf(id)) {
- if (g != null && r.add(g)) {
+ Collection<AccountGroup.UUID> parents = includeCache.parentGroupsOf(q);
+ q.clear();
+ for (AccountGroup.UUID g : parents) {
+ if (r.add(g)) {
q.add(g);
memberOf.put(g, true);
}
diff --git a/java/com/google/gerrit/server/query/group/InternalGroupQuery.java b/java/com/google/gerrit/server/query/group/InternalGroupQuery.java
index b9b58b8..078acd4 100644
--- a/java/com/google/gerrit/server/query/group/InternalGroupQuery.java
+++ b/java/com/google/gerrit/server/query/group/InternalGroupQuery.java
@@ -17,7 +17,9 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
+import com.google.common.collect.Maps;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AccountGroup;
@@ -27,8 +29,12 @@
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.server.index.group.GroupIndexCollection;
import com.google.inject.Inject;
+import java.util.HashSet;
import java.util.List;
+import java.util.Map;
import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
/**
* Query wrapper for the group index.
@@ -57,8 +63,29 @@
return query(GroupPredicates.member(memberId));
}
- public List<InternalGroup> bySubgroup(AccountGroup.UUID subgroupId) {
- return query(GroupPredicates.subgroup(subgroupId));
+ /**
+ * Get all immediate parents of the provided {@code subgroupIds}.
+ *
+ * @return map pointing from children to list of its immediate parents
+ */
+ public Map<AccountGroup.UUID, ImmutableSet<AccountGroup.UUID>> bySubgroups(
+ ImmutableSet<AccountGroup.UUID> subgroupIds) {
+ List<Predicate<InternalGroup>> predicates =
+ subgroupIds.stream().map(e -> GroupPredicates.subgroup(e)).collect(Collectors.toList());
+ List<InternalGroup> groups = query(Predicate.or(predicates));
+
+ Map<AccountGroup.UUID, Set<AccountGroup.UUID>> parentsByChild =
+ Maps.newHashMapWithExpectedSize(groups.size());
+ subgroupIds.stream().forEach(c -> parentsByChild.put(c, new HashSet<>()));
+ for (InternalGroup parent : groups) {
+ for (AccountGroup.UUID child : parent.getSubgroups()) {
+ if (subgroupIds.contains(child)) {
+ parentsByChild.get(child).add(parent.getGroupUUID());
+ }
+ }
+ }
+ return parentsByChild.entrySet().stream()
+ .collect(Collectors.toMap(Map.Entry::getKey, e -> ImmutableSet.copyOf(e.getValue())));
}
private Optional<InternalGroup> getOnlyGroup(
diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupIndexerIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupIndexerIT.java
index ece46c5..98ed56c 100644
--- a/javatests/com/google/gerrit/acceptance/api/group/GroupIndexerIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/group/GroupIndexerIT.java
@@ -14,12 +14,14 @@
package com.google.gerrit.acceptance.api.group;
+import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.gerrit.server.group.testing.InternalGroupSubject.internalGroups;
-import static com.google.gerrit.truth.ListSubject.assertThat;
import static com.google.gerrit.truth.OptionalSubject.assertThat;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.InternalGroup;
import com.google.gerrit.exceptions.NoSuchGroupException;
@@ -34,13 +36,12 @@
import com.google.gerrit.server.index.group.GroupIndexer;
import com.google.gerrit.server.query.group.InternalGroupQuery;
import com.google.gerrit.testing.InMemoryTestEnvironment;
-import com.google.gerrit.truth.ListSubject;
import com.google.gerrit.truth.OptionalSubject;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.io.IOException;
-import java.util.List;
import java.util.Optional;
+import java.util.Set;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.junit.Rule;
import org.junit.Test;
@@ -53,6 +54,7 @@
@Inject private GroupCache groupCache;
@Inject @ServerInitiated private GroupsUpdate groupsUpdate;
@Inject private Provider<InternalGroupQuery> groupQueryProvider;
+ @Inject private GroupOperations groupOperations;
@Test
public void indexingUpdatesTheIndex() throws Exception {
@@ -66,8 +68,10 @@
groupIndexer.index(groupUuid);
- List<InternalGroup> parentGroups = groupQueryProvider.get().bySubgroup(subgroupUuid);
- assertThatGroups(parentGroups).onlyElement().groupUuid().isEqualTo(groupUuid);
+ Set<AccountGroup.UUID> parentGroups =
+ groupQueryProvider.get().bySubgroups(ImmutableSet.of(subgroupUuid)).get(subgroupUuid);
+ assertThat(parentGroups).hasSize(1);
+ assertThat(parentGroups).containsExactly(groupUuid);
}
@Test
@@ -83,8 +87,10 @@
groupIndexer.index(groupUuid);
- List<InternalGroup> parentGroups = groupQueryProvider.get().bySubgroup(subgroupUuid);
- assertThatGroups(parentGroups).onlyElement().groupUuid().isEqualTo(groupUuid);
+ Set<AccountGroup.UUID> parentGroups =
+ groupQueryProvider.get().bySubgroups(ImmutableSet.of(subgroupUuid)).get(subgroupUuid);
+ assertThat(parentGroups).hasSize(1);
+ assertThat(parentGroups).containsExactly(groupUuid);
}
@Test
@@ -111,8 +117,10 @@
groupIndexer.reindexIfStale(groupUuid);
- List<InternalGroup> parentGroups = groupQueryProvider.get().bySubgroup(subgroupUuid);
- assertThatGroups(parentGroups).onlyElement().groupUuid().isEqualTo(groupUuid);
+ Set<AccountGroup.UUID> parentGroups =
+ groupQueryProvider.get().bySubgroups(ImmutableSet.of(subgroupUuid)).get(subgroupUuid);
+ assertThat(parentGroups).hasSize(1);
+ assertThat(parentGroups).containsExactly(groupUuid);
}
@Test
@@ -137,6 +145,20 @@
assertWithMessage("Group should have been reindexed").that(reindexed).isTrue();
}
+ @Test
+ public void getMultipleParents() throws Exception {
+ AccountGroup.UUID sub1 = groupOperations.newGroup().create();
+ AccountGroup.UUID sub2 = groupOperations.newGroup().create();
+ AccountGroup.UUID parent1 = groupOperations.newGroup().addSubgroup(sub1).create();
+ AccountGroup.UUID parent2 = groupOperations.newGroup().addSubgroup(sub2).create();
+ AccountGroup.UUID parent3 = groupOperations.newGroup().addSubgroup(sub2).create();
+
+ assertThat(groupQueryProvider.get().bySubgroups(ImmutableSet.of(sub1, sub2)))
+ .containsExactlyEntriesIn(
+ ImmutableMap.of(
+ sub1, ImmutableSet.of(parent1), sub2, ImmutableSet.of(parent2, parent3)));
+ }
+
private AccountGroup.UUID createGroup(String name) throws RestApiException {
GroupInfo group = gApi.groups().create(name).get();
return AccountGroup.uuid(group.id);
@@ -164,9 +186,4 @@
Optional<InternalGroup> updatedGroup) {
return assertThat(updatedGroup, internalGroups());
}
-
- private static ListSubject<InternalGroupSubject, InternalGroup> assertThatGroups(
- List<InternalGroup> parentGroups) {
- return assertThat(parentGroups, internalGroups());
- }
}
