Merge "Add staleness checker for groups in NoteDb"

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<Account.Id> members,
       ImmutableSet<AccountGroup.UUID> subgroups) {
+    return create(accountGroup, members, subgroups, null);
+  }
+
+  public static InternalGroup create(
+      AccountGroup accountGroup,
+      ImmutableSet<Account.Id> members,
+      ImmutableSet<AccountGroup.UUID> 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<AccountGroup.UUID> 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<AccountGroup.UUID> subgroups);
 
+    public abstract Builder setRefState(ObjectId refState);
+
     public abstract InternalGroup build();
   }
 }

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<Account.Id> members = readMembers();
       ImmutableSet<AccountGroup.UUID> 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<Account.Id> members,
       ImmutableSet<AccountGroup.UUID> 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;

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<AccountGroup.UUID> 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<AccountGroup.UUID> modifiedSubgroups);
 
+      public abstract Builder setRefState(ObjectId refState);
+
       abstract UpdateResult build();
     }
   }

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<String, String> CUSTOM_CHAR_MAPPING =
       ImmutableMap.of("_", " ", ".", " ");
 
+  public static final Function<Exception, IOException> MAPPER =
+      new Function<Exception, IOException>() {
+        @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 {

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<Exception, IOException> MAPPER =
-      new Function<Exception, IOException>() {
-        @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<ReviewDb> schemaFactory;
@@ -335,7 +320,8 @@ public class ChangeIndexer {
   @SuppressWarnings("deprecation")
   private static <T> com.google.common.util.concurrent.CheckedFuture<T, IOException> submit(
       Callable<T> 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<T> implements Callable<T> {

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/<UUID>. */
+  public static final FieldDef<InternalGroup, byte[]> 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;
+              });
 }

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<GroupIndexedListener> 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<GroupIndexedListener> 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<GroupIndexedListener> 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.
+   *
+   * <p>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<Boolean, IOException> reindexIfStale(
+      AccountGroup.UUID uuid) {
+    Callable<Boolean> 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) {

java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java

@@ -34,8 +34,11 @@ public class GroupSchemaDefinitions extends SchemaDefinitions<InternalGroup> {
 
   @Deprecated static final Schema<InternalGroup> V3 = schema(V2, GroupField.CREATED_ON);
 
+  @Deprecated
   static final Schema<InternalGroup> V4 = schema(V3, GroupField.MEMBER, GroupField.SUBGROUP);
 
+  static final Schema<InternalGroup> V5 = schema(V4, GroupField.REF_STATE);
+
   public static final GroupSchemaDefinitions INSTANCE = new GroupSchemaDefinitions();
 
   private GroupSchemaDefinitions() {

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<AccountGroup.UUID, InternalGroup>
     implements DataSource<InternalGroup> {
 
+  public static QueryOptions createOptions(
+      IndexConfig config, int start, int limit, Set<String> 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<AccountGroup.UUID, InternalGroup> index,
       Predicate<InternalGroup> pred,

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.
+ *
+ * <p>An index document is considered stale if the stored SHA1 differs from the HEAD SHA1 of the
+ * groups branch.
+ *
+ * <p>Note: This only applies to NoteDb.
+ */
+@Singleton
+public class StalenessChecker {
+  public static final ImmutableSet<String> 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<FieldBundle> 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));
+    }
+  }
+}

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<InternalGroup> owner(AccountGroup.UUID ownerUuid) {
     return new GroupPredicate(
         GroupField.OWNER_UUID, GroupQueryBuilder.FIELD_OWNER, ownerUuid.get());

java/com/google/gerrit/server/query/group/GroupQueryBuilder.java

@@ -49,7 +49,7 @@ public class GroupQueryBuilder extends QueryBuilder<InternalGroup> {
   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<InternalGroup, GroupQueryBuilder> mydef =
@@ -109,12 +109,6 @@ public class GroupQueryBuilder extends QueryBuilder<InternalGroup> {
     return GroupPredicates.name(name);
   }
 
-  @Operator
-  public Predicate<InternalGroup> owner(String owner) throws QueryParseException {
-    AccountGroup.UUID groupUuid = parseGroup(owner);
-    return GroupPredicates.owner(groupUuid);
-  }
-
   @Operator
   public Predicate<InternalGroup> is(String value) throws QueryParseException {
     if ("visibletoall".equalsIgnoreCase(value)) {
@@ -133,11 +127,6 @@ public class GroupQueryBuilder extends QueryBuilder<InternalGroup> {
     if (!Strings.isNullOrEmpty(query)) {
       preds.add(description(query));
     }
-    try {
-      preds.add(owner(query));
-    } catch (QueryParseException e) {
-      // Skip.
-    }
     return Predicate.or(preds);
   }
 

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<CommitInfo> 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());

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<String, Optional<InternalGroup>> 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(

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();
+  }
 }

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");