opendev/gerrit
Code Issues Proposed changes

Perform group lookups by AccountGroup.Id from the group index

Browse Source 
Now that we have the group index, we can use it to look up groups by
AccountGroup.Id. This change is a prerequisite for migrating groups to
NoteDb.

When migrating changes from ReviewDb to NoteDb, the author for the
NoteDb commit is looked up from the account cache. Account lookups
include accessing details about groups which are now retrieved from the
group index. For this reason, the site program MigrateToNoteDb needs to
set up the indices to be able to run successfully.

Change-Id: I86f85b6e418f3cad2dbfeacbf57c9375923221e6
This commit is contained in:
Alice Kober-Sotzek
2017-08-24 17:58:36 +02:00
parent 1e1a63a6bd
commit 2776014d30
11 changed files with 78 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files

19
gerrit-pgm/src/main/java/com/google/gerrit/pgm/MigrateToNoteDb.java
View File

@@ -20,8 +20,11 @@ import static java.util.stream.Collectors.joining;
import static java.util.stream.Collectors.toList;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.gerrit.elasticsearch.ElasticIndexModule;
import com.google.gerrit.extensions.config.FactoryModule;
import com.google.gerrit.lifecycle.LifecycleManager;
import com.google.gerrit.lucene.LuceneIndexModule;
import com.google.gerrit.pgm.util.BatchProgramModule;
import com.google.gerrit.pgm.util.RuntimeShutdown;
import com.google.gerrit.pgm.util.SiteProgram;
@@ -30,10 +33,11 @@ import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.index.DummyIndexModule;
import com.google.gerrit.server.index.IndexModule;
import com.google.gerrit.server.notedb.rebuild.NoteDbMigrator;
import com.google.inject.Inject;
import com.google.inject.Injector;
import com.google.inject.Module;
import com.google.inject.Provider;
import java.util.ArrayList;
import java.util.List;
@@ -160,12 +164,23 @@ public class MigrateToNoteDb extends SiteProgram {
public void configure() {
install(dbInjector.getInstance(BatchProgramModule.class));
bind(GitReferenceUpdated.class).toInstance(GitReferenceUpdated.DISABLED);
install(new DummyIndexModule());
install(getIndexModule());
factory(ChangeResource.Factory.class);
}
});
}
private Module getIndexModule() {
switch (IndexModule.getIndexType(dbInjector)) {
case LUCENE:
return LuceneIndexModule.singleVersionWithExplicitVersions(ImmutableMap.of(), threads);
case ELASTICSEARCH:
return ElasticIndexModule.singleVersionWithExplicitVersions(ImmutableMap.of(), threads);
default:
throw new IllegalStateException("unsupported index.type");
}
}
private void stop() {
try {
LifecycleManager m = sysManager;

7
gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java
View File

@@ -20,6 +20,7 @@ import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_USE
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Streams;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
@@ -33,6 +34,7 @@ import com.google.gerrit.server.account.externalids.ExternalIds;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.group.Groups;
import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.index.account.AccountIndexer;
import com.google.gerrit.server.query.account.InternalAccountQuery;
import com.google.gwtorm.server.OrmException;
@@ -46,7 +48,6 @@ import com.google.inject.name.Named;
import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.ExecutionException;
@@ -199,8 +200,8 @@ public class AccountCacheImpl implements AccountCache {
groups
.getGroupsWithMember(db, who)
.map(groupCache::get)
.map(AccountGroup::getGroupUUID)
.filter(Objects::nonNull)
.flatMap(Streams::stream)
.map(InternalGroup::getGroupUUID)
.collect(toImmutableSet());
try {

9
gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCache.java
View File

@@ -22,7 +22,14 @@ import java.util.Optional;
/** Tracks group objects in memory for efficient access. */
public interface GroupCache {
AccountGroup get(AccountGroup.Id groupId);
/**
* Looks up an internal group by its ID.
*
* @param groupId the ID of the internal group
* @return an {@code Optional} of the internal group, or an empty {@code Optional} if no internal
* group with this ID exists on this server or an error occurred during lookup
*/
Optional<InternalGroup> get(AccountGroup.Id groupId);
/**
* Looks up an internal group by its name.

35
gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java
View File

@@ -21,7 +21,6 @@ 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.gerrit.common.TimeUtil;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -57,7 +56,7 @@ public class GroupCacheImpl implements GroupCache {
return new CacheModule() {
@Override
protected void configure() {
cache(BYID_NAME, AccountGroup.Id.class, new TypeLiteral<Optional<AccountGroup>>() {})
cache(BYID_NAME, AccountGroup.Id.class, new TypeLiteral<Optional<InternalGroup>>() {})
.loader(ByIdLoader.class);
cache(BYNAME_NAME, String.class, new TypeLiteral<Optional<InternalGroup>>() {})
@@ -72,7 +71,7 @@ public class GroupCacheImpl implements GroupCache {
};
}
private final LoadingCache<AccountGroup.Id, Optional<AccountGroup>> byId;
private final LoadingCache<AccountGroup.Id, Optional<InternalGroup>> byId;
private final LoadingCache<String, Optional<InternalGroup>> byName;
private final LoadingCache<String, Optional<InternalGroup>> byUUID;
private final SchemaFactory<ReviewDb> schema;
@@ -81,7 +80,7 @@ public class GroupCacheImpl implements GroupCache {
@Inject
GroupCacheImpl(
@Named(BYID_NAME) LoadingCache<AccountGroup.Id, Optional<AccountGroup>> byId,
@Named(BYID_NAME) LoadingCache<AccountGroup.Id, Optional<InternalGroup>> byId,
@Named(BYNAME_NAME) LoadingCache<String, Optional<InternalGroup>> byName,
@Named(BYUUID_NAME) LoadingCache<String, Optional<InternalGroup>> byUUID,
SchemaFactory<ReviewDb> schema,
@@ -96,13 +95,12 @@ public class GroupCacheImpl implements GroupCache {
}
@Override
public AccountGroup get(AccountGroup.Id groupId) {
public Optional<InternalGroup> get(AccountGroup.Id groupId) {
try {
Optional<AccountGroup> g = byId.get(groupId);
return g.isPresent() ? g.get() : missing(groupId);
return byId.get(groupId);
} catch (ExecutionException e) {
log.warn("Cannot load group " + groupId, e);
return missing(groupId);
return Optional.empty();
}
}
@@ -171,26 +169,17 @@ public class GroupCacheImpl implements GroupCache {
indexer.get().index(group.getGroupUUID());
}
private static AccountGroup missing(AccountGroup.Id key) {
AccountGroup.NameKey name = new AccountGroup.NameKey("Deleted Group" + key);
return new AccountGroup(name, key, null, TimeUtil.nowTs());
}
static class ByIdLoader extends CacheLoader<AccountGroup.Id, Optional<AccountGroup>> {
private final SchemaFactory<ReviewDb> schema;
private final Groups groups;
static class ByIdLoader extends CacheLoader<AccountGroup.Id, Optional<InternalGroup>> {
private final Provider<InternalGroupQuery> groupQueryProvider;
@Inject
ByIdLoader(SchemaFactory<ReviewDb> sf, Groups groups) {
schema = sf;
this.groups = groups;
ByIdLoader(Provider<InternalGroupQuery> groupQueryProvider) {
this.groupQueryProvider = groupQueryProvider;
}
@Override
public Optional<AccountGroup> load(AccountGroup.Id key) throws Exception {
try (ReviewDb db = schema.open()) {
return groups.getGroup(db, key);
}
public Optional<InternalGroup> load(AccountGroup.Id key) throws Exception {
return groupQueryProvider.get().byId(key);
}
}

13
gerrit-server/src/main/java/com/google/gerrit/server/account/GroupControl.java
View File

@@ -21,12 +21,15 @@ import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.group.InternalGroupDescription;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.util.Optional;
/** Access control management for a group of accounts managed in Gerrit. */
public class GroupControl {
@@ -71,11 +74,11 @@ public class GroupControl {
}
public GroupControl controlFor(AccountGroup.Id groupId) throws NoSuchGroupException {
final AccountGroup group = groupCache.get(groupId);
if (group == null) {
throw new NoSuchGroupException(groupId);
}
return controlFor(GroupDescriptions.forAccountGroup(group));
Optional<InternalGroup> group = groupCache.get(groupId);
return group
.map(InternalGroupDescription::new)
.map(this::controlFor)
.orElseThrow(() -> new NoSuchGroupException(groupId));
}
public GroupControl controlFor(AccountGroup.UUID groupId) throws NoSuchGroupException {

7
gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java
View File

@@ -19,11 +19,13 @@ import static com.google.common.collect.ImmutableList.toImmutableList;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Streams;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.group.Groups;
import com.google.gerrit.server.group.InternalGroup;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.Inject;
@@ -33,7 +35,6 @@ import com.google.inject.TypeLiteral;
import com.google.inject.name.Named;
import java.util.Collection;
import java.util.Collections;
import java.util.Objects;
import java.util.concurrent.ExecutionException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -174,8 +175,8 @@ public class GroupIncludeCacheImpl implements GroupIncludeCache {
return groups
.getParentGroups(db, key)
.map(groupCache::get)
.map(AccountGroup::getGroupUUID)
.filter(Objects::nonNull)
.flatMap(Streams::stream)
.map(InternalGroup::getGroupUUID)
.collect(toImmutableList());
}
}

7
gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java
View File

@@ -56,6 +56,7 @@ import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Locale;
import java.util.Optional;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.PersonIdent;
@@ -198,10 +199,8 @@ public class CreateGroup implements RestModifyView<TopLevelResource, GroupInput>
new AccountGroup(createGroupArgs.getGroup(), groupId, uuid, TimeUtil.nowTs());
group.setVisibleToAll(createGroupArgs.visibleToAll);
if (createGroupArgs.ownerGroupId != null) {
AccountGroup ownerGroup = groupCache.get(createGroupArgs.ownerGroupId);
if (ownerGroup != null) {
group.setOwnerGroupUUID(ownerGroup.getGroupUUID());
}
Optional<InternalGroup> ownerGroup = groupCache.get(createGroupArgs.ownerGroupId);
ownerGroup.map(InternalGroup::getGroupUUID).ifPresent(group::setOwnerGroupUUID);
}
if (createGroupArgs.groupDescription != null) {
group.setDescription(createGroupArgs.groupDescription);

8
gerrit-server/src/main/java/com/google/gerrit/server/group/DbGroupMemberAuditListener.java
View File

@@ -152,7 +152,7 @@ class DbGroupMemberAuditListener implements GroupMemberAuditListener {
Account.Id accountId = m.getAccountId();
String userName = accountCache.get(accountId).getUserName();
AccountGroup.Id groupId = m.getAccountGroupId();
String groupName = groupCache.get(groupId).getName();
String groupName = getGroupName(groupId);
descriptions.add(
MessageFormat.format(
@@ -168,7 +168,7 @@ class DbGroupMemberAuditListener implements GroupMemberAuditListener {
AccountGroup.UUID groupUuid = m.getIncludeUUID();
String groupName = groupBackend.get(groupUuid).getName();
AccountGroup.Id targetGroupId = m.getGroupId();
String targetGroupName = groupCache.get(targetGroupId).getName();
String targetGroupName = getGroupName(targetGroupId);
descriptions.add(
MessageFormat.format(
@@ -178,6 +178,10 @@ class DbGroupMemberAuditListener implements GroupMemberAuditListener {
logOrmException(header, me, descriptions, e);
}
private String getGroupName(AccountGroup.Id groupId) {
return groupCache.get(groupId).map(InternalGroup::getName).orElse("Deleted group " + groupId);
}
private void logOrmException(String header, Account.Id me, Iterable<?> values, OrmException e) {
StringBuilder message = new StringBuilder(header);
message.append(" ");

12
gerrit-server/src/main/java/com/google/gerrit/server/group/Groups.java
View File

@@ -59,18 +59,6 @@ public class Groups {
return group.orElseThrow(() -> new NoSuchGroupException(groupUuid));
}
/**
* Returns the {@code AccountGroup} for the specified ID if it exists.
*
* @param db the {@code ReviewDb} instance to use for lookups
* @param groupId the ID of the group
* @return the found {@code AccountGroup} if it exists, or else an empty {@code Optional}
* @throws OrmException if the group couldn't be retrieved from ReviewDb
*/
public Optional<AccountGroup> getGroup(ReviewDb db, AccountGroup.Id groupId) throws OrmException {
return Optional.ofNullable(db.accountGroups().get(groupId));
}
/**
* Returns the {@code AccountGroup} for the specified UUID if it exists.
*

4
gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupPredicates.java
View File

@@ -24,6 +24,10 @@ import com.google.gerrit.server.index.group.GroupField;
import java.util.Locale;
public class GroupPredicates {
public static Predicate<InternalGroup> id(AccountGroup.Id groupId) {
return new GroupPredicate(GroupField.ID, groupId.toString());
}
public static Predicate<InternalGroup> uuid(AccountGroup.UUID uuid) {
return new GroupPredicate(GroupField.UUID, GroupQueryBuilder.FIELD_UUID, uuid.get());
}

14
gerrit-server/src/main/java/com/google/gerrit/server/query/group/InternalGroupQuery.java
View File

@@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.index.query.InternalQuery;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.index.group.GroupIndexCollection;
@@ -46,7 +47,16 @@ public class InternalGroupQuery extends InternalQuery<InternalGroup> {
}
public Optional<InternalGroup> byName(AccountGroup.NameKey groupName) throws OrmException {
List<InternalGroup> groups = query(GroupPredicates.name(groupName.get()));
return getOnlyGroup(GroupPredicates.name(groupName.get()), "group name '" + groupName + "'");
}
public Optional<InternalGroup> byId(AccountGroup.Id groupId) throws OrmException {
return getOnlyGroup(GroupPredicates.id(groupId), "group id '" + groupId + "'");
}
private Optional<InternalGroup> getOnlyGroup(
Predicate<InternalGroup> predicate, String groupDescription) throws OrmException {
List<InternalGroup> groups = query(predicate);
if (groups.isEmpty()) {
return Optional.empty();
}
@@ -57,7 +67,7 @@ public class InternalGroupQuery extends InternalQuery<InternalGroup> {
ImmutableList<AccountGroup.UUID> groupUuids =
groups.stream().map(InternalGroup::getGroupUUID).collect(toImmutableList());
log.warn(String.format("Ambiguous group name '%s' for groups %s.", groupName, groupUuids));
log.warn(String.format("Ambiguous %s for groups %s.", groupDescription, groupUuids));
return Optional.empty();
}
}