Merge changes Ib7773ffa,I6eccd939,I854037bc,I1d07b8ec * changes: Fix flaky AbstractQueryChangesTest#mergeable() test Test that submit triggers asynchronous reindex of open changes on same branch Add test for IndexChanges REST endpoint Simplify ReindexAfterRefUpdate
diff --git a/java/com/google/gerrit/acceptance/GerritServer.java b/java/com/google/gerrit/acceptance/GerritServer.java index ea63d73..4279764 100644 --- a/java/com/google/gerrit/acceptance/GerritServer.java +++ b/java/com/google/gerrit/acceptance/GerritServer.java
@@ -522,7 +522,9 @@ cfg.setInt("sshd", null, "commandStartThreads", 1); cfg.setInt("receive", null, "threadPoolSize", 1); cfg.setInt("index", null, "threads", 1); - cfg.setBoolean("index", null, "reindexAfterRefUpdate", false); + if (cfg.getString("index", null, "reindexAfterRefUpdate") == null) { + cfg.setBoolean("index", null, "reindexAfterRefUpdate", false); + } } private static Injector createTestInjector(Daemon daemon) throws Exception {
diff --git a/java/com/google/gerrit/extensions/api/projects/ProjectApi.java b/java/com/google/gerrit/extensions/api/projects/ProjectApi.java index 3d70996..c6d9dee 100644 --- a/java/com/google/gerrit/extensions/api/projects/ProjectApi.java +++ b/java/com/google/gerrit/extensions/api/projects/ProjectApi.java
@@ -201,6 +201,9 @@ */ void index(boolean indexChildren) throws RestApiException; + /** Reindexes all changes of the project. */ + void indexChanges() throws RestApiException; + /** * A default implementation which allows source compatibility when adding new methods to the * interface. @@ -370,5 +373,10 @@ public void index(boolean indexChildren) throws RestApiException { throw new NotImplementedException(); } + + @Override + public void indexChanges() throws RestApiException { + throw new NotImplementedException(); + } } }
diff --git a/java/com/google/gerrit/extensions/events/ChangeIndexedListener.java b/java/com/google/gerrit/extensions/events/ChangeIndexedListener.java index 8dd64ed..64bc022 100644 --- a/java/com/google/gerrit/extensions/events/ChangeIndexedListener.java +++ b/java/com/google/gerrit/extensions/events/ChangeIndexedListener.java
@@ -20,6 +20,21 @@ @ExtensionPoint public interface ChangeIndexedListener { /** + * Invoked when a change is scheduled for indexing. + * + * @param projectName project containing the change + * @param id id of the change that was scheduled for indexing + */ + default void onChangeScheduledForIndexing(String projectName, int id) {} + + /** + * Invoked when a change is scheduled for deletion from indexing. + * + * @param id id of the change that was scheduled for deletion from indexing + */ + default void onChangeScheduledForDeletionFromIndex(int id) {} + + /** * Invoked when a change is indexed. * * @param projectName project containing the change
diff --git a/java/com/google/gerrit/server/api/projects/ProjectApiImpl.java b/java/com/google/gerrit/server/api/projects/ProjectApiImpl.java index dc968f6..1ac905d 100644 --- a/java/com/google/gerrit/server/api/projects/ProjectApiImpl.java +++ b/java/com/google/gerrit/server/api/projects/ProjectApiImpl.java
@@ -43,6 +43,7 @@ import com.google.gerrit.extensions.api.projects.TagApi; import com.google.gerrit.extensions.api.projects.TagInfo; import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.extensions.common.Input; import com.google.gerrit.extensions.common.ProjectInfo; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.IdString; @@ -69,6 +70,7 @@ import com.google.gerrit.server.restapi.project.GetHead; import com.google.gerrit.server.restapi.project.GetParent; import com.google.gerrit.server.restapi.project.Index; +import com.google.gerrit.server.restapi.project.IndexChanges; import com.google.gerrit.server.restapi.project.ListBranches; import com.google.gerrit.server.restapi.project.ListDashboards; import com.google.gerrit.server.restapi.project.ListTags; @@ -124,6 +126,7 @@ private final GetParent getParent; private final SetParent setParent; private final Index index; + private final IndexChanges indexChanges; @AssistedInject ProjectApiImpl( @@ -158,6 +161,7 @@ GetParent getParent, SetParent setParent, Index index, + IndexChanges indexChanges, @Assisted ProjectResource project) { this( permissionBackend, @@ -192,6 +196,7 @@ getParent, setParent, index, + indexChanges, null); } @@ -228,6 +233,7 @@ GetParent getParent, SetParent setParent, Index index, + IndexChanges indexChanges, @Assisted String name) { this( permissionBackend, @@ -262,6 +268,7 @@ getParent, setParent, index, + indexChanges, name); } @@ -298,6 +305,7 @@ GetParent getParent, SetParent setParent, Index index, + IndexChanges indexChanges, String name) { this.permissionBackend = permissionBackend; this.createProject = createProject; @@ -332,6 +340,7 @@ this.setParent = setParent; this.name = name; this.index = index; + this.indexChanges = indexChanges; } @Override @@ -648,6 +657,15 @@ } } + @Override + public void indexChanges() throws RestApiException { + try { + indexChanges.apply(checkExists(), new Input()); + } catch (Exception e) { + throw asRestApiException("Cannot index changes", e); + } + } + private ProjectResource checkExists() throws ResourceNotFoundException { if (project == null) { throw new ResourceNotFoundException(name);
diff --git a/java/com/google/gerrit/server/index/change/ChangeIndexer.java b/java/com/google/gerrit/server/index/change/ChangeIndexer.java index 07bd963..4a1180c 100644 --- a/java/com/google/gerrit/server/index/change/ChangeIndexer.java +++ b/java/com/google/gerrit/server/index/change/ChangeIndexer.java
@@ -31,7 +31,9 @@ import com.google.gerrit.server.logging.Metadata; import com.google.gerrit.server.logging.TraceContext; import com.google.gerrit.server.logging.TraceContext.TraceTimer; +import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.plugincontext.PluginSetContext; +import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.util.RequestContext; import com.google.gerrit.server.util.ThreadLocalRequestContext; @@ -67,6 +69,7 @@ @Nullable private final ChangeIndexCollection indexes; @Nullable private final ChangeIndex index; private final ChangeData.Factory changeDataFactory; + private final ChangeNotes.Factory notesFactory; private final ThreadLocalRequestContext context; private final ListeningExecutorService batchExecutor; private final ListeningExecutorService executor; @@ -83,6 +86,7 @@ ChangeIndexer( @GerritServerConfig Config cfg, ChangeData.Factory changeDataFactory, + ChangeNotes.Factory notesFactory, ThreadLocalRequestContext context, PluginSetContext<ChangeIndexedListener> indexedListeners, StalenessChecker stalenessChecker, @@ -91,6 +95,7 @@ @Assisted ChangeIndex index) { this.executor = executor; this.changeDataFactory = changeDataFactory; + this.notesFactory = notesFactory; this.context = context; this.indexedListeners = indexedListeners; this.stalenessChecker = stalenessChecker; @@ -104,6 +109,7 @@ ChangeIndexer( @GerritServerConfig Config cfg, ChangeData.Factory changeDataFactory, + ChangeNotes.Factory notesFactory, ThreadLocalRequestContext context, PluginSetContext<ChangeIndexedListener> indexedListeners, StalenessChecker stalenessChecker, @@ -112,6 +118,7 @@ @Assisted ChangeIndexCollection indexes) { this.executor = executor; this.changeDataFactory = changeDataFactory; + this.notesFactory = notesFactory; this.context = context; this.indexedListeners = indexedListeners; this.stalenessChecker = stalenessChecker; @@ -134,6 +141,7 @@ public ListenableFuture<?> indexAsync(Project.NameKey project, Change.Id id) { IndexTask task = new IndexTask(project, id); if (queuedIndexTasks.add(task)) { + fireChangeScheduledForIndexingEvent(project.get(), id.get()); return submit(task); } return Futures.immediateFuture(null); @@ -159,6 +167,11 @@ * @param cd change to index. */ public void index(ChangeData cd) { + fireChangeScheduledForIndexingEvent(cd.project().get(), cd.getId().get()); + doIndex(cd); + } + + private void doIndex(ChangeData cd) { indexImpl(cd); // Always double-check whether the change might be stale immediately after @@ -199,10 +212,18 @@ fireChangeIndexedEvent(cd.project().get(), cd.getId().get()); } + private void fireChangeScheduledForIndexingEvent(String projectName, int id) { + indexedListeners.runEach(l -> l.onChangeScheduledForIndexing(projectName, id)); + } + private void fireChangeIndexedEvent(String projectName, int id) { indexedListeners.runEach(l -> l.onChangeIndexed(projectName, id)); } + private void fireChangeScheduledForDeletionFromIndexEvent(int id) { + indexedListeners.runEach(l -> l.onChangeScheduledForDeletionFromIndex(id)); + } + private void fireChangeDeletedFromIndexEvent(int id) { indexedListeners.runEach(l -> l.onChangeDeleted(id)); } @@ -233,6 +254,7 @@ * @return future for the deleting task. */ public ListenableFuture<?> deleteAsync(Change.Id id) { + fireChangeScheduledForDeletionFromIndexEvent(id.get()); return submit(new DeleteTask(id)); } @@ -242,6 +264,11 @@ * @param id change ID to delete. */ public void delete(Change.Id id) { + fireChangeScheduledForDeletionFromIndexEvent(id.get()); + doDelete(id); + } + + private void doDelete(Change.Id id) { new DeleteTask(id).call(); } @@ -332,8 +359,12 @@ @Override public Void callImpl() throws Exception { remove(); - ChangeData cd = changeDataFactory.create(project, id); - index(cd); + try { + ChangeNotes changeNotes = notesFactory.createChecked(project, id); + doIndex(changeDataFactory.create(changeNotes)); + } catch (NoSuchChangeException e) { + doDelete(id); + } return null; }
diff --git a/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java b/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java index 21579d2..c429158 100644 --- a/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java +++ b/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java
@@ -17,7 +17,6 @@ import static com.google.common.util.concurrent.MoreExecutors.directExecutor; import static com.google.gerrit.server.query.change.ChangeData.asChanges; -import com.google.common.base.Objects; import com.google.common.flogger.FluentLogger; import com.google.common.util.concurrent.FutureCallback; import com.google.common.util.concurrent.Futures; @@ -34,20 +33,14 @@ import com.google.gerrit.server.git.QueueProvider.QueueType; import com.google.gerrit.server.index.IndexExecutor; import com.google.gerrit.server.index.account.AccountIndexer; -import com.google.gerrit.server.notedb.ChangeNotes; -import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gerrit.server.util.ManualRequestContext; import com.google.gerrit.server.util.OneOffRequestContext; import com.google.gerrit.server.util.RequestContext; import com.google.inject.Inject; import com.google.inject.Provider; -import java.io.IOException; -import java.util.Collections; import java.util.List; -import java.util.Set; import java.util.concurrent.Callable; -import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Future; import org.eclipse.jgit.lib.Config; @@ -58,15 +51,12 @@ private final Provider<InternalChangeQuery> queryProvider; private final ChangeIndexer.Factory indexerFactory; private final ChangeIndexCollection indexes; - private final ChangeNotes.Factory notesFactory; private final AllUsersName allUsersName; private final AccountCache accountCache; private final Provider<AccountIndexer> indexer; private final ListeningExecutorService executor; private final boolean enabled; - private final Set<Index> queuedIndexTasks = Collections.newSetFromMap(new ConcurrentHashMap<>()); - @Inject ReindexAfterRefUpdate( @GerritServerConfig Config cfg, @@ -74,7 +64,6 @@ Provider<InternalChangeQuery> queryProvider, ChangeIndexer.Factory indexerFactory, ChangeIndexCollection indexes, - ChangeNotes.Factory notesFactory, AllUsersName allUsersName, AccountCache accountCache, Provider<AccountIndexer> indexer, @@ -83,7 +72,6 @@ this.queryProvider = queryProvider; this.indexerFactory = indexerFactory; this.indexes = indexes; - this.notesFactory = notesFactory; this.allUsersName = allUsersName; this.accountCache = accountCache; this.indexer = indexer; @@ -113,12 +101,9 @@ @Override public void onSuccess(List<Change> changes) { for (Change c : changes) { - Index task = new Index(event, c.getId()); - if (queuedIndexTasks.add(task)) { - // Don't retry indefinitely; if this fails changes may be stale. - @SuppressWarnings("unused") - Future<?> possiblyIgnoredError = executor.submit(task); - } + @SuppressWarnings("unused") + Future<?> possiblyIgnoredError = + indexerFactory.create(executor, indexes).indexAsync(c.getProject(), c.getId()); } } @@ -178,51 +163,4 @@ @Override protected void remove() {} } - - private class Index extends Task<Void> { - private final Change.Id id; - - Index(Event event, Change.Id id) { - super(event); - this.id = id; - } - - @Override - protected Void impl(RequestContext ctx) throws IOException { - // Reload change, as some time may have passed since GetChanges. - remove(); - try { - Change c = - notesFactory.createChecked(Project.nameKey(event.getProjectName()), id).getChange(); - indexerFactory.create(executor, indexes).index(c); - } catch (NoSuchChangeException e) { - indexerFactory.create(executor, indexes).delete(id); - } - return null; - } - - @Override - public int hashCode() { - return Objects.hashCode(Index.class, id.get()); - } - - @Override - public boolean equals(Object obj) { - if (!(obj instanceof Index)) { - return false; - } - Index other = (Index) obj; - return id.get() == other.id.get(); - } - - @Override - public String toString() { - return "Index change " + id.get() + " of project " + event.getProjectName(); - } - - @Override - protected void remove() { - queuedIndexTasks.remove(this); - } - } }
diff --git a/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java b/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java index a9c7d99..e3796c4 100644 --- a/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java +++ b/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java
@@ -24,6 +24,9 @@ import static com.google.gerrit.server.project.ProjectState.OVERRIDDEN_BY_PARENT; import static com.google.gerrit.testing.GerritJUnit.assertThrows; import static java.util.stream.Collectors.toSet; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -47,6 +50,7 @@ import com.google.gerrit.extensions.client.InheritableBoolean; import com.google.gerrit.extensions.client.ProjectState; import com.google.gerrit.extensions.client.SubmitType; +import com.google.gerrit.extensions.events.ChangeIndexedListener; import com.google.gerrit.extensions.events.ProjectIndexedListener; import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.extensions.registration.RegistrationHandle; @@ -54,6 +58,7 @@ import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; +import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.group.SystemGroupBackend; @@ -79,6 +84,7 @@ private static final String JIRA_MATCH = "(jira\\\\s+#?)(\\\\d+)"; @Inject private DynamicSet<ProjectIndexedListener> projectIndexedListeners; + @Inject private DynamicSet<ChangeIndexedListener> changeIndexedListeners; @Inject private ProjectOperations projectOperations; @Inject private RequestScopeOperations requestScopeOperations; @@ -473,6 +479,26 @@ } @Test + public void reindexChangesOfProject() throws Exception { + Change.Id changeId1 = createChange().getChange().getId(); + Change.Id changeId2 = createChange().getChange().getId(); + + ChangeIndexedListener changeIndexedListener = mock(ChangeIndexedListener.class); + RegistrationHandle registrationHandle = + changeIndexedListeners.add("gerrit", changeIndexedListener); + try { + gApi.projects().name(project.get()).indexChanges(); + + verify(changeIndexedListener, times(1)) + .onChangeScheduledForIndexing(project.get(), changeId1.get()); + verify(changeIndexedListener, times(1)) + .onChangeScheduledForIndexing(project.get(), changeId2.get()); + } finally { + registrationHandle.remove(); + } + } + + @Test public void maxObjectSizeIsNotSetByDefault() throws Exception { ConfigInfo info = getConfig(); assertThat(info.maxObjectSizeLimit.value).isNull();
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java index c2df9ca..a756c97 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
@@ -32,6 +32,10 @@ import static java.util.concurrent.TimeUnit.SECONDS; import static java.util.stream.Collectors.toList; import static org.eclipse.jgit.lib.Constants.EMPTY_TREE_ID; +import static org.mockito.Mockito.atLeast; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -39,6 +43,7 @@ import com.google.common.collect.Lists; import com.google.common.collect.Sets; import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.GerritConfig; import com.google.gerrit.acceptance.GitUtil; import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.PushOneCommit; @@ -60,6 +65,7 @@ import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.ChangeInput; import com.google.gerrit.extensions.common.LabelInfo; +import com.google.gerrit.extensions.events.ChangeIndexedListener; import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.extensions.registration.RegistrationHandle; import com.google.gerrit.extensions.restapi.AuthException; @@ -125,6 +131,7 @@ @Inject private ApprovalsUtil approvalsUtil; @Inject private DynamicSet<OnSubmitValidationListener> onSubmitValidationListeners; + @Inject private DynamicSet<ChangeIndexedListener> changeIndexedListeners; @Inject private IdentifiedUser.GenericFactory userFactory; @Inject private ProjectOperations projectOperations; @Inject private RequestScopeOperations requestScopeOperations; @@ -1210,6 +1217,63 @@ } } + @Test + @GerritConfig(name = "index.reindexAfterRefUpdate", value = "true") + public void submitSchedulesOpenChangesOfSameBranchForReindexing() throws Throwable { + // Create a merged change. + PushOneCommit push = + pushFactory.create(admin.newIdent(), testRepo, "Merged Change", "foo.txt", "foo"); + PushOneCommit.Result mergedChange = push.to("refs/for/master"); + mergedChange.assertOkStatus(); + approve(mergedChange.getChangeId()); + submit(mergedChange.getChangeId()); + + // Create some open changes. + PushOneCommit.Result change1 = createChange(); + PushOneCommit.Result change2 = createChange(); + PushOneCommit.Result change3 = createChange(); + + // Create a branch with one open change. + BranchInput in = new BranchInput(); + in.revision = projectOperations.project(project).getHead("master").name(); + gApi.projects().name(project.get()).branch("dev").create(in); + PushOneCommit.Result changeOtherBranch = createChange("refs/for/dev"); + + ChangeIndexedListener changeIndexedListener = mock(ChangeIndexedListener.class); + RegistrationHandle registrationHandle = + changeIndexedListeners.add("gerrit", changeIndexedListener); + try { + // submit a change, this should trigger asynchronous reindexing of the open changes on the + // same branch + approve(change1.getChangeId()); + submit(change1.getChangeId()); + assertThat(gApi.changes().id(change1.getChangeId()).get().status) + .isEqualTo(ChangeStatus.MERGED); + + // on submit the change that is submitted gets reindexed synchronously + verify(changeIndexedListener, atLeast(1)) + .onChangeScheduledForIndexing(project.get(), change1.getChange().getId().get()); + verify(changeIndexedListener, atLeast(1)) + .onChangeIndexed(project.get(), change1.getChange().getId().get()); + + // the open changes on the same branch get reindexed asynchronously + verify(changeIndexedListener, times(1)) + .onChangeScheduledForIndexing(project.get(), change2.getChange().getId().get()); + verify(changeIndexedListener, times(1)) + .onChangeScheduledForIndexing(project.get(), change3.getChange().getId().get()); + + // merged changes don't get reindexed + verify(changeIndexedListener, times(0)) + .onChangeScheduledForIndexing(project.get(), mergedChange.getChange().getId().get()); + + // open changes on other branches don't get reindexed + verify(changeIndexedListener, times(0)) + .onChangeScheduledForIndexing(project.get(), changeOtherBranch.getChange().getId().get()); + } finally { + registrationHandle.remove(); + } + } + private void assertSubmitter(PushOneCommit.Result change) throws Throwable { ChangeInfo info = get(change.getChangeId(), ListChangesOption.MESSAGES); assertThat(info.messages).isNotNull();
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index b0555ca..20f3bb9 100644 --- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -2115,6 +2115,12 @@ gApi.changes().id(change1.getChangeId()).current().review(ReviewInput.approve()); gApi.changes().id(change1.getChangeId()).current().submit(); + // If a change gets submitted, the remaining open changes get reindexed asynchronously to update + // their mergeability information. If the further assertions in this test are done before the + // asynchronous reindex completed they fail because the mergeability information in the index + // was not updated yet. To avoid this flakiness we index change2 synchronously here. + gApi.changes().id(change2.getChangeId()).index(); + assertQuery("status:open conflicts:" + change2.getId().get()); assertQuery("status:open is:mergeable"); assertQuery("status:open -is:mergeable", change2);