opendev/gerrit
Code Issues Proposed changes

Merge changes Ib7773ffa,I6eccd939,I854037bc,I1d07b8ec

Browse Source 
* 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
This commit is contained in:
David Pursehouse
2019-09-20 01:11:08 +00:00
committed by Gerrit Code Review
parent 88c71f2b01 b088c1b0e6
commit 2428c0f075
9 changed files with 176 additions and 68 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
java/com/google/gerrit/acceptance/GerritServer.java
View File

@@ -522,7 +522,9 @@ public class GerritServer implements AutoCloseable {
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 {

8
java/com/google/gerrit/extensions/api/projects/ProjectApi.java
View File

@@ -201,6 +201,9 @@ public interface ProjectApi {
*/
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 interface ProjectApi {
public void index(boolean indexChildren) throws RestApiException {
throw new NotImplementedException();
}
@Override
public void indexChanges() throws RestApiException {
throw new NotImplementedException();
}
}
}

15
java/com/google/gerrit/extensions/events/ChangeIndexedListener.java
View File

@@ -19,6 +19,21 @@ import com.google.gerrit.extensions.annotations.ExtensionPoint;
/** Notified whenever a change is indexed or deleted from the index. */
@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.
*

18
java/com/google/gerrit/server/api/projects/ProjectApiImpl.java
View File

@@ -43,6 +43,7 @@ import com.google.gerrit.extensions.api.projects.ProjectInput;
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.GetDescription;
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 @@ public class ProjectApiImpl implements ProjectApi {
private final GetParent getParent;
private final SetParent setParent;
private final Index index;
private final IndexChanges indexChanges;
@AssistedInject
ProjectApiImpl(
@@ -158,6 +161,7 @@ public class ProjectApiImpl implements ProjectApi {
GetParent getParent,
SetParent setParent,
Index index,
IndexChanges indexChanges,
@Assisted ProjectResource project) {
this(
permissionBackend,
@@ -192,6 +196,7 @@ public class ProjectApiImpl implements ProjectApi {
getParent,
setParent,
index,
indexChanges,
null);
}
@@ -228,6 +233,7 @@ public class ProjectApiImpl implements ProjectApi {
GetParent getParent,
SetParent setParent,
Index index,
IndexChanges indexChanges,
@Assisted String name) {
this(
permissionBackend,
@@ -262,6 +268,7 @@ public class ProjectApiImpl implements ProjectApi {
getParent,
setParent,
index,
indexChanges,
name);
}
@@ -298,6 +305,7 @@ public class ProjectApiImpl implements ProjectApi {
GetParent getParent,
SetParent setParent,
Index index,
IndexChanges indexChanges,
String name) {
this.permissionBackend = permissionBackend;
this.createProject = createProject;
@@ -332,6 +340,7 @@ public class ProjectApiImpl implements ProjectApi {
this.setParent = setParent;
this.name = name;
this.index = index;
this.indexChanges = indexChanges;
}
@Override
@@ -648,6 +657,15 @@ public class ProjectApiImpl implements ProjectApi {
}
}
@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);

35
java/com/google/gerrit/server/index/change/ChangeIndexer.java
View File

@@ -31,7 +31,9 @@ import com.google.gerrit.server.index.IndexExecutor;
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 @@ public class ChangeIndexer {
@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 @@ public class ChangeIndexer {
ChangeIndexer(
@GerritServerConfig Config cfg,
ChangeData.Factory changeDataFactory,
ChangeNotes.Factory notesFactory,
ThreadLocalRequestContext context,
PluginSetContext<ChangeIndexedListener> indexedListeners,
StalenessChecker stalenessChecker,
@@ -91,6 +95,7 @@ public class ChangeIndexer {
@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 @@ public class ChangeIndexer {
ChangeIndexer(
@GerritServerConfig Config cfg,
ChangeData.Factory changeDataFactory,
ChangeNotes.Factory notesFactory,
ThreadLocalRequestContext context,
PluginSetContext<ChangeIndexedListener> indexedListeners,
StalenessChecker stalenessChecker,
@@ -112,6 +118,7 @@ public class ChangeIndexer {
@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 class ChangeIndexer {
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 @@ public class ChangeIndexer {
* @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 @@ public class ChangeIndexer {
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 @@ public class ChangeIndexer {
* @return future for the deleting task.
*/
public ListenableFuture<?> deleteAsync(Change.Id id) {
fireChangeScheduledForDeletionFromIndexEvent(id.get());
return submit(new DeleteTask(id));
}
@@ -242,6 +264,11 @@ public class ChangeIndexer {
* @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 @@ public class ChangeIndexer {
@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;
}

68
java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java
View File

@@ -17,7 +17,6 @@ package com.google.gerrit.server.index.change;
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.config.GerritServerConfig;
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 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
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 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
Provider<InternalChangeQuery> queryProvider,
ChangeIndexer.Factory indexerFactory,
ChangeIndexCollection indexes,
ChangeNotes.Factory notesFactory,
AllUsersName allUsersName,
AccountCache accountCache,
Provider<AccountIndexer> indexer,
@@ -83,7 +72,6 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
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 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
@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 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
@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);
}
}
}

26
javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java
View File

@@ -24,6 +24,9 @@ import static com.google.gerrit.server.project.ProjectState.OVERRIDDEN_BY_GLOBAL
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.api.projects.ProjectInput;
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.AuthException;
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 @@ public class ProjectIT extends AbstractDaemonTest {
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;
@@ -472,6 +478,26 @@ public class ProjectIT extends AbstractDaemonTest {
ImmutableMap.of(project.get(), 1L, middle.get(), 1L, leave.get(), 1L));
}
@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();

64
javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
View File

@@ -32,6 +32,10 @@ import static java.util.Objects.requireNonNull;
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.Iterables;
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.client.SubmitType;
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 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
@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 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
}
}
@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();

6
javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
View File

@@ -2115,6 +2115,12 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
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);