diff --git a/java/com/google/gerrit/server/account/GroupCache.java b/java/com/google/gerrit/server/account/GroupCache.java index 224ac1465e..aaae95a51d 100644 --- a/java/com/google/gerrit/server/account/GroupCache.java +++ b/java/com/google/gerrit/server/account/GroupCache.java @@ -16,6 +16,8 @@ package com.google.gerrit.server.account; import com.google.gerrit.entities.AccountGroup; import com.google.gerrit.entities.InternalGroup; +import java.util.Collection; +import java.util.Map; import java.util.Optional; /** Tracks group objects in memory for efficient access. */ @@ -47,6 +49,18 @@ public interface GroupCache { */ Optional get(AccountGroup.UUID groupUuid); + /** + * Returns a {@code Map} of {@code AccountGroup.UUID} to {@code InternalGroup} for the given + * groups UUIDs. If not cached yet the groups are loaded. If a group can't be loaded (e.g. because + * it is missing), the entry will be missing from the result. + * + * @param groupUuids UUIDs of the groups that should be retrieved + * @return {@code Map} of {@code AccountGroup.UUID} to {@code InternalGroup} instances for the + * given group UUIDs, if a group can't be loaded (e.g. because it is missing), the entry will + * be missing from the result. + */ + Map get(Collection groupUuids); + /** * Removes the association of the given ID with a group. * @@ -88,4 +102,7 @@ public interface GroupCache { * @param groupUuid the UUID of a possibly associated group */ void evict(AccountGroup.UUID groupUuid); + + /** @see #evict(AccountGroup.UUID); */ + void evict(Collection groupUuid); } diff --git a/java/com/google/gerrit/server/account/GroupCacheImpl.java b/java/com/google/gerrit/server/account/GroupCacheImpl.java index 6f06dd3b91..eaec9baecd 100644 --- a/java/com/google/gerrit/server/account/GroupCacheImpl.java +++ b/java/com/google/gerrit/server/account/GroupCacheImpl.java @@ -14,8 +14,14 @@ package com.google.gerrit.server.account; +import static com.google.common.collect.ImmutableMap.toImmutableMap; +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.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; import com.google.common.flogger.FluentLogger; import com.google.gerrit.entities.AccountGroup; import com.google.gerrit.entities.InternalGroup; @@ -40,7 +46,14 @@ import com.google.inject.Provider; import com.google.inject.Singleton; import com.google.inject.TypeLiteral; import com.google.inject.name.Named; +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashMap; +import java.util.Iterator; +import java.util.List; +import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.concurrent.ExecutionException; import org.bouncycastle.util.Strings; import org.eclipse.jgit.lib.ObjectId; @@ -156,6 +169,20 @@ public class GroupCacheImpl implements GroupCache { } } + @Override + public Map get(Collection groupUuids) { + try { + Set groupUuidsStringSet = + groupUuids.stream().map(u -> u.get()).collect(toImmutableSet()); + return byUUID.getAll(groupUuidsStringSet).entrySet().stream() + .filter(g -> g.getValue().isPresent()) + .collect(toImmutableMap(g -> AccountGroup.uuid(g.getKey()), g -> g.getValue().get())); + } catch (ExecutionException e) { + logger.atWarning().withCause(e).log("Cannot look up groups %s by uuids", groupUuids); + return ImmutableMap.of(); + } + } + @Override public void evict(AccountGroup.Id groupId) { if (groupId != null) { @@ -180,6 +207,14 @@ public class GroupCacheImpl implements GroupCache { } } + @Override + public void evict(Collection groupUuids) { + if (groupUuids != null && !groupUuids.isEmpty()) { + logger.atFine().log("Evict groups %s by UUID", groupUuids); + byUUID.invalidateAll(groupUuids); + } + } + static class ByIdLoader extends CacheLoader> { private final Provider groupQueryProvider; @@ -234,23 +269,42 @@ public class GroupCacheImpl implements GroupCache { @Override public Optional load(String uuid) throws Exception { + return loadAll(ImmutableSet.of(uuid)).get(uuid); + } + + @Override + public Map> loadAll(Iterable uuids) + throws Exception { + Map> toReturn = new HashMap<>(); + if (Iterables.isEmpty(uuids)) { + return toReturn; + } + Iterator uuidIterator = uuids.iterator(); + List keyList = new ArrayList<>(); try (TraceTimer ignored = TraceContext.newTimer( "Loading group from serialized cache", - Metadata.builder().groupUuid(uuid).build()); + Metadata.builder().cacheName(BYUUID_NAME_PERSISTED).build()); Repository allUsers = repoManager.openRepository(allUsersName)) { - String ref = RefNames.refsGroups(AccountGroup.uuid(uuid)); - Ref sha1 = allUsers.exactRef(ref); - if (sha1 == null) { - return Optional.empty(); + while (uuidIterator.hasNext()) { + String currentUuid = uuidIterator.next(); + String ref = RefNames.refsGroups(AccountGroup.uuid(currentUuid)); + Ref sha1 = allUsers.exactRef(ref); + if (sha1 == null) { + toReturn.put(currentUuid, Optional.empty()); + continue; + } + Cache.GroupKeyProto key = + Cache.GroupKeyProto.newBuilder() + .setUuid(currentUuid) + .setRevision(ObjectIdConverter.create().toByteString(sha1.getObjectId())) + .build(); + keyList.add(key); } - Cache.GroupKeyProto key = - Cache.GroupKeyProto.newBuilder() - .setUuid(uuid) - .setRevision(ObjectIdConverter.create().toByteString(sha1.getObjectId())) - .build(); - return Optional.of(persistedCache.get(key)); } + persistedCache.getAll(keyList).entrySet().stream() + .forEach(g -> toReturn.put(g.getKey().getUuid(), Optional.of(g.getValue()))); + return toReturn; } } diff --git a/java/com/google/gerrit/server/account/IncludingGroupMembership.java b/java/com/google/gerrit/server/account/IncludingGroupMembership.java index a277fac347..8cec8bf1c9 100644 --- a/java/com/google/gerrit/server/account/IncludingGroupMembership.java +++ b/java/com/google/gerrit/server/account/IncludingGroupMembership.java @@ -27,7 +27,6 @@ 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; @@ -83,6 +82,9 @@ public class IncludingGroupMembership implements GroupMembership { } if (tryExpanding) { + Set queryIdsSet = new HashSet<>(); + queryIds.forEach(i -> queryIdsSet.add(i)); + Map groups = groupCache.get(queryIdsSet); for (AccountGroup.UUID id : queryIds) { if (memberOf.containsKey(id)) { // Membership was earlier proven to be false. @@ -90,15 +92,15 @@ public class IncludingGroupMembership implements GroupMembership { } memberOf.put(id, false); - Optional group = groupCache.get(id); - if (!group.isPresent()) { + InternalGroup group = groups.get(id); + if (group == null) { continue; } - if (user.isIdentifiedUser() && group.get().getMembers().contains(user.getAccountId())) { + if (user.isIdentifiedUser() && group.getMembers().contains(user.getAccountId())) { memberOf.put(id, true); return true; } - if (search(group.get().getSubgroups())) { + if (search(group.getSubgroups())) { memberOf.put(id, true); return true; } diff --git a/java/com/google/gerrit/server/index/group/AllGroupsIndexer.java b/java/com/google/gerrit/server/index/group/AllGroupsIndexer.java index 3edd26ba40..b3ef679675 100644 --- a/java/com/google/gerrit/server/index/group/AllGroupsIndexer.java +++ b/java/com/google/gerrit/server/index/group/AllGroupsIndexer.java @@ -35,7 +35,7 @@ import com.google.inject.Singleton; import java.io.IOException; import java.util.ArrayList; import java.util.List; -import java.util.Optional; +import java.util.Map; import java.util.concurrent.ExecutionException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; @@ -88,16 +88,17 @@ public class AllGroupsIndexer extends SiteIndexer reindexedGroups = groupCache.get(uuids); for (AccountGroup.UUID uuid : uuids) { String desc = "group " + uuid; ListenableFuture future = executor.submit( () -> { try { - groupCache.evict(uuid); - Optional internalGroup = groupCache.get(uuid); - if (internalGroup.isPresent()) { - index.replace(internalGroup.get()); + InternalGroup internalGroup = reindexedGroups.get(uuid); + if (internalGroup != null) { + index.replace(internalGroup); } else { index.delete(uuid); diff --git a/java/com/google/gerrit/server/restapi/group/GetAuditLog.java b/java/com/google/gerrit/server/restapi/group/GetAuditLog.java index 706a1f4272..e3aa0f3697 100644 --- a/java/com/google/gerrit/server/restapi/group/GetAuditLog.java +++ b/java/com/google/gerrit/server/restapi/group/GetAuditLog.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.restapi.group; +import static com.google.common.collect.ImmutableList.toImmutableList; import static java.util.Comparator.comparing; import com.google.gerrit.entities.AccountGroup; @@ -42,7 +43,7 @@ import com.google.inject.Singleton; import java.io.IOException; import java.util.ArrayList; import java.util.List; -import java.util.Optional; +import java.util.Map; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Repository; @@ -105,14 +106,17 @@ public class GetAuditLog implements RestReadView { member)); } } - - for (AccountGroupByIdAudit auditEvent : - groups.getSubgroupsAudit(allUsersRepo, group.getGroupUUID())) { + List subGroupsAudit = + groups.getSubgroupsAudit(allUsersRepo, group.getGroupUUID()); + Map groups = + groupCache.get( + subGroupsAudit.stream().map(a -> a.includeUuid()).collect(toImmutableList())); + for (AccountGroupByIdAudit auditEvent : subGroupsAudit) { AccountGroup.UUID includedGroupUUID = auditEvent.includeUuid(); - Optional includedGroup = groupCache.get(includedGroupUUID); + InternalGroup includedGroup = groups.get(includedGroupUUID); GroupInfo member; - if (includedGroup.isPresent()) { - member = groupJson.format(new InternalGroupDescription(includedGroup.get())); + if (includedGroup != null) { + member = groupJson.format(new InternalGroupDescription(includedGroup)); } else { member = new GroupInfo(); member.id = Url.encode(includedGroupUUID.get()); diff --git a/java/com/google/gerrit/server/restapi/group/ListGroups.java b/java/com/google/gerrit/server/restapi/group/ListGroups.java index 3e2a577eae..96402be88a 100644 --- a/java/com/google/gerrit/server/restapi/group/ListGroups.java +++ b/java/com/google/gerrit/server/restapi/group/ListGroups.java @@ -15,12 +15,12 @@ package com.google.gerrit.server.restapi.group; import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static java.util.stream.Collectors.toList; import com.google.common.base.MoreObjects; import com.google.common.base.Strings; import com.google.common.collect.Lists; -import com.google.common.collect.Streams; import com.google.gerrit.entities.Account; import com.google.gerrit.entities.AccountGroup; import com.google.gerrit.entities.GroupDescription; @@ -57,7 +57,6 @@ import java.util.EnumSet; import java.util.HashSet; import java.util.List; import java.util.Locale; -import java.util.Optional; import java.util.Set; import java.util.SortedMap; import java.util.TreeMap; @@ -273,10 +272,12 @@ public class ListGroups implements RestReadView { throws IOException, ConfigInvalidException, PermissionBackendException { Pattern pattern = getRegexPattern(); Stream existingGroups = - getAllExistingGroups() - .filter(group -> isRelevant(pattern, group)) - .map(this::loadGroup) - .flatMap(Streams::stream) + loadGroups( + getAllExistingGroups() + .filter(group -> isRelevant(pattern, group)) + .map(g -> g.getUUID()) + .collect(toImmutableSet())) + .stream() .filter(this::isVisible) .sorted(GROUP_COMPARATOR) .skip(start); @@ -359,11 +360,13 @@ public class ListGroups implements RestReadView { throws IOException, ConfigInvalidException, PermissionBackendException { Pattern pattern = getRegexPattern(); Stream foundGroups = - groups - .getAllGroupReferences() - .filter(group -> isRelevant(pattern, group)) - .map(this::loadGroup) - .flatMap(Streams::stream) + loadGroups( + groups + .getAllGroupReferences() + .filter(group -> isRelevant(pattern, group)) + .map(g -> g.getUUID()) + .collect(toImmutableSet())) + .stream() .filter(this::isVisible) .filter(filter) .sorted(GROUP_COMPARATOR) @@ -379,8 +382,10 @@ public class ListGroups implements RestReadView { return groupInfos; } - private Optional loadGroup(GroupReference groupReference) { - return groupCache.get(groupReference.getUUID()).map(InternalGroupDescription::new); + private Set loadGroups(Collection groupUuids) { + return groupCache.get(groupUuids).values().stream() + .map(InternalGroupDescription::new) + .collect(toImmutableSet()); } private List getGroupsOwnedBy(String id) diff --git a/java/com/google/gerrit/server/restapi/group/ListMembers.java b/java/com/google/gerrit/server/restapi/group/ListMembers.java index cba6408dea..218eb98056 100644 --- a/java/com/google/gerrit/server/restapi/group/ListMembers.java +++ b/java/com/google/gerrit/server/restapi/group/ListMembers.java @@ -144,23 +144,21 @@ public class ListMembers implements RestReadView { private Set getIndirectMemberIds( GroupDescription.Internal group, HashSet seenGroups) { Set indirectMembers = new HashSet<>(); + Set subgroupMembersToLoad = new HashSet<>(); for (AccountGroup.UUID subgroupUuid : group.getSubgroups()) { if (!seenGroups.contains(subgroupUuid)) { seenGroups.add(subgroupUuid); - - Set subgroupMembers = - groupCache - .get(subgroupUuid) - .map(InternalGroupDescription::new) - .map( - subgroup -> { - GroupControl subgroupControl = groupControlFactory.controlFor(subgroup); - return getTransitiveMemberIds(subgroup, subgroupControl, seenGroups); - }) - .orElseGet(ImmutableSet::of); - indirectMembers.addAll(subgroupMembers); + subgroupMembersToLoad.add(subgroupUuid); } } + groupCache.get(subgroupMembersToLoad).values().stream() + .map(InternalGroupDescription::new) + .forEach( + subgroup -> { + GroupControl subgroupControl = groupControlFactory.controlFor(subgroup); + indirectMembers.addAll(getTransitiveMemberIds(subgroup, subgroupControl, seenGroups)); + }); + return indirectMembers; }