Merge changes Icff51096,Iba2284a7
* changes:
Migrate stars index queries to read from All-Users repository
Migrate has:draft to read changes from All-Users repository
diff --git a/java/com/google/gerrit/entities/RefNames.java b/java/com/google/gerrit/entities/RefNames.java
index 2263aba..349b67e 100644
--- a/java/com/google/gerrit/entities/RefNames.java
+++ b/java/com/google/gerrit/entities/RefNames.java
@@ -489,7 +489,7 @@
return Integer.parseInt(rest.substring(0, ie));
}
- static Integer parseRefSuffix(String name) {
+ public static Integer parseRefSuffix(String name) {
if (name == null) {
return null;
}
diff --git a/java/com/google/gerrit/server/CommentsUtil.java b/java/com/google/gerrit/server/CommentsUtil.java
index ba9f6d6..1d38877 100644
--- a/java/com/google/gerrit/server/CommentsUtil.java
+++ b/java/com/google/gerrit/server/CommentsUtil.java
@@ -52,9 +52,11 @@
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
+import java.util.Set;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
@@ -465,10 +467,35 @@
}
}
+ /** returns all changes that contain draft comments of {@code accountId}. */
+ public Collection<Change.Id> getChangesWithDrafts(Account.Id accountId) {
+ try (Repository repo = repoManager.openRepository(allUsers)) {
+ return getChangesWithDrafts(repo, accountId);
+ } catch (IOException e) {
+ throw new StorageException(e);
+ }
+ }
+
private Collection<Ref> getDraftRefs(Repository repo, Change.Id changeId) throws IOException {
return repo.getRefDatabase().getRefsByPrefix(RefNames.refsDraftCommentsPrefix(changeId));
}
+ private Collection<Change.Id> getChangesWithDrafts(Repository repo, Account.Id accountId)
+ throws IOException {
+ Set<Change.Id> changes = new HashSet<>();
+ for (Ref ref : repo.getRefDatabase().getRefsByPrefix(RefNames.REFS_DRAFT_COMMENTS)) {
+ Integer accountIdFromRef = RefNames.parseRefSuffix(ref.getName());
+ if (accountIdFromRef != null && accountIdFromRef == accountId.get()) {
+ Change.Id changeId = Change.Id.fromAllUsersRef(ref.getName());
+ if (changeId == null) {
+ continue;
+ }
+ changes.add(changeId);
+ }
+ }
+ return changes;
+ }
+
private static <T extends Comment> List<T> sort(List<T> comments) {
comments.sort(COMMENT_ORDER);
return comments;
diff --git a/java/com/google/gerrit/server/StarredChangesUtil.java b/java/com/google/gerrit/server/StarredChangesUtil.java
index 5dcbd01..7c61c92 100644
--- a/java/com/google/gerrit/server/StarredChangesUtil.java
+++ b/java/com/google/gerrit/server/StarredChangesUtil.java
@@ -289,6 +289,35 @@
}
}
+ public ImmutableSet<Change.Id> byAccountId(Account.Id accountId, String label) {
+ try (Repository repo = repoManager.openRepository(allUsers)) {
+ ImmutableSet.Builder<Change.Id> builder = ImmutableSet.builder();
+ for (Ref ref : repo.getRefDatabase().getRefsByPrefix(RefNames.REFS_STARRED_CHANGES)) {
+ Account.Id currentAccountId = Account.Id.fromRef(ref.getName());
+ // Skip all refs that don't correspond with accountId.
+ if (currentAccountId == null || !currentAccountId.equals(accountId)) {
+ continue;
+ }
+ // Skip all refs that don't contain the required label.
+ StarRef starRef = readLabels(repo, ref.getName());
+ if (!starRef.labels().contains(label)) {
+ continue;
+ }
+
+ // Skip invalid change ids.
+ Change.Id changeId = Change.Id.fromAllUsersRef(ref.getName());
+ if (changeId == null) {
+ continue;
+ }
+ builder.add(changeId);
+ }
+ return builder.build();
+ } catch (IOException e) {
+ throw new StorageException(
+ String.format("Get starred changes for account %d failed", accountId.get()), e);
+ }
+ }
+
public ImmutableListMultimap<Account.Id, String> byChangeFromIndex(Change.Id changeId) {
List<ChangeData> changeData =
queryProvider
diff --git a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
index 1486559..65f8f2d 100644
--- a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
+++ b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
@@ -45,6 +45,13 @@
GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS_BACKFILLING_ON_DASHBOARD =
"GerritBackendRequestFeature__enable_submit_requirements_backfilling_on_dashboard";
+ /**
+ * When set, we compute information from All-Users repository if able, instead of computing it
+ * from the change index.
+ */
+ public static final String GERRIT_BACKEND_REQUEST_FEATURE_COMPUTE_FROM_ALL_USERS_REPOSITORY =
+ "GerritBackendRequestFeature__compute_from_all_users_repository";
+
/** Features, enabled by default in the current release. */
public static final ImmutableSet<String> DEFAULT_ENABLED_FEATURES =
ImmutableSet.of(UI_FEATURE_PATCHSET_COMMENTS);
diff --git a/java/com/google/gerrit/server/query/change/ChangePredicates.java b/java/com/google/gerrit/server/query/change/ChangePredicates.java
index 043b37d..3afdcdd 100644
--- a/java/com/google/gerrit/server/query/change/ChangePredicates.java
+++ b/java/com/google/gerrit/server/query/change/ChangePredicates.java
@@ -14,6 +14,8 @@
package com.google.gerrit.server.query.change;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+
import com.google.common.base.CharMatcher;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
@@ -21,12 +23,15 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.git.ObjectIds;
import com.google.gerrit.index.query.Predicate;
+import com.google.gerrit.server.CommentsUtil;
+import com.google.gerrit.server.StarredChangesUtil;
import com.google.gerrit.server.change.HashtagsUtil;
import com.google.gerrit.server.index.change.ChangeField;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Locale;
+import java.util.Set;
/** Predicates that match against {@link ChangeData}. */
public class ChangePredicates {
@@ -76,8 +81,34 @@
* Returns a predicate that matches changes where the provided {@link
* com.google.gerrit.entities.Account.Id} has a pending draft comment.
*/
- public static Predicate<ChangeData> draftBy(Account.Id id) {
- return new ChangeIndexPredicate(ChangeField.DRAFTBY, id.toString());
+ public static Predicate<ChangeData> draftBy(
+ boolean computeFromAllUsersRepository, CommentsUtil commentsUtil, Account.Id id) {
+ if (!computeFromAllUsersRepository) {
+ return new ChangeIndexPredicate(ChangeField.DRAFTBY, id.toString());
+ }
+ Set<Predicate<ChangeData>> changeIdPredicates =
+ commentsUtil.getChangesWithDrafts(id).stream()
+ .map(ChangePredicates::idStr)
+ .collect(toImmutableSet());
+ return Predicate.or(changeIdPredicates);
+ }
+
+ /**
+ * Returns a predicate that matches changes where the provided {@link
+ * com.google.gerrit.entities.Account.Id} has starred changes with {@code label}.
+ */
+ public static Predicate<ChangeData> starBy(
+ boolean computeFromAllUsersRepository,
+ StarredChangesUtil starredChangesUtil,
+ Account.Id id,
+ String label) {
+ if (!computeFromAllUsersRepository) {
+ return new StarPredicate(id, label);
+ }
+ return Predicate.or(
+ starredChangesUtil.byAccountId(id, label).stream()
+ .map(ChangePredicates::idStr)
+ .collect(toImmutableSet()));
}
/**
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index da36633..57191c5 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -17,6 +17,7 @@
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.entities.Change.CHANGE_ID_PATTERN;
import static com.google.gerrit.server.account.AccountResolver.isSelf;
+import static com.google.gerrit.server.experiments.ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_COMPUTE_FROM_ALL_USERS_REPOSITORY;
import static com.google.gerrit.server.query.change.ChangeData.asChanges;
import static java.util.stream.Collectors.toList;
import static java.util.stream.Collectors.toSet;
@@ -70,6 +71,7 @@
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.HasOperandAliasConfig;
import com.google.gerrit.server.config.OperatorAliasConfig;
+import com.google.gerrit.server.experiments.ExperimentFeatures;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.index.change.ChangeIndex;
@@ -253,6 +255,7 @@
final OperatorAliasConfig operatorAliasConfig;
final boolean indexMergeable;
final boolean conflictsPredicateEnabled;
+ final ExperimentFeatures experimentFeatures;
final HasOperandAliasConfig hasOperandAliasConfig;
final PluginSetContext<SubmitRule> submitRules;
@@ -288,6 +291,7 @@
GroupMembers groupMembers,
OperatorAliasConfig operatorAliasConfig,
@GerritServerConfig Config gerritConfig,
+ ExperimentFeatures experimentFeatures,
HasOperandAliasConfig hasOperandAliasConfig,
ChangeIsVisibleToPredicate.Factory changeIsVisbleToPredicateFactory,
PluginSetContext<SubmitRule> submitRules) {
@@ -320,6 +324,7 @@
operatorAliasConfig,
MergeabilityComputationBehavior.fromConfig(gerritConfig).includeInIndex(),
gerritConfig.getBoolean("change", null, "conflictsPredicateEnabled", true),
+ experimentFeatures,
hasOperandAliasConfig,
changeIsVisbleToPredicateFactory,
submitRules);
@@ -354,6 +359,7 @@
OperatorAliasConfig operatorAliasConfig,
boolean indexMergeable,
boolean conflictsPredicateEnabled,
+ ExperimentFeatures experimentFeatures,
HasOperandAliasConfig hasOperandAliasConfig,
ChangeIsVisibleToPredicate.Factory changeIsVisbleToPredicateFactory,
PluginSetContext<SubmitRule> submitRules) {
@@ -386,6 +392,7 @@
this.operatorAliasConfig = operatorAliasConfig;
this.indexMergeable = indexMergeable;
this.conflictsPredicateEnabled = conflictsPredicateEnabled;
+ this.experimentFeatures = experimentFeatures;
this.hasOperandAliasConfig = hasOperandAliasConfig;
this.submitRules = submitRules;
}
@@ -420,6 +427,7 @@
operatorAliasConfig,
indexMergeable,
conflictsPredicateEnabled,
+ experimentFeatures,
hasOperandAliasConfig,
changeIsVisbleToPredicateFactory,
submitRules);
@@ -1089,15 +1097,29 @@
}
private Predicate<ChangeData> ignoredBySelf() throws QueryParseException {
- return new StarPredicate(self(), StarredChangesUtil.IGNORE_LABEL);
+ return ChangePredicates.starBy(
+ args.experimentFeatures.isFeatureEnabled(
+ GERRIT_BACKEND_REQUEST_FEATURE_COMPUTE_FROM_ALL_USERS_REPOSITORY),
+ args.starredChangesUtil,
+ self(),
+ StarredChangesUtil.IGNORE_LABEL);
}
private Predicate<ChangeData> starredBySelf() throws QueryParseException {
- return new StarPredicate(self(), StarredChangesUtil.DEFAULT_LABEL);
+ return ChangePredicates.starBy(
+ args.experimentFeatures.isFeatureEnabled(
+ GERRIT_BACKEND_REQUEST_FEATURE_COMPUTE_FROM_ALL_USERS_REPOSITORY),
+ args.starredChangesUtil,
+ self(),
+ StarredChangesUtil.DEFAULT_LABEL);
}
private Predicate<ChangeData> draftBySelf() throws QueryParseException {
- return ChangePredicates.draftBy(self());
+ return ChangePredicates.draftBy(
+ args.experimentFeatures.isFeatureEnabled(
+ GERRIT_BACKEND_REQUEST_FEATURE_COMPUTE_FROM_ALL_USERS_REPOSITORY),
+ args.commentsUtil,
+ self());
}
@Operator
diff --git a/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java b/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java
index 1485a6e56..ad3c56b 100644
--- a/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java
+++ b/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.restapi.account;
import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.gerrit.server.experiments.ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_COMPUTE_FROM_ALL_USERS_REPOSITORY;
import com.google.common.base.CharMatcher;
import com.google.common.base.Strings;
@@ -39,6 +40,7 @@
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.account.AccountResource;
import com.google.gerrit.server.change.ChangeJson;
+import com.google.gerrit.server.experiments.ExperimentFeatures;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangePredicates;
@@ -75,6 +77,7 @@
private final Provider<CommentJson> commentJsonProvider;
private final CommentsUtil commentsUtil;
private final PatchSetUtil psUtil;
+ private final ExperimentFeatures experimentFeatures;
@Inject
DeleteDraftComments(
@@ -86,7 +89,8 @@
ChangeJson.Factory changeJsonFactory,
Provider<CommentJson> commentJsonProvider,
CommentsUtil commentsUtil,
- PatchSetUtil psUtil) {
+ PatchSetUtil psUtil,
+ ExperimentFeatures experimentFeatures) {
this.userProvider = userProvider;
this.batchUpdateFactory = batchUpdateFactory;
this.queryBuilderProvider = queryBuilderProvider;
@@ -96,6 +100,7 @@
this.commentJsonProvider = commentJsonProvider;
this.commentsUtil = commentsUtil;
this.psUtil = psUtil;
+ this.experimentFeatures = experimentFeatures;
}
@Override
@@ -146,7 +151,12 @@
private Predicate<ChangeData> predicate(Account.Id accountId, DeleteDraftCommentsInput input)
throws BadRequestException {
- Predicate<ChangeData> hasDraft = ChangePredicates.draftBy(accountId);
+ Predicate<ChangeData> hasDraft =
+ ChangePredicates.draftBy(
+ experimentFeatures.isFeatureEnabled(
+ GERRIT_BACKEND_REQUEST_FEATURE_COMPUTE_FROM_ALL_USERS_REPOSITORY),
+ commentsUtil,
+ accountId);
if (CharMatcher.whitespace().trimFrom(Strings.nullToEmpty(input.query)).isEmpty()) {
return hasDraft;
}
diff --git a/javatests/com/google/gerrit/server/index/change/FakeQueryBuilder.java b/javatests/com/google/gerrit/server/index/change/FakeQueryBuilder.java
index 66a98e8..67b0342 100644
--- a/javatests/com/google/gerrit/server/index/change/FakeQueryBuilder.java
+++ b/javatests/com/google/gerrit/server/index/change/FakeQueryBuilder.java
@@ -56,6 +56,7 @@
new Config(),
null,
null,
+ null,
null));
}
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 332548a..3b671aa 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -111,6 +111,7 @@
import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.change.PatchSetInserter;
import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.experiments.ExperimentFeaturesConstants;
import com.google.gerrit.server.git.meta.MetaDataUpdate;
import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.index.change.ChangeIndexCollection;
@@ -2358,7 +2359,21 @@
}
@Test
- public void byHasDraft() throws Exception {
+ public void byHasDraft_draftsComputedFromIndex() throws Exception {
+ byHasDraft();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value =
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_COMPUTE_FROM_ALL_USERS_REPOSITORY)
+ public void byHasDraft_draftsComputedFromAllUsersRepository() throws Exception {
+ byHasDraft();
+ }
+
+ private void byHasDraft() throws Exception {
TestRepository<Repo> repo = createProject("repo");
Change change1 = insert(repo, newChange(repo));
Change change2 = insert(repo, newChange(repo));
@@ -2386,7 +2401,11 @@
assertQuery("has:draft");
}
- @Test
+ /**
+ * This test does not have a test about drafts computed from All-Users Repository because zombie
+ * drafts can't be filtered when computing from All-Users repository. TODO(paiking): During
+ * rollout, we should find a way to fix zombie drafts.
+ */
public void byHasDraftExcludesZombieDrafts() throws Exception {
Project.NameKey project = Project.nameKey("repo");
TestRepository<Repo> repo = createProject(project.get());
@@ -2424,8 +2443,62 @@
assertQuery("has:draft");
}
+ public void byHasDraftWithManyDrafts_draftsComputedFromIndex() throws Exception {
+ byHasDraftWithManyDrafts();
+ }
+
+ @GerritConfig(
+ name = "experiments.enabled",
+ value =
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_COMPUTE_FROM_ALL_USERS_REPOSITORY)
+ public void byHasDraftWithManyDrafts_draftsComputedFromAllUsersRepository() throws Exception {
+ byHasDraftWithManyDrafts();
+ }
+
+ private void byHasDraftWithManyDrafts() throws Exception {
+ TestRepository<Repo> repo = createProject("repo");
+ Change[] changesWithDrafts = new Change[30];
+
+ // unrelated change not shown in the result.
+ insert(repo, newChange(repo));
+
+ for (int i = 0; i < changesWithDrafts.length; i++) {
+ // put the changes in reverse order since this is the order we receive them from the index.
+ changesWithDrafts[changesWithDrafts.length - 1 - i] = insert(repo, newChange(repo));
+ DraftInput in = new DraftInput();
+ in.line = 1;
+ in.message = "nit: trailing whitespace";
+ in.path = Patch.COMMIT_MSG;
+ gApi.changes()
+ .id(changesWithDrafts[changesWithDrafts.length - 1 - i].getId().get())
+ .current()
+ .createDraft(in);
+ }
+ assertQuery("has:draft", changesWithDrafts);
+
+ Account.Id user2 =
+ accountManager.authenticate(authRequestFactory.createForUser("anotheruser")).getAccountId();
+ requestContext.setContext(newRequestContext(user2));
+ assertQuery("has:draft");
+ }
+
@Test
- public void byStarredBy() throws Exception {
+ public void byStarredBy_starsComputedFromIndex() throws Exception {
+ byStarredBy();
+ }
+
+ @GerritConfig(
+ name = "experiments.enabled",
+ value =
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_COMPUTE_FROM_ALL_USERS_REPOSITORY)
+ @Test
+ public void byStarredBy_starsComputedFromAllUsersRepository() throws Exception {
+ byStarredBy();
+ }
+
+ private void byStarredBy() throws Exception {
TestRepository<Repo> repo = createProject("repo");
Change change1 = insert(repo, newChange(repo));
Change change2 = insert(repo, newChange(repo));
@@ -2446,7 +2519,21 @@
}
@Test
- public void byStar() throws Exception {
+ public void byStar_starsComputedFromIndex() throws Exception {
+ byStar();
+ }
+
+ @GerritConfig(
+ name = "experiments.enabled",
+ value =
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_COMPUTE_FROM_ALL_USERS_REPOSITORY)
+ @Test
+ public void byStar_starsComputedFromAllUsersRepository() throws Exception {
+ byStar();
+ }
+
+ private void byStar() throws Exception {
TestRepository<Repo> repo = createProject("repo");
Change change1 = insert(repo, newChangeWithStatus(repo, Change.Status.MERGED));
Change change2 = insert(repo, newChangeWithStatus(repo, Change.Status.MERGED));
@@ -2472,7 +2559,21 @@
}
@Test
- public void byIgnore() throws Exception {
+ public void byIgnore_starsComputedFromIndex() throws Exception {
+ byIgnore();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value =
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_COMPUTE_FROM_ALL_USERS_REPOSITORY)
+ public void byIgnore_starsComputedFromAllUsersRepository() throws Exception {
+ byIgnore();
+ }
+
+ private void byIgnore() throws Exception {
TestRepository<Repo> repo = createProject("repo");
Account.Id user2 =
accountManager.authenticate(authRequestFactory.createForUser("anotheruser")).getAccountId();
@@ -2492,6 +2593,42 @@
assertQuery("-star:ignore", change2, change1);
}
+ public void byStarWithManyStars_starsComputedFromIndex() throws Exception {
+ byStarWithManyStars();
+ }
+
+ @GerritConfig(
+ name = "experiments.enabled",
+ value =
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_COMPUTE_FROM_ALL_USERS_REPOSITORY)
+ public void byStarWithManyStars_starsComputedFromAllUsersRepository() throws Exception {
+ byStarWithManyStars();
+ }
+
+ private void byStarWithManyStars() throws Exception {
+ TestRepository<Repo> repo = createProject("repo");
+ Change[] changesWithDrafts = new Change[30];
+ for (int i = 0; i < changesWithDrafts.length; i++) {
+ // put the changes in reverse order since this is the order we receive them from the index.
+ changesWithDrafts[changesWithDrafts.length - 1 - i] = insert(repo, newChange(repo));
+
+ // star the change
+ gApi.accounts()
+ .self()
+ .starChange(changesWithDrafts[changesWithDrafts.length - 1 - i].getId().toString());
+
+ // ignore the change
+ gApi.changes()
+ .id(changesWithDrafts[changesWithDrafts.length - 1 - i].getId().toString())
+ .ignore(true);
+ }
+
+ // all changes are both starred and ignored.
+ assertQuery("is:ignored", changesWithDrafts);
+ assertQuery("is:starred", changesWithDrafts);
+ }
+
@Test
public void byFrom() throws Exception {
TestRepository<Repo> repo = createProject("repo");
