opendev/gerrit
Code Issues Proposed changes

Rely on the group index for lookups per member/subgroup

Browse Source 
Due to Idc7bd2bf3, we now also have a group index when in slave mode.
Additionally, we require an offline reindex when upgrading to the
current version of Gerrit (I5143cd0d0) and hence can use all fields of
the group index without defining fallbacks. Thanks to those two aspects,
we can always use the group index when looking up (parent) groups per
member/subgroup. This means that we can also get rid of the code which
required us to stay on ReviewDb for groups until now.

We also checked for the existence for some of the fields of the group
index in other parts of the code. Those checks aren't necessary anymore
either and hence removed.

Change-Id: I47c98855a5da0d6b3a0aa2a484997e1fb487601c
This commit is contained in:
Alice Kober-Sotzek
2018-02-08 17:14:21 +01:00
parent 0f0de8624d
commit 77037b8cad
6 changed files with 23 additions and 194 deletions
Download Patch File Download Diff File Expand all files Collapse all files

19
java/com/google/gerrit/server/account/GroupCacheImpl.java
View File

@@ -21,7 +21,6 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.group.db.Groups;
import com.google.gerrit.server.index.group.GroupIndexCollection;
import com.google.gerrit.server.query.group.InternalGroupQuery;
import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.Inject;
@@ -32,7 +31,6 @@ import com.google.inject.TypeLiteral;
import com.google.inject.name.Named;
import java.util.Optional;
import java.util.concurrent.ExecutionException;
import java.util.function.BooleanSupplier;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -140,29 +138,16 @@ public class GroupCacheImpl implements GroupCache {
}
static class ByIdLoader extends CacheLoader<AccountGroup.Id, Optional<InternalGroup>> {
private final SchemaFactory<ReviewDb> schema;
private final BooleanSupplier hasGroupIndex;
private final Provider<InternalGroupQuery> groupQueryProvider;
@Inject
ByIdLoader(
SchemaFactory<ReviewDb> schema,
GroupIndexCollection groupIndexCollection,
Provider<InternalGroupQuery> groupQueryProvider) {
this.schema = schema;
hasGroupIndex = () -> groupIndexCollection.getSearchIndex() != null;
ByIdLoader(Provider<InternalGroupQuery> groupQueryProvider) {
this.groupQueryProvider = groupQueryProvider;
}
@Override
public Optional<InternalGroup> load(AccountGroup.Id key) throws Exception {
if (hasGroupIndex.getAsBoolean()) {
return groupQueryProvider.get().byId(key);
}
try (ReviewDb db = schema.open()) {
return Groups.getGroupFromReviewDb(db, key);
}
return groupQueryProvider.get().byId(key);
}
}

72
java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java
View File

@@ -21,16 +21,12 @@ 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.reviewdb.client.Account;
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.InternalGroup;
import com.google.gerrit.server.group.db.Groups;
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.gwtorm.server.SchemaFactory;
@@ -145,81 +141,41 @@ 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;
GroupsWithMemberLoader(Provider<InternalGroupQuery> groupQueryProvider) {
this.groupQueryProvider = groupQueryProvider;
this.groupCache = groupCache;
}
@Override
public ImmutableSet<AccountGroup.UUID> load(Account.Id memberId) throws OrmException {
GroupIndex groupIndex = groupIndexProvider.get();
if (groupIndex != null && groupIndex.getSchema().hasField(GroupField.MEMBER)) {
return groupQueryProvider
.get()
.byMember(memberId)
.stream()
.map(InternalGroup::getGroupUUID)
.collect(toImmutableSet());
}
try (ReviewDb db = schema.open()) {
return Groups.getGroupsWithMemberFromReviewDb(db, memberId)
.map(groupCache::get)
.flatMap(Streams::stream)
.map(InternalGroup::getGroupUUID)
.collect(toImmutableSet());
}
return groupQueryProvider
.get()
.byMember(memberId)
.stream()
.map(InternalGroup::getGroupUUID)
.collect(toImmutableSet());
}
}
static class ParentGroupsLoader
extends CacheLoader<AccountGroup.UUID, ImmutableList<AccountGroup.UUID>> {
private final SchemaFactory<ReviewDb> schema;
private final Provider<GroupIndex> groupIndexProvider;
private final Provider<InternalGroupQuery> groupQueryProvider;
private final GroupCache groupCache;
@Inject
ParentGroupsLoader(
SchemaFactory<ReviewDb> sf,
GroupIndexCollection groupIndexCollection,
Provider<InternalGroupQuery> groupQueryProvider,
GroupCache groupCache) {
schema = sf;
this.groupIndexProvider = groupIndexCollection::getSearchIndex;
ParentGroupsLoader(Provider<InternalGroupQuery> groupQueryProvider) {
this.groupQueryProvider = groupQueryProvider;
this.groupCache = groupCache;
}
@Override
public ImmutableList<AccountGroup.UUID> load(AccountGroup.UUID key) throws OrmException {
GroupIndex groupIndex = groupIndexProvider.get();
if (groupIndex != null && groupIndex.getSchema().hasField(GroupField.SUBGROUP)) {
return groupQueryProvider
.get()
.bySubgroup(key)
.stream()
.map(InternalGroup::getGroupUUID)
.collect(toImmutableList());
}
try (ReviewDb db = schema.open()) {
return Groups.getParentGroupsFromReviewDb(db, key)
.map(groupCache::get)
.flatMap(Streams::stream)
.map(InternalGroup::getGroupUUID)
.collect(toImmutableList());
}
return groupQueryProvider
.get()
.bySubgroup(key)
.stream()
.map(InternalGroup::getGroupUUID)
.collect(toImmutableList());
}
}

62
java/com/google/gerrit/server/group/db/Groups.java
View File

@@ -77,23 +77,6 @@ public class Groups {
this.auditLogReader = auditLogReader;
}
/**
* 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 static Optional<InternalGroup> getGroupFromReviewDb(ReviewDb db, AccountGroup.Id groupId)
throws OrmException {
AccountGroup accountGroup = db.accountGroups().get(groupId);
if (accountGroup == null) {
return Optional.empty();
}
return Optional.of(asInternalGroup(db, accountGroup));
}
/**
* Returns the {@code InternalGroup} for the specified UUID if it exists.
*
@@ -210,7 +193,7 @@ public class Groups {
* @return a stream of the IDs of the members
* @throws OrmException if an error occurs while reading from ReviewDb
*/
public static Stream<Account.Id> getMembersFromReviewDb(ReviewDb db, AccountGroup.Id groupId)
static Stream<Account.Id> getMembersFromReviewDb(ReviewDb db, AccountGroup.Id groupId)
throws OrmException {
ResultSet<AccountGroupMember> accountGroupMembers = db.accountGroupMembers().byGroup(groupId);
return Streams.stream(accountGroupMembers).map(AccountGroupMember::getAccountId);
@@ -229,51 +212,12 @@ public class Groups {
* @return a stream of the UUIDs of the subgroups
* @throws OrmException if an error occurs while reading from ReviewDb
*/
public static Stream<AccountGroup.UUID> getSubgroupsFromReviewDb(
ReviewDb db, AccountGroup.Id groupId) throws OrmException {
static Stream<AccountGroup.UUID> getSubgroupsFromReviewDb(ReviewDb db, AccountGroup.Id groupId)
throws OrmException {
ResultSet<AccountGroupById> accountGroupByIds = db.accountGroupById().byGroup(groupId);
return Streams.stream(accountGroupByIds).map(AccountGroupById::getIncludeUUID).distinct();
}
/**
* Returns the groups of which the specified account is a member.
*
* <p><strong>Note</strong>: This method returns an empty stream if the account doesn't exist.
* This method doesn't check whether the groups exist.
*
* @param db the {@code ReviewDb} instance to use for lookups
* @param accountId the ID of the account
* @return a stream of the IDs of the groups of which the account is a member
* @throws OrmException if an error occurs while reading from ReviewDb
*/
public static Stream<AccountGroup.Id> getGroupsWithMemberFromReviewDb(
ReviewDb db, Account.Id accountId) throws OrmException {
ResultSet<AccountGroupMember> accountGroupMembers =
db.accountGroupMembers().byAccount(accountId);
return Streams.stream(accountGroupMembers).map(AccountGroupMember::getAccountGroupId);
}
/**
* Returns the parent groups of the specified (sub)group.
*
* <p>The subgroup may either be an internal or an external group whereas the returned parent
* groups represent only internal groups.
*
* <p><strong>Note</strong>: This method returns an empty stream if the specified group doesn't
* exist. This method doesn't check whether the parent groups exist.
*
* @param db the {@code ReviewDb} instance to use for lookups
* @param subgroupUuid the UUID of the subgroup
* @return a stream of the IDs of the parent groups
* @throws OrmException if an error occurs while reading from ReviewDb
*/
public static Stream<AccountGroup.Id> getParentGroupsFromReviewDb(
ReviewDb db, AccountGroup.UUID subgroupUuid) throws OrmException {
ResultSet<AccountGroupById> accountGroupByIds =
db.accountGroupById().byIncludeUUID(subgroupUuid);
return Streams.stream(accountGroupByIds).map(AccountGroupById::getGroupId);
}
/**
* Returns all known external groups. External groups are 'known' when they are specified as a
* subgroup of an internal group.

3
java/com/google/gerrit/server/index/group/StalenessChecker.java
View File

@@ -72,9 +72,6 @@ public class StalenessChecker {
if (i == null) {
return false; // No index; caller couldn't do anything if it is stale.
}
if (!i.getSchema().hasField(GroupField.REF_STATE)) {
return false; // Index version not new enough for this check.
}
Optional<FieldBundle> result =
i.getRaw(uuid, IndexedGroupQuery.createOptions(indexConfig, 0, 1, FIELDS));

29
java/com/google/gerrit/server/query/group/GroupQueryBuilder.java
View File

@@ -20,7 +20,6 @@ import com.google.common.base.Strings;
import com.google.common.collect.Lists;
import com.google.common.primitives.Ints;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.query.LimitPredicate;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryBuilder;
@@ -32,9 +31,6 @@ import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.account.GroupBackends;
import com.google.gerrit.server.account.GroupCache;
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.gwtorm.server.OrmException;
import com.google.inject.Inject;
import java.io.IOException;
@@ -56,18 +52,12 @@ public class GroupQueryBuilder extends QueryBuilder<InternalGroup> {
new QueryBuilder.Definition<>(GroupQueryBuilder.class);
public static class Arguments {
final GroupIndex groupIndex;
final GroupCache groupCache;
final GroupBackend groupBackend;
final AccountResolver accountResolver;
@Inject
Arguments(
GroupIndexCollection groupIndexCollection,
GroupCache groupCache,
GroupBackend groupBackend,
AccountResolver accountResolver) {
this.groupIndex = groupIndexCollection.getSearchIndex();
Arguments(GroupCache groupCache, GroupBackend groupBackend, AccountResolver accountResolver) {
this.groupCache = groupCache;
this.groupBackend = groupBackend;
this.accountResolver = accountResolver;
@@ -144,10 +134,6 @@ public class GroupQueryBuilder extends QueryBuilder<InternalGroup> {
@Operator
public Predicate<InternalGroup> member(String query)
throws QueryParseException, OrmException, ConfigInvalidException, IOException {
if (isFieldAbsentFromIndex(GroupField.MEMBER)) {
throw getExceptionForUnsupportedOperator("member");
}
Set<Account.Id> accounts = parseAccount(query);
List<Predicate<InternalGroup>> predicates =
accounts.stream().map(GroupPredicates::member).collect(toImmutableList());
@@ -156,10 +142,6 @@ public class GroupQueryBuilder extends QueryBuilder<InternalGroup> {
@Operator
public Predicate<InternalGroup> subgroup(String query) throws QueryParseException {
if (isFieldAbsentFromIndex(GroupField.SUBGROUP)) {
throw getExceptionForUnsupportedOperator("subgroup");
}
AccountGroup.UUID groupUuid = parseGroup(query);
return GroupPredicates.subgroup(groupUuid);
}
@@ -173,15 +155,6 @@ public class GroupQueryBuilder extends QueryBuilder<InternalGroup> {
return new LimitPredicate<>(FIELD_LIMIT, limit);
}
private boolean isFieldAbsentFromIndex(FieldDef<InternalGroup, ?> field) {
return !args.groupIndex.getSchema().hasField(field);
}
private static QueryParseException getExceptionForUnsupportedOperator(String operatorName) {
return new QueryParseException(
String.format("'%s' operator is not supported by group index version", operatorName));
}
private Set<Account.Id> parseAccount(String nameOrEmail)
throws QueryParseException, OrmException, IOException, ConfigInvalidException {
Set<Account.Id> foundAccounts = args.accountResolver.findAll(nameOrEmail);

32
javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java
View File

@@ -16,8 +16,8 @@ package com.google.gerrit.server.query.group;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;
import static com.google.common.truth.TruthJUnit.assume;
import static java.util.stream.Collectors.toList;
import static org.junit.Assert.fail;
import com.google.common.base.CharMatcher;
import com.google.gerrit.extensions.api.GerritApi;
@@ -27,7 +27,6 @@ import com.google.gerrit.extensions.api.groups.Groups.QueryRequest;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.GroupInfo;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.index.QueryOptions;
import com.google.gerrit.index.Schema;
@@ -263,12 +262,7 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests {
@Test
public void byMember() throws Exception {
if (getSchemaVersion() < 4) {
assertMissingField(GroupField.MEMBER);
assertFailingQuery(
"member:someName", "'member' operator is not supported by group index version");
return;
}
assume().that(getSchemaVersion() >= 4).isTrue();
AccountInfo user1 = createAccount("user1", "User1", "user1@example.com");
AccountInfo user2 = createAccount("user2", "User2", "user2@example.com");
@@ -288,12 +282,7 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests {
@Test
public void bySubgroups() throws Exception {
if (getSchemaVersion() < 4) {
assertMissingField(GroupField.SUBGROUP);
assertFailingQuery(
"subgroup:someGroupName", "'subgroup' operator is not supported by group index version");
return;
}
assume().that(getSchemaVersion() >= 4).isTrue();
GroupInfo superParentGroup = createGroup(name("superParentGroup"));
GroupInfo parentGroup1 = createGroup(name("parentGroup1"));
@@ -549,21 +538,6 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests {
return name + "_" + getSanitizedMethodName();
}
protected void assertMissingField(FieldDef<InternalGroup, ?> field) {
assertThat(getSchema().hasField(field))
.named("schema %s has field %s", getSchemaVersion(), field.getName())
.isFalse();
}
protected void assertFailingQuery(String query, String expectedMessage) throws Exception {
try {
assertQuery(query);
fail("expected BadRequestException for query '" + query + "'");
} catch (BadRequestException e) {
assertThat(e.getMessage()).isEqualTo(expectedMessage);
}
}
protected int getSchemaVersion() {
return getSchema().getVersion();
}