From 24a7d37a0c38c7d57498238541479a7bb42df39c Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Wed, 8 Nov 2017 17:14:40 +0100 Subject: [PATCH] Add staleness checker for groups in NoteDb This commit adds a new field to the group index to store the SHA1 of the groups ref that the index document belongs to. With that, staleness can be detected by comparing the value of the field to the current HEAD SHA1 on the groups ref. This mechanism is similar to what was used for changes and accounts. This commit also adds a staleness checker for groups as well as a test. Change-Id: I113546ecd27079c03bbd1db728a22483db0a7ee2 --- .../gerrit/server/group/InternalGroup.java | 17 ++++ .../gerrit/server/group/db/GroupConfig.java | 20 +++- .../gerrit/server/group/db/GroupsUpdate.java | 15 ++- .../gerrit/server/index/IndexUtils.java | 16 ++++ .../server/index/change/ChangeIndexer.java | 22 +---- .../gerrit/server/index/group/GroupField.java | 14 +++ .../server/index/group/GroupIndexerImpl.java | 63 +++++++++++++ .../index/group/GroupSchemaDefinitions.java | 3 + .../server/index/group/IndexedGroupQuery.java | 13 +++ .../server/index/group/StalenessChecker.java | 94 +++++++++++++++++++ .../server/query/group/GroupPredicates.java | 1 + .../server/query/group/GroupQueryBuilder.java | 13 +-- .../api/group/GroupRebuilderIT.java | 8 +- .../gerrit/acceptance/api/group/GroupsIT.java | 51 ++++++++++ .../server/group/db/GroupRebuilderTest.java | 6 +- .../query/group/AbstractQueryGroupsTest.java | 13 --- 16 files changed, 318 insertions(+), 51 deletions(-) create mode 100644 java/com/google/gerrit/server/index/group/StalenessChecker.java diff --git a/java/com/google/gerrit/server/group/InternalGroup.java b/java/com/google/gerrit/server/group/InternalGroup.java index ad6fa5c4c8..7828586271 100644 --- a/java/com/google/gerrit/server/group/InternalGroup.java +++ b/java/com/google/gerrit/server/group/InternalGroup.java @@ -21,6 +21,7 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; import java.io.Serializable; import java.sql.Timestamp; +import org.eclipse.jgit.lib.ObjectId; @AutoValue public abstract class InternalGroup implements Serializable { @@ -30,6 +31,14 @@ public abstract class InternalGroup implements Serializable { AccountGroup accountGroup, ImmutableSet members, ImmutableSet subgroups) { + return create(accountGroup, members, subgroups, null); + } + + public static InternalGroup create( + AccountGroup accountGroup, + ImmutableSet members, + ImmutableSet subgroups, + ObjectId refState) { return builder() .setId(accountGroup.getId()) .setNameKey(accountGroup.getNameKey()) @@ -40,6 +49,7 @@ public abstract class InternalGroup implements Serializable { .setCreatedOn(accountGroup.getCreatedOn()) .setMembers(members) .setSubgroups(subgroups) + .setRefState(refState) .build(); } @@ -66,6 +76,11 @@ public abstract class InternalGroup implements Serializable { public abstract ImmutableSet getSubgroups(); + @Nullable + public abstract ObjectId getRefState(); + + public abstract Builder toBuilder(); + public static Builder builder() { return new AutoValue_InternalGroup.Builder(); } @@ -90,6 +105,8 @@ public abstract class InternalGroup implements Serializable { public abstract Builder setSubgroups(ImmutableSet subgroups); + public abstract Builder setRefState(ObjectId refState); + public abstract InternalGroup build(); } } diff --git a/java/com/google/gerrit/server/group/db/GroupConfig.java b/java/com/google/gerrit/server/group/db/GroupConfig.java index 8ac533c51c..0cdcdecb76 100644 --- a/java/com/google/gerrit/server/group/db/GroupConfig.java +++ b/java/com/google/gerrit/server/group/db/GroupConfig.java @@ -25,6 +25,7 @@ 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.client.RefNames; +import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.git.VersionedMetaData; import com.google.gerrit.server.group.InternalGroup; import com.google.gwtorm.server.OrmDuplicateKeyException; @@ -40,6 +41,7 @@ import java.util.stream.Stream; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; @@ -123,7 +125,9 @@ public class GroupConfig extends VersionedMetaData { Config config = readConfig(GROUP_CONFIG_FILE); ImmutableSet members = readMembers(); ImmutableSet subgroups = readSubgroups(); - loadedGroup = Optional.of(createFrom(groupUuid, config, members, subgroups, createdOn)); + loadedGroup = + Optional.of( + createFrom(groupUuid, config, members, subgroups, createdOn, revision.toObjectId())); } isLoaded = true; @@ -134,7 +138,8 @@ public class GroupConfig extends VersionedMetaData { Config config, ImmutableSet members, ImmutableSet subgroups, - Timestamp createdOn) { + Timestamp createdOn, + ObjectId refState) { InternalGroup.Builder group = InternalGroup.builder(); group.setGroupUUID(groupUuid); Arrays.stream(GroupConfigEntry.values()) @@ -142,9 +147,17 @@ public class GroupConfig extends VersionedMetaData { group.setMembers(members); group.setSubgroups(subgroups); group.setCreatedOn(createdOn); + group.setRefState(refState); return group.build(); } + @Override + public RevCommit commit(MetaDataUpdate update) throws IOException { + RevCommit c = super.commit(update); + loadedGroup = Optional.of(loadedGroup.get().toBuilder().setRefState(c.toObjectId()).build()); + return c; + } + @Override protected boolean onSave(CommitBuilder commit) throws IOException, ConfigInvalidException { checkLoaded(); @@ -184,7 +197,8 @@ public class GroupConfig extends VersionedMetaData { config, updatedMembers.orElse(originalMembers), updatedSubgroups.orElse(originalSubgroups), - createdOn)); + createdOn, + null)); groupCreation = Optional.empty(); return true; diff --git a/java/com/google/gerrit/server/group/db/GroupsUpdate.java b/java/com/google/gerrit/server/group/db/GroupsUpdate.java index 2e67ec48ee..f6f1242ae4 100644 --- a/java/com/google/gerrit/server/group/db/GroupsUpdate.java +++ b/java/com/google/gerrit/server/group/db/GroupsUpdate.java @@ -48,6 +48,7 @@ import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.git.RenameGroupOp; import com.google.gerrit.server.group.InternalGroup; +import com.google.gerrit.server.group.InternalGroup.Builder; import com.google.gerrit.server.update.RefUpdateUtil; import com.google.gwtorm.server.OrmDuplicateKeyException; import com.google.gwtorm.server.OrmException; @@ -62,6 +63,7 @@ import java.util.concurrent.TimeUnit; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevWalk; @@ -264,7 +266,10 @@ public class GroupsUpdate { AccountGroup group = createAccountGroup(groupCreation); UpdateResult updateResult = updateGroupInReviewDb(db, group, groupUpdate); return InternalGroup.create( - group, updateResult.getModifiedMembers(), updateResult.getModifiedSubgroups()); + group, + updateResult.getModifiedMembers(), + updateResult.getModifiedSubgroups(), + updateResult.getRefState()); } public static AccountGroup createAccountGroup( @@ -504,7 +509,8 @@ public class GroupsUpdate { .setGroupId(updatedGroup.getId()) .setGroupName(updatedGroup.getNameKey()) .setModifiedMembers(modifiedMembers) - .setModifiedSubgroups(modifiedSubgroups); + .setModifiedSubgroups(modifiedSubgroups) + .setRefState(updatedGroup.getRefState()); if (!Objects.equals(originalGroup.getNameKey(), updatedGroup.getNameKey())) { resultBuilder.setPreviousGroupName(originalGroup.getNameKey()); } @@ -637,6 +643,9 @@ public class GroupsUpdate { abstract ImmutableSet getModifiedSubgroups(); + @Nullable + public abstract ObjectId getRefState(); + static Builder builder() { return new AutoValue_GroupsUpdate_UpdateResult.Builder(); } @@ -655,6 +664,8 @@ public class GroupsUpdate { abstract Builder setModifiedSubgroups(Set modifiedSubgroups); + public abstract Builder setRefState(ObjectId refState); + abstract UpdateResult build(); } } diff --git a/java/com/google/gerrit/server/index/IndexUtils.java b/java/com/google/gerrit/server/index/IndexUtils.java index b37ed61c54..9730e3f07c 100644 --- a/java/com/google/gerrit/server/index/IndexUtils.java +++ b/java/com/google/gerrit/server/index/IndexUtils.java @@ -18,6 +18,7 @@ import static com.google.gerrit.server.index.change.ChangeField.CHANGE; import static com.google.gerrit.server.index.change.ChangeField.LEGACY_ID; import static com.google.gerrit.server.index.change.ChangeField.PROJECT; +import com.google.common.base.Function; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; @@ -30,12 +31,27 @@ import com.google.gerrit.server.index.project.ProjectField; import com.google.gerrit.server.query.change.SingleGroupUser; import java.io.IOException; import java.util.Set; +import java.util.concurrent.ExecutionException; import org.eclipse.jgit.errors.ConfigInvalidException; public final class IndexUtils { public static final ImmutableMap CUSTOM_CHAR_MAPPING = ImmutableMap.of("_", " ", ".", " "); + public static final Function MAPPER = + new Function() { + @Override + public IOException apply(Exception in) { + if (in instanceof IOException) { + return (IOException) in; + } else if (in instanceof ExecutionException && in.getCause() instanceof IOException) { + return (IOException) in.getCause(); + } else { + return new IOException(in); + } + } + }; + public static void setReady(SitePaths sitePaths, String name, int version, boolean ready) throws IOException { try { diff --git a/java/com/google/gerrit/server/index/change/ChangeIndexer.java b/java/com/google/gerrit/server/index/change/ChangeIndexer.java index f62b6623e0..d205e9197a 100644 --- a/java/com/google/gerrit/server/index/change/ChangeIndexer.java +++ b/java/com/google/gerrit/server/index/change/ChangeIndexer.java @@ -17,7 +17,6 @@ package com.google.gerrit.server.index.change; import static com.google.gerrit.server.extensions.events.EventUtil.logEventListenerError; import static com.google.gerrit.server.git.QueueProvider.QueueType.BATCH; -import com.google.common.base.Function; import com.google.common.util.concurrent.Atomics; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; @@ -31,6 +30,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.index.IndexExecutor; +import com.google.gerrit.server.index.IndexUtils; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.query.change.ChangeData; @@ -50,7 +50,6 @@ import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.concurrent.Callable; -import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicReference; import org.eclipse.jgit.lib.Config; @@ -79,23 +78,9 @@ public class ChangeIndexer { // ExecutionException, so we can reuse the same mapper as for a single // future. Assume the actual contents of the exception are not useful to // callers. All exceptions are already logged by IndexTask. - return Futures.makeChecked(Futures.allAsList(futures), MAPPER); + return Futures.makeChecked(Futures.allAsList(futures), IndexUtils.MAPPER); } - private static final Function MAPPER = - new Function() { - @Override - public IOException apply(Exception in) { - if (in instanceof IOException) { - return (IOException) in; - } else if (in instanceof ExecutionException && in.getCause() instanceof IOException) { - return (IOException) in.getCause(); - } else { - return new IOException(in); - } - } - }; - private final ChangeIndexCollection indexes; private final ChangeIndex index; private final SchemaFactory schemaFactory; @@ -335,7 +320,8 @@ public class ChangeIndexer { @SuppressWarnings("deprecation") private static com.google.common.util.concurrent.CheckedFuture submit( Callable task, ListeningExecutorService executor) { - return Futures.makeChecked(Futures.nonCancellationPropagating(executor.submit(task)), MAPPER); + return Futures.makeChecked( + Futures.nonCancellationPropagating(executor.submit(task)), IndexUtils.MAPPER); } private abstract class AbstractIndexTask implements Callable { diff --git a/java/com/google/gerrit/server/index/group/GroupField.java b/java/com/google/gerrit/server/index/group/GroupField.java index 078433aaf4..29e386731e 100644 --- a/java/com/google/gerrit/server/index/group/GroupField.java +++ b/java/com/google/gerrit/server/index/group/GroupField.java @@ -19,14 +19,18 @@ import static com.google.gerrit.index.FieldDef.exact; import static com.google.gerrit.index.FieldDef.fullText; import static com.google.gerrit.index.FieldDef.integer; import static com.google.gerrit.index.FieldDef.prefix; +import static com.google.gerrit.index.FieldDef.storedOnly; import static com.google.gerrit.index.FieldDef.timestamp; +import com.google.common.base.MoreObjects; import com.google.gerrit.index.FieldDef; import com.google.gerrit.index.SchemaUtil; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.server.group.InternalGroup; import java.sql.Timestamp; +import org.eclipse.jgit.lib.Constants; +import org.eclipse.jgit.lib.ObjectId; /** Secondary index schemas for groups. */ public class GroupField { @@ -72,4 +76,14 @@ public class GroupField { .buildRepeatable( g -> g.getSubgroups().stream().map(AccountGroup.UUID::get).collect(toImmutableList())); + + /** ObjectId of HEAD:refs/groups/. */ + public static final FieldDef REF_STATE = + storedOnly("ref_state") + .build( + g -> { + byte[] a = new byte[Constants.OBJECT_ID_STRING_LENGTH]; + MoreObjects.firstNonNull(g.getRefState(), ObjectId.zeroId()).copyTo(a, 0); + return a; + }); } diff --git a/java/com/google/gerrit/server/index/group/GroupIndexerImpl.java b/java/com/google/gerrit/server/index/group/GroupIndexerImpl.java index 69b29bcf29..cb69ea8265 100644 --- a/java/com/google/gerrit/server/index/group/GroupIndexerImpl.java +++ b/java/com/google/gerrit/server/index/group/GroupIndexerImpl.java @@ -14,20 +14,30 @@ package com.google.gerrit.server.index.group; +import static com.google.gerrit.server.git.QueueProvider.QueueType.BATCH; + import com.google.common.collect.ImmutableSet; +import com.google.common.util.concurrent.Futures; +import com.google.common.util.concurrent.ListeningExecutorService; import com.google.gerrit.common.Nullable; import com.google.gerrit.extensions.events.GroupIndexedListener; import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.index.Index; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.server.account.GroupCache; +import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.group.InternalGroup; +import com.google.gerrit.server.index.IndexExecutor; +import com.google.gerrit.server.index.IndexUtils; import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.AssistedInject; import java.io.IOException; import java.util.Collection; import java.util.Collections; import java.util.Optional; +import java.util.concurrent.Callable; +import java.util.concurrent.Future; +import org.eclipse.jgit.lib.Config; public class GroupIndexerImpl implements GroupIndexer { public interface Factory { @@ -39,16 +49,25 @@ public class GroupIndexerImpl implements GroupIndexer { private final GroupCache groupCache; private final DynamicSet indexedListener; private final GroupIndexCollection indexes; + private final StalenessChecker stalenessChecker; + private final boolean autoReindexIfStale; private final GroupIndex index; + private final ListeningExecutorService batchExecutor; @AssistedInject GroupIndexerImpl( GroupCache groupCache, DynamicSet indexedListener, + StalenessChecker stalenessChecker, + @GerritServerConfig Config config, + @IndexExecutor(BATCH) ListeningExecutorService batchExecutor, @Assisted GroupIndexCollection indexes) { this.groupCache = groupCache; this.indexedListener = indexedListener; this.indexes = indexes; + this.stalenessChecker = stalenessChecker; + this.autoReindexIfStale = autoReindexIfStale(config); + this.batchExecutor = batchExecutor; this.index = null; } @@ -56,10 +75,16 @@ public class GroupIndexerImpl implements GroupIndexer { GroupIndexerImpl( GroupCache groupCache, DynamicSet indexedListener, + StalenessChecker stalenessChecker, + @GerritServerConfig Config config, + @IndexExecutor(BATCH) ListeningExecutorService batchExecutor, @Assisted GroupIndex index) { this.groupCache = groupCache; this.indexedListener = indexedListener; this.indexes = null; + this.stalenessChecker = stalenessChecker; + this.autoReindexIfStale = autoReindexIfStale(config); + this.batchExecutor = batchExecutor; this.index = index; } @@ -74,6 +99,44 @@ public class GroupIndexerImpl implements GroupIndexer { } } fireGroupIndexedEvent(uuid.get()); + autoReindexIfStale(uuid); + } + + private static boolean autoReindexIfStale(Config cfg) { + return cfg.getBoolean("index", null, "autoReindexIfStale", true); + } + + private void autoReindexIfStale(AccountGroup.UUID uuid) { + if (autoReindexIfStale) { + // Don't retry indefinitely; if this fails the change will be stale. + @SuppressWarnings("unused") + Future possiblyIgnoredError = reindexIfStale(uuid); + } + } + + /** + * Asynchronously check if a group is stale, and reindex if it is. + * + *

Always run on the batch executor, even if this indexer instance is configured to use a + * different executor. + * + * @param uuid the unique identified of the group. + * @return future for reindexing the group; returns true if the change was stale. + */ + @SuppressWarnings("deprecation") + public com.google.common.util.concurrent.CheckedFuture reindexIfStale( + AccountGroup.UUID uuid) { + Callable task = + () -> { + if (stalenessChecker.isStale(uuid)) { + index(uuid); + return true; + } + return false; + }; + + return Futures.makeChecked( + Futures.nonCancellationPropagating(batchExecutor.submit(task)), IndexUtils.MAPPER); } private void fireGroupIndexedEvent(String uuid) { diff --git a/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java b/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java index b280b253e0..912524fe3c 100644 --- a/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java +++ b/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java @@ -34,8 +34,11 @@ public class GroupSchemaDefinitions extends SchemaDefinitions { @Deprecated static final Schema V3 = schema(V2, GroupField.CREATED_ON); + @Deprecated static final Schema V4 = schema(V3, GroupField.MEMBER, GroupField.SUBGROUP); + static final Schema V5 = schema(V4, GroupField.REF_STATE); + public static final GroupSchemaDefinitions INSTANCE = new GroupSchemaDefinitions(); private GroupSchemaDefinitions() { diff --git a/java/com/google/gerrit/server/index/group/IndexedGroupQuery.java b/java/com/google/gerrit/server/index/group/IndexedGroupQuery.java index 255df329c6..79f25c0861 100644 --- a/java/com/google/gerrit/server/index/group/IndexedGroupQuery.java +++ b/java/com/google/gerrit/server/index/group/IndexedGroupQuery.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.index.group; import com.google.gerrit.index.Index; +import com.google.gerrit.index.IndexConfig; import com.google.gerrit.index.IndexedQuery; import com.google.gerrit.index.QueryOptions; import com.google.gerrit.index.query.DataSource; @@ -22,10 +23,22 @@ import com.google.gerrit.index.query.Predicate; import com.google.gerrit.index.query.QueryParseException; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.server.group.InternalGroup; +import java.util.HashSet; +import java.util.Set; public class IndexedGroupQuery extends IndexedQuery implements DataSource { + public static QueryOptions createOptions( + IndexConfig config, int start, int limit, Set fields) { + // Always include GroupField.UUID since it is needed to load the group from NoteDb. + if (!fields.contains(GroupField.UUID.getName())) { + fields = new HashSet<>(fields); + fields.add(GroupField.UUID.getName()); + } + return QueryOptions.create(config, start, limit, fields); + } + public IndexedGroupQuery( Index index, Predicate pred, diff --git a/java/com/google/gerrit/server/index/group/StalenessChecker.java b/java/com/google/gerrit/server/index/group/StalenessChecker.java new file mode 100644 index 0000000000..dba89ad6da --- /dev/null +++ b/java/com/google/gerrit/server/index/group/StalenessChecker.java @@ -0,0 +1,94 @@ +// Copyright (C) 2017 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.index.group; + +import com.google.common.collect.ImmutableSet; +import com.google.gerrit.index.IndexConfig; +import com.google.gerrit.index.query.FieldBundle; +import com.google.gerrit.reviewdb.client.AccountGroup; +import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.config.GerritServerConfig; +import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; +import com.google.inject.Singleton; +import java.io.IOException; +import java.util.Optional; +import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.Repository; + +/** + * Checks if documents in the group index are stale. + * + *

An index document is considered stale if the stored SHA1 differs from the HEAD SHA1 of the + * groups branch. + * + *

Note: This only applies to NoteDb. + */ +@Singleton +public class StalenessChecker { + public static final ImmutableSet FIELDS = + ImmutableSet.of(GroupField.UUID.getName(), GroupField.REF_STATE.getName()); + + private final GroupIndexCollection indexes; + private final GitRepositoryManager repoManager; + private final IndexConfig indexConfig; + private final AllUsersName allUsers; + private final Config config; + + @Inject + StalenessChecker( + GroupIndexCollection indexes, + GitRepositoryManager repoManager, + IndexConfig indexConfig, + AllUsersName allUsers, + @GerritServerConfig Config config) { + this.indexes = indexes; + this.repoManager = repoManager; + this.indexConfig = indexConfig; + this.allUsers = allUsers; + this.config = config; + } + + public boolean isStale(AccountGroup.UUID id) throws IOException, OrmException { + if (!config.getBoolean("user", "readGroupsFromNoteDb", false)) { + return false; // This class only treats staleness for groups in NoteDb. + } + + GroupIndex i = indexes.getSearchIndex(); + 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 result = + i.getRaw(id, IndexedGroupQuery.createOptions(indexConfig, 0, 1, FIELDS)); + if (!result.isPresent()) { + // No document in the index + return true; + } + + try (Repository repo = repoManager.openRepository(allUsers)) { + Ref ref = repo.exactRef(RefNames.refsGroups(id)); + ObjectId head = ref == null ? ObjectId.zeroId() : ref.getObjectId(); + return !head.equals(ObjectId.fromString(result.get().getValue(GroupField.REF_STATE), 0)); + } + } +} diff --git a/java/com/google/gerrit/server/query/group/GroupPredicates.java b/java/com/google/gerrit/server/query/group/GroupPredicates.java index d02f6a46fa..210651e9c8 100644 --- a/java/com/google/gerrit/server/query/group/GroupPredicates.java +++ b/java/com/google/gerrit/server/query/group/GroupPredicates.java @@ -46,6 +46,7 @@ public class GroupPredicates { return new GroupPredicate(GroupField.NAME, GroupQueryBuilder.FIELD_NAME, name); } + @SuppressWarnings("deprecation") public static Predicate owner(AccountGroup.UUID ownerUuid) { return new GroupPredicate( GroupField.OWNER_UUID, GroupQueryBuilder.FIELD_OWNER, ownerUuid.get()); diff --git a/java/com/google/gerrit/server/query/group/GroupQueryBuilder.java b/java/com/google/gerrit/server/query/group/GroupQueryBuilder.java index 057cc440ea..c7fdfba76c 100644 --- a/java/com/google/gerrit/server/query/group/GroupQueryBuilder.java +++ b/java/com/google/gerrit/server/query/group/GroupQueryBuilder.java @@ -49,7 +49,7 @@ public class GroupQueryBuilder extends QueryBuilder { public static final String FIELD_DESCRIPTION = "description"; public static final String FIELD_INNAME = "inname"; public static final String FIELD_NAME = "name"; - public static final String FIELD_OWNER = "owner"; + @Deprecated public static final String FIELD_OWNER = "owner"; public static final String FIELD_LIMIT = "limit"; private static final QueryBuilder.Definition mydef = @@ -109,12 +109,6 @@ public class GroupQueryBuilder extends QueryBuilder { return GroupPredicates.name(name); } - @Operator - public Predicate owner(String owner) throws QueryParseException { - AccountGroup.UUID groupUuid = parseGroup(owner); - return GroupPredicates.owner(groupUuid); - } - @Operator public Predicate is(String value) throws QueryParseException { if ("visibletoall".equalsIgnoreCase(value)) { @@ -133,11 +127,6 @@ public class GroupQueryBuilder extends QueryBuilder { if (!Strings.isNullOrEmpty(query)) { preds.add(description(query)); } - try { - preds.add(owner(query)); - } catch (QueryParseException e) { - // Skip. - } return Predicate.or(preds); } diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupRebuilderIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupRebuilderIT.java index 3437cf4eb4..dbbd07ea81 100644 --- a/javatests/com/google/gerrit/acceptance/api/group/GroupRebuilderIT.java +++ b/javatests/com/google/gerrit/acceptance/api/group/GroupRebuilderIT.java @@ -90,7 +90,7 @@ public class GroupRebuilderIT extends AbstractDaemonTest { InternalGroup reviewDbGroup = groups.getGroup(db, new AccountGroup.UUID(createdGroup.id)).get(); deleteGroupRefs(reviewDbGroup); - assertThat(rebuild(reviewDbGroup)).isEqualTo(roundToSecond(reviewDbGroup)); + assertThat(removeRefState(rebuild(reviewDbGroup))).isEqualTo(roundToSecond(reviewDbGroup)); } @Test @@ -109,7 +109,7 @@ public class GroupRebuilderIT extends AbstractDaemonTest { deleteGroupRefs(reviewDbGroup); InternalGroup noteDbGroup = rebuild(reviewDbGroup); - assertThat(noteDbGroup).isEqualTo(roundToSecond(reviewDbGroup)); + assertThat(removeRefState(noteDbGroup)).isEqualTo(roundToSecond(reviewDbGroup)); ImmutableList log = log(group1); assertThat(log).hasSize(4); @@ -147,6 +147,10 @@ public class GroupRebuilderIT extends AbstractDaemonTest { assertThat(log.get(3)).committer().hasSameDateAs(log.get(3).author); } + private static InternalGroup removeRefState(InternalGroup group) throws Exception { + return group.toBuilder().setRefState(null).build(); + } + private void deleteGroupRefs(InternalGroup group) throws Exception { try (Repository repo = repoManager.openRepository(allUsers)) { String refName = RefNames.refsGroups(group.getGroupUUID()); diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java index 631f96ef2a..a9b4e880b3 100644 --- a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java +++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java @@ -24,6 +24,7 @@ import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; import static java.util.stream.Collectors.toList; +import com.google.common.cache.LoadingCache; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.AbstractDaemonTest; @@ -66,9 +67,11 @@ import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.group.SystemGroupBackend; import com.google.gerrit.server.group.db.Groups; +import com.google.gerrit.server.index.group.StalenessChecker; import com.google.gerrit.server.util.MagicBranch; import com.google.gerrit.testing.ConfigSuite; import com.google.inject.Inject; +import com.google.inject.name.Named; import java.io.IOException; import java.sql.Timestamp; import java.time.Instant; @@ -79,6 +82,7 @@ import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.Optional; import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.CommitBuilder; @@ -89,6 +93,7 @@ import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.transport.PushResult; import org.eclipse.jgit.transport.RemoteRefUpdate; @@ -107,6 +112,11 @@ public class GroupsIT extends AbstractDaemonTest { @Inject private Groups groups; @Inject private GroupIncludeCache groupIncludeCache; @Inject private GroupBackend groupBackend; + @Inject private StalenessChecker stalenessChecker; + + @Inject + @Named("groups_byuuid") + LoadingCache> groupsByUUIDCache; @Test public void systemGroupCanBeRetrievedFromIndex() throws Exception { @@ -974,6 +984,47 @@ public class GroupsIT extends AbstractDaemonTest { assertThat(rule.getMax()).isEqualTo(0); } + @Test + public void stalenessChecker() throws Exception { + assume().that(groupsInNoteDb()).isTrue(); + + // Newly created group is not stale + GroupInfo groupInfo = gApi.groups().create(name("foo")).get(); + AccountGroup.UUID groupUuid = new AccountGroup.UUID(groupInfo.id); + assertThat(stalenessChecker.isStale(groupUuid)).isFalse(); + + // Manual update makes index document stale + String groupRef = RefNames.refsGroups(groupUuid); + try (Repository repo = repoManager.openRepository(allUsers); + ObjectInserter oi = repo.newObjectInserter(); + RevWalk rw = new RevWalk(repo)) { + RevCommit commit = rw.parseCommit(repo.exactRef(groupRef).getObjectId()); + + PersonIdent ident = new PersonIdent(serverIdent.get(), TimeUtil.nowTs()); + CommitBuilder cb = new CommitBuilder(); + cb.setTreeId(commit.getTree()); + cb.setCommitter(ident); + cb.setAuthor(ident); + cb.setMessage(commit.getFullMessage()); + ObjectId emptyCommit = oi.insert(cb); + oi.flush(); + + RefUpdate updateRef = repo.updateRef(groupRef); + updateRef.setExpectedOldObjectId(commit.toObjectId()); + updateRef.setNewObjectId(emptyCommit); + assertThat(updateRef.forceUpdate()).isEqualTo(RefUpdate.Result.FORCED); + } + // Evict group from cache to be sure that we use the index state for staleness checks. This has + // to happen directly on the groupsByUUID cache because GroupsCacheImpl triggers a reindex for + // the group. + groupsByUUIDCache.invalidate(groupUuid); + assertThat(stalenessChecker.isStale(groupUuid)).isTrue(); + + // Reindex fixes staleness + gApi.groups().id(groupInfo.id).index(); + assertThat(stalenessChecker.isStale(groupUuid)).isFalse(); + } + private void pushToGroupBranchForReviewAndSubmit(Project.NameKey project, String expectedError) throws Exception { grantLabel( diff --git a/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java b/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java index 1d26b2fd70..863d195278 100644 --- a/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java +++ b/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java @@ -381,7 +381,7 @@ public class GroupRebuilderTest extends GerritBaseTests { } private InternalGroup reload(AccountGroup g) throws Exception { - return GroupConfig.loadForGroup(repo, g.getGroupUUID()).getLoadedGroup().get(); + return removeRefState(GroupConfig.loadForGroup(repo, g.getGroupUUID()).getLoadedGroup().get()); } private AccountGroup newGroup(String name) { @@ -473,4 +473,8 @@ public class GroupRebuilderTest extends GerritBaseTests { assertThat(commitInfo).committer().date().isEqualTo(commitInfo.author.date); assertThat(commitInfo).committer().tz().isEqualTo(commitInfo.author.tz); } + + private static InternalGroup removeRefState(InternalGroup group) throws Exception { + return group.toBuilder().setRefState(null).build(); + } } diff --git a/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java b/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java index 54c22f932c..68cdaae388 100644 --- a/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java +++ b/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java @@ -245,19 +245,6 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests { assertQuery("description:\"\""); } - @Test - public void byOwner() throws Exception { - GroupInfo ownerGroup = createGroup(name("owner-group")); - GroupInfo group = createGroupWithOwner(name("group"), ownerGroup); - createGroup(name("group2")); - - assertQuery("owner:" + group.id); - - // ownerGroup owns itself - assertQuery("owner:" + ownerGroup.id, group, ownerGroup); - assertQuery("owner:" + ownerGroup.name, group, ownerGroup); - } - @Test public void byIsVisibleToAll() throws Exception { assertQuery("is:visibletoall");