Perform group lookups by AccountGroup.Id from the group index
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
diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/MigrateToNoteDb.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/MigrateToNoteDb.java
index 5fbd411..dda8d14 100644
--- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/MigrateToNoteDb.java
+++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/MigrateToNoteDb.java
@@ -20,8 +20,11 @@
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.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 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;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java
index 4c6f670..44ffb90 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java
@@ -20,6 +20,7 @@
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.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 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 @@
groups
.getGroupsWithMember(db, who)
.map(groupCache::get)
- .map(AccountGroup::getGroupUUID)
- .filter(Objects::nonNull)
+ .flatMap(Streams::stream)
+ .map(InternalGroup::getGroupUUID)
.collect(toImmutableSet());
try {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCache.java
index 122c0bc..82bcce3 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCache.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCache.java
@@ -22,7 +22,14 @@
/** 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.
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java
index c95517b..988439c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java
@@ -21,7 +21,6 @@
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 @@
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 @@
};
}
- 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 @@
@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 @@
}
@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 @@
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);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupControl.java
index 5af4898..020a04d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupControl.java
@@ -21,12 +21,15 @@
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 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 {
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 10c002c..c686928 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
@@ -19,11 +19,13 @@
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.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 @@
return groups
.getParentGroups(db, key)
.map(groupCache::get)
- .map(AccountGroup::getGroupUUID)
- .filter(Objects::nonNull)
+ .flatMap(Streams::stream)
+ .map(InternalGroup::getGroupUUID)
.collect(toImmutableList());
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java
index a1aefc5..e55397e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java
@@ -56,6 +56,7 @@
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 @@
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);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/DbGroupMemberAuditListener.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/DbGroupMemberAuditListener.java
index ce287d0..5af7ebd 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/DbGroupMemberAuditListener.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/DbGroupMemberAuditListener.java
@@ -152,7 +152,7 @@
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 @@
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 @@
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(" ");
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/Groups.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/Groups.java
index a1947ec..1e5d634 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/Groups.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/Groups.java
@@ -60,18 +60,6 @@
}
/**
- * 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.
*
* @param db the {@code ReviewDb} instance to use for lookups
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupPredicates.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupPredicates.java
index 983d3b3..d02f6a4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupPredicates.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupPredicates.java
@@ -24,6 +24,10 @@
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());
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/group/InternalGroupQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/group/InternalGroupQuery.java
index b261e25..513fffe 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/group/InternalGroupQuery.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/group/InternalGroupQuery.java
@@ -20,6 +20,7 @@
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 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 @@
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();
}
}
