Do not await async ChangeIndex updates in Batch Update, subject to experiment
Index calls are executed asynchronously, but they are awaited in
BatchUpdate. If Index operation is slow, it slows down the entire
update. If the indexing failed, the update is not reverted anyway (so
the strictly speaking, the caller does not get any consistency
guarantees).
Since for some indexes, an index write can take long, it makes sense to
not await the re-indexing operation, and let the indexing an post ops
finish fully async.
Since there are some unknowns about how this change will actually affect
the UI and the API users, guard the change by an experiment.
If successful, the experiment can be converted to a config.
Contributors: mariasavtchouk@google.com
Release-Notes: skip
Google-Bug-Id: b/261864182
Change-Id: Idf841969c938b3def19b0314c01e466054c72402
diff --git a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
index 32ec401..e294d55 100644
--- a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
+++ b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
@@ -25,4 +25,8 @@
/** Features, enabled by default in the current release. */
public static final ImmutableSet<String> DEFAULT_ENABLED_FEATURES = ImmutableSet.of();
+
+ /** On BatchUpdate, do not await index completion before returning to the user */
+ public static String GERRIT_BACKEND_FEATURE_DO_NOT_AWAIT_CHANGE_INDEXING =
+ "GerritBackendFeature__do_not_await_change_indexing";
}
diff --git a/java/com/google/gerrit/server/update/BatchUpdate.java b/java/com/google/gerrit/server/update/BatchUpdate.java
index 3f5fa97..9250513 100644
--- a/java/com/google/gerrit/server/update/BatchUpdate.java
+++ b/java/com/google/gerrit/server/update/BatchUpdate.java
@@ -58,6 +58,8 @@
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.change.NotifyResolver;
+import com.google.gerrit.server.experiments.ExperimentFeatures;
+import com.google.gerrit.server.experiments.ExperimentFeaturesConstants;
import com.google.gerrit.server.extensions.events.AttentionSetObserver;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -423,6 +425,7 @@
private final Map<Change.Id, Change> newChanges = new HashMap<>();
private final List<OpData<RepoOnlyOp>> repoOnlyOps = new ArrayList<>();
private final Map<Change.Id, NotifyHandling> perChangeNotifyHandling = new HashMap<>();
+ private final ExperimentFeatures experimentFeatures;
private RepoView repoView;
private BatchRefUpdate batchRefUpdate;
@@ -449,6 +452,7 @@
GitReferenceUpdated gitRefUpdated,
RefLogIdentityProvider refLogIdentityProvider,
AttentionSetObserver attentionSetObserver,
+ ExperimentFeatures experimentFeatures,
@Assisted Project.NameKey project,
@Assisted CurrentUser user,
@Assisted Instant when) {
@@ -462,6 +466,7 @@
this.gitRefUpdated = gitRefUpdated;
this.refLogIdentityProvider = refLogIdentityProvider;
this.attentionSetObserver = attentionSetObserver;
+ this.experimentFeatures = experimentFeatures;
this.project = project;
this.user = user;
this.when = when;
@@ -661,6 +666,11 @@
}
}
+ private boolean indexAsync() {
+ return experimentFeatures.isFeatureEnabled(
+ ExperimentFeaturesConstants.GERRIT_BACKEND_FEATURE_DO_NOT_AWAIT_CHANGE_INDEXING);
+ }
+
private void fireRefChangeEvent() {
if (batchRefUpdate != null) {
gitRefUpdated.fire(project, batchRefUpdate, getAccount().orElse(null));
@@ -683,11 +693,13 @@
private final NoteDbUpdateManager manager;
private final boolean dryrun;
private final Map<Change.Id, ChangeResult> results;
+ private final boolean indexAsync;
- ChangesHandle(NoteDbUpdateManager manager, boolean dryrun) {
+ ChangesHandle(NoteDbUpdateManager manager, boolean dryrun, boolean indexAsync) {
this.manager = manager;
this.dryrun = dryrun;
results = new HashMap<>();
+ this.indexAsync = indexAsync;
}
@Override
@@ -738,6 +750,17 @@
throw new IllegalStateException("unexpected result: " + e.getValue());
}
}
+ if (indexAsync) {
+ // We want to index asynchronously. However, the callers will await all
+ // index futures. This allows us to - even in synchronous case -
+ // parallelize indexing changes.
+ // Returning immediate futures for newly-created change data objects
+ // while letting the actual futures go will make actual indexing
+ // asynchronous.
+ return results.keySet().stream()
+ .map(cId -> Futures.immediateFuture(changeDataFactory.create(project, cId)))
+ .collect(toImmutableList());
+ }
return indexFutures.build();
}
}
@@ -759,7 +782,8 @@
.setBatchUpdateListeners(batchUpdateListeners)
.setChangeRepo(
repo, repoView.getRevWalk(), repoView.getInserter(), repoView.getCommands()),
- dryrun);
+ dryrun,
+ indexAsync());
getRefLogIdent().ifPresent(handle.manager::setRefLogIdent);
handle.manager.setRefLogMessage(refLogMessage);
handle.manager.setPushCertificate(pushCert);
