BatchUpdate: Cache ChangeData instances that are created for indexing
When a change is updated it needs to be reindexed. For reindexing a
change a ChangeData instance is created and the submit rules are
executed. In the postUpdate step of BatchUpdate a ChangaData instance
for the updated change is needed again to send a change-updated event.
For the event the change is formatted as JSON which triggers the submit
rules a second time.
ChangeData already caches the result of running submit rules, this means
if we can avoid that ChangeData is instantiated twice, we can avoid that
the submit rules are executed twice. This is important since executing
submit rules is rather expensive.
To avoid instantiating ChangeData twice we now cache the ChangeData
instances that were created for indexing and make them available to the
postUpdate step via the context that is provided to the postUpdate step.
With this change the postUpdate steps do not use the provided ChangeData
instances yet, so that ChangeData is still instantiated a second time
for sending the event. This will be addressed in a follow-up change.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I415d745deb148ea12b1c1f4ed06cc695b7355d90
diff --git a/java/com/google/gerrit/server/PublishCommentsOp.java b/java/com/google/gerrit/server/PublishCommentsOp.java
index 358ce92..e0f86c6 100644
--- a/java/com/google/gerrit/server/PublishCommentsOp.java
+++ b/java/com/google/gerrit/server/PublishCommentsOp.java
@@ -31,7 +31,7 @@
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.CommentsRejectedException;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.RepoView;
import com.google.gerrit.server.util.LabelVote;
import com.google.inject.Inject;
@@ -108,7 +108,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(PostUpdateContext ctx) {
if (message == null || comments.isEmpty()) {
return;
}
diff --git a/java/com/google/gerrit/server/change/AbandonOp.java b/java/com/google/gerrit/server/change/AbandonOp.java
index 6c39ed0..f8a3601 100644
--- a/java/com/google/gerrit/server/change/AbandonOp.java
+++ b/java/com/google/gerrit/server/change/AbandonOp.java
@@ -32,7 +32,7 @@
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -111,7 +111,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(PostUpdateContext ctx) {
NotifyResolver.Result notify = ctx.getNotify(change.getId());
try {
ReplyToChangeSender emailSender =
diff --git a/java/com/google/gerrit/server/change/AddReviewersOp.java b/java/com/google/gerrit/server/change/AddReviewersOp.java
index ff8e5c6..250a393 100644
--- a/java/com/google/gerrit/server/change/AddReviewersOp.java
+++ b/java/com/google/gerrit/server/change/AddReviewersOp.java
@@ -44,7 +44,7 @@
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
@@ -238,7 +238,7 @@
}
@Override
- public void postUpdate(Context ctx) throws Exception {
+ public void postUpdate(PostUpdateContext ctx) throws Exception {
opResult =
Result.builder()
.setAddedReviewers(addedReviewers)
diff --git a/java/com/google/gerrit/server/change/AddToAttentionSetOp.java b/java/com/google/gerrit/server/change/AddToAttentionSetOp.java
index 8053b30..a980c32 100644
--- a/java/com/google/gerrit/server/change/AddToAttentionSetOp.java
+++ b/java/com/google/gerrit/server/change/AddToAttentionSetOp.java
@@ -27,7 +27,7 @@
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.util.AttentionSetEmail;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -101,7 +101,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(PostUpdateContext ctx) {
if (!notify) {
return;
}
diff --git a/java/com/google/gerrit/server/change/ChangeInserter.java b/java/com/google/gerrit/server/change/ChangeInserter.java
index fb027bd..fb0158d 100644
--- a/java/com/google/gerrit/server/change/ChangeInserter.java
+++ b/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -74,6 +74,7 @@
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
import com.google.gerrit.server.update.InsertChangeOp;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.RepoContext;
import com.google.gerrit.server.util.CommitMessageUtil;
import com.google.gerrit.server.util.RequestScopePropagator;
@@ -475,7 +476,7 @@
}
@Override
- public void postUpdate(Context ctx) throws Exception {
+ public void postUpdate(PostUpdateContext ctx) throws Exception {
reviewerAdditions.postUpdate(ctx);
NotifyResolver.Result notify = ctx.getNotify(change.getId());
if (sendMail && notify.shouldNotify()) {
diff --git a/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java b/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java
index 255e13a..2505024 100644
--- a/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java
+++ b/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java
@@ -24,7 +24,7 @@
import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.util.Collections;
@@ -73,7 +73,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(PostUpdateContext ctx) {
try {
NotifyResolver.Result notify = ctx.getNotify(change.getId());
if (!notify.shouldNotify()) {
diff --git a/java/com/google/gerrit/server/change/DeleteReviewerOp.java b/java/com/google/gerrit/server/change/DeleteReviewerOp.java
index bf00d27..84f0178 100644
--- a/java/com/google/gerrit/server/change/DeleteReviewerOp.java
+++ b/java/com/google/gerrit/server/change/DeleteReviewerOp.java
@@ -46,7 +46,7 @@
import com.google.gerrit.server.project.RemoveReviewerControl;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.RepoView;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -177,7 +177,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(PostUpdateContext ctx) {
NotifyResolver.Result notify = ctx.getNotify(currChange.getId());
if (input.notify == null
&& currChange.isWorkInProgress()
diff --git a/java/com/google/gerrit/server/change/PatchSetInserter.java b/java/com/google/gerrit/server/change/PatchSetInserter.java
index d2bf3fe..c7e2590 100644
--- a/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -53,7 +53,7 @@
import com.google.gerrit.server.ssh.NoSshInfo;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.RepoContext;
import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.Inject;
@@ -298,7 +298,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(PostUpdateContext ctx) {
NotifyResolver.Result notify = ctx.getNotify(change.getId());
if (notify.shouldNotify() && sendEmail) {
requireNonNull(changeMessage);
diff --git a/java/com/google/gerrit/server/change/RebaseChangeOp.java b/java/com/google/gerrit/server/change/RebaseChangeOp.java
index b43996e..aee04ec 100644
--- a/java/com/google/gerrit/server/change/RebaseChangeOp.java
+++ b/java/com/google/gerrit/server/change/RebaseChangeOp.java
@@ -41,7 +41,7 @@
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.RepoContext;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -270,7 +270,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(PostUpdateContext ctx) {
patchSetInserter.postUpdate(ctx);
}
diff --git a/java/com/google/gerrit/server/change/RemoveFromAttentionSetOp.java b/java/com/google/gerrit/server/change/RemoveFromAttentionSetOp.java
index e532409..50ee9d4 100644
--- a/java/com/google/gerrit/server/change/RemoveFromAttentionSetOp.java
+++ b/java/com/google/gerrit/server/change/RemoveFromAttentionSetOp.java
@@ -28,7 +28,7 @@
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.util.AttentionSetEmail;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -101,7 +101,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(PostUpdateContext ctx) {
if (!notify) {
return;
}
diff --git a/java/com/google/gerrit/server/change/ReviewerAdder.java b/java/com/google/gerrit/server/change/ReviewerAdder.java
index 5d55b4d..1732a2c 100644
--- a/java/com/google/gerrit/server/change/ReviewerAdder.java
+++ b/java/com/google/gerrit/server/change/ReviewerAdder.java
@@ -69,7 +69,7 @@
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.io.IOException;
@@ -586,7 +586,7 @@
}
}
- public void postUpdate(Context ctx) throws Exception {
+ public void postUpdate(PostUpdateContext ctx) throws Exception {
for (ReviewerAddition addition : additions()) {
if (addition.op != null) {
addition.op.postUpdate(ctx);
diff --git a/java/com/google/gerrit/server/change/SetAssigneeOp.java b/java/com/google/gerrit/server/change/SetAssigneeOp.java
index 411c9b6..a30444a 100644
--- a/java/com/google/gerrit/server/change/SetAssigneeOp.java
+++ b/java/com/google/gerrit/server/change/SetAssigneeOp.java
@@ -30,7 +30,7 @@
import com.google.gerrit.server.plugincontext.PluginSetContext;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.validators.AssigneeValidationListener;
import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.Inject;
@@ -120,7 +120,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(PostUpdateContext ctx) {
try {
SetAssigneeSender emailSender =
setAssigneeSenderFactory.create(
diff --git a/java/com/google/gerrit/server/change/SetHashtagsOp.java b/java/com/google/gerrit/server/change/SetHashtagsOp.java
index 712e1f3..566a9f2 100644
--- a/java/com/google/gerrit/server/change/SetHashtagsOp.java
+++ b/java/com/google/gerrit/server/change/SetHashtagsOp.java
@@ -35,7 +35,7 @@
import com.google.gerrit.server.plugincontext.PluginSetContext;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.validators.HashtagValidationListener;
import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.Inject;
@@ -144,7 +144,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(PostUpdateContext ctx) {
if (updated() && fireEvent) {
hashtagsEdited.fire(
change, ctx.getAccount(), updatedHashtags, toAdd, toRemove, ctx.getWhen());
diff --git a/java/com/google/gerrit/server/change/SetPrivateOp.java b/java/com/google/gerrit/server/change/SetPrivateOp.java
index 382a4f6..a0c9ec8 100644
--- a/java/com/google/gerrit/server/change/SetPrivateOp.java
+++ b/java/com/google/gerrit/server/change/SetPrivateOp.java
@@ -30,7 +30,7 @@
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -88,7 +88,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(PostUpdateContext ctx) {
if (!isNoOp) {
privateStateChanged.fire(change, ps, ctx.getAccount(), ctx.getWhen());
}
diff --git a/java/com/google/gerrit/server/change/SetTopicOp.java b/java/com/google/gerrit/server/change/SetTopicOp.java
index c4a49b0..203e7da 100644
--- a/java/com/google/gerrit/server/change/SetTopicOp.java
+++ b/java/com/google/gerrit/server/change/SetTopicOp.java
@@ -24,7 +24,7 @@
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -81,7 +81,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(PostUpdateContext ctx) {
if (change != null) {
topicEdited.fire(change, ctx.getAccount(), oldTopicName, ctx.getWhen());
}
diff --git a/java/com/google/gerrit/server/change/WorkInProgressOp.java b/java/com/google/gerrit/server/change/WorkInProgressOp.java
index f0ebb80..384ff08 100644
--- a/java/com/google/gerrit/server/change/WorkInProgressOp.java
+++ b/java/com/google/gerrit/server/change/WorkInProgressOp.java
@@ -30,7 +30,7 @@
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.RepoView;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -126,7 +126,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(PostUpdateContext ctx) {
stateChanged.fire(change, ps, ctx.getAccount(), ctx.getWhen());
NotifyResolver.Result notify = ctx.getNotify(change.getId());
if (workInProgress
diff --git a/java/com/google/gerrit/server/git/CommitUtil.java b/java/com/google/gerrit/server/git/CommitUtil.java
index 47cbd60..82cba5a 100644
--- a/java/com/google/gerrit/server/git/CommitUtil.java
+++ b/java/com/google/gerrit/server/git/CommitUtil.java
@@ -48,7 +48,7 @@
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.CommitMessageUtil;
import com.google.inject.Inject;
@@ -310,7 +310,7 @@
}
@Override
- public void postUpdate(Context ctx) throws Exception {
+ public void postUpdate(PostUpdateContext ctx) throws Exception {
changeReverted.fire(change, ins.getChange(), ctx.getWhen());
try {
RevertedSender emailSender = revertedSenderFactory.create(ctx.getProject(), change.getId());
diff --git a/java/com/google/gerrit/server/git/MergedByPushOp.java b/java/com/google/gerrit/server/git/MergedByPushOp.java
index 78cb013..faf4044 100644
--- a/java/com/google/gerrit/server/git/MergedByPushOp.java
+++ b/java/com/google/gerrit/server/git/MergedByPushOp.java
@@ -33,7 +33,7 @@
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.util.RequestScopePropagator;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -178,7 +178,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(PostUpdateContext ctx) {
if (!correctBranch) {
return;
}
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index f35b3dd..633a407 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -172,7 +172,7 @@
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.RepoContext;
import com.google.gerrit.server.update.RepoOnlyOp;
import com.google.gerrit.server.update.RetryHelper;
@@ -3143,7 +3143,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(PostUpdateContext ctx) {
String refName = cmd.getRefName();
if (cmd.getType() == ReceiveCommand.Type.UPDATE) { // aka fast-forward
logger.atFine().log("Updating tag cache on fast-forward of %s", cmd.getRefName());
diff --git a/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
index ce62d7a..cd1cd3f 100644
--- a/java/com/google/gerrit/server/git/receive/ReplaceOp.java
+++ b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
@@ -74,6 +74,7 @@
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.RepoContext;
import com.google.gerrit.server.util.RequestScopePropagator;
import com.google.gerrit.server.validators.ValidationException;
@@ -493,7 +494,7 @@
}
@Override
- public void postUpdate(Context ctx) throws Exception {
+ public void postUpdate(PostUpdateContext ctx) throws Exception {
reviewerAdditions.postUpdate(ctx);
if (changeKind != ChangeKind.TRIVIAL_REBASE) {
// TODO(dborowitz): Merge email templates so we only have to send one.
diff --git a/java/com/google/gerrit/server/index/change/ChangeIndexer.java b/java/com/google/gerrit/server/index/change/ChangeIndexer.java
index e0f6bec..f7f0f33 100644
--- a/java/com/google/gerrit/server/index/change/ChangeIndexer.java
+++ b/java/com/google/gerrit/server/index/change/ChangeIndexer.java
@@ -46,6 +46,7 @@
import java.util.Collection;
import java.util.Collections;
import java.util.List;
+import java.util.Map;
import java.util.Set;
import java.util.concurrent.Callable;
import java.util.concurrent.ConcurrentHashMap;
@@ -79,8 +80,7 @@
private final StalenessChecker stalenessChecker;
private final boolean autoReindexIfStale;
- private final Set<IndexTask> queuedIndexTasks =
- Collections.newSetFromMap(new ConcurrentHashMap<>());
+ private final Map<Change.Id, IndexTask> queuedIndexTasks = new ConcurrentHashMap<>();
private final Set<ReindexIfStaleTask> queuedReindexIfStaleTasks =
Collections.newSetFromMap(new ConcurrentHashMap<>());
@@ -137,16 +137,44 @@
/**
* Start indexing a change.
*
- * @param id change to index.
+ * @param changeId change to index.
* @return future for the indexing task.
*/
- 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);
+ public ListenableFuture<ChangeData> indexAsync(Project.NameKey project, Change.Id changeId) {
+ // If the change was already scheduled for indexing, we do not need to schedule it again. Change
+ // updates that happened after the change was scheduled for indexing will automatically be taken
+ // into account when the index task is executed (as it reads the current change state).
+ // To skip duplicate index requests, queuedIndexTasks keeps track of the scheduled index tasks.
+ // Here we check if the change has already been scheduled for indexing, and only if not we
+ // create a new index task for the change.
+ // By using computeIfAbsent we ensure that the lookup and the insertion of a new task happens
+ // atomically. Some attempted update operations on this map by other threads may be blocked
+ // while the computation is in progress (but not all as ConcurrentHashMap doesn't lock the
+ // entire table on write, but only segments of the table).
+ IndexTask task =
+ queuedIndexTasks.computeIfAbsent(
+ changeId,
+ id -> {
+ fireChangeScheduledForIndexingEvent(project.get(), id.get());
+ return new IndexTask(project, id);
+ });
+ // Submitting the task to the executor must not happen from within the computeIfAbsent callback,
+ // as this could result in the task being executed before the computeIfAbsent method has
+ // finished (e.g. if a direct executor is used, but also if starting the task asynchronously is
+ // faster than finishing the computeIfAbsent method). This could lead to failures and unexpected
+ // behavior:
+ // * The first thing that IndexTask does is to remove itself from queuedIndexTasks.
+ // This is done so that index requests which are received while an index task for the same
+ // change is in progress, are not dropped but added to the queue. This is important since
+ // the change state that is written to the index is read at the beginning of the index task
+ // and change updates that happen after this read will not be considered when updating the
+ // index.
+ // * Trying to remove the IndexTask from queuedIndexTasks at the beginning of the task doesn't
+ // work if the computeIfAbsent method hasn't finished yet. Either the queuedIndexTasks doesn't
+ // contain the new entry yet and the removal has no effect as it is done before the entry is
+ // added to the map, or the removal fails with {@link IllegalStateException} as recursive
+ // updates from within the computeIfAbsent callback are not allowed.
+ return task.submitIfNeeded();
}
/**
@@ -155,8 +183,9 @@
* @param ids changes to index.
* @return future for completing indexing of all changes.
*/
- public ListenableFuture<?> indexAsync(Project.NameKey project, Collection<Change.Id> ids) {
- List<ListenableFuture<?>> futures = new ArrayList<>(ids.size());
+ public ListenableFuture<List<ChangeData>> indexAsync(
+ Project.NameKey project, Collection<Change.Id> ids) {
+ List<ListenableFuture<ChangeData>> futures = new ArrayList<>(ids.size());
for (Change.Id id : ids) {
futures.add(indexAsync(project, id));
}
@@ -259,9 +288,9 @@
* Start deleting a change.
*
* @param id change to delete.
- * @return future for the deleting task.
+ * @return future for the deleting task, the result of the future is always {@code null}
*/
- public ListenableFuture<?> deleteAsync(Change.Id id) {
+ public ListenableFuture<ChangeData> deleteAsync(Change.Id id) {
fireChangeScheduledForDeletionFromIndexEvent(id.get());
return submit(new DeleteTask(id));
}
@@ -359,17 +388,46 @@
}
}
- private class IndexTask extends AbstractIndexTask<Void> {
+ private class IndexTask extends AbstractIndexTask<ChangeData> {
+ ListenableFuture<ChangeData> future;
+
private IndexTask(Project.NameKey project, Change.Id id) {
super(project, id);
}
+ /**
+ * Submits this task to be executed, if it wasn't submitted yet.
+ *
+ * <p>Submits this task to the executor if it hasn't been submitted yet. The future is cached so
+ * that it can be returned if this method is called again.
+ *
+ * <p>This method must be synchronized so that concurrent calls do not submit this task to the
+ * executor multiple times.
+ *
+ * @return future from which the result of the index task (the {@link ChangeData} instance) can
+ * be retrieved.
+ */
+ private synchronized ListenableFuture<ChangeData> submitIfNeeded() {
+ if (future == null) {
+ future = submit(this);
+ }
+ return future;
+ }
+
@Override
- public Void callImpl() throws Exception {
+ public ChangeData callImpl() throws Exception {
+ // Remove this task from queuedIndexTasks. This is done right at the beginning of this task so
+ // that index requests which are received for the same change while this index task is in
+ // progress, are not dropped but added to the queue. This is important since change updates
+ // that happen after reading the change notes below will not be considered when updating the
+ // index.
remove();
+
try {
ChangeNotes changeNotes = notesFactory.createChecked(project, id);
- doIndex(changeDataFactory.create(changeNotes));
+ ChangeData changeData = changeDataFactory.create(changeNotes);
+ doIndex(changeData);
+ return changeData;
} catch (NoSuchChangeException e) {
doDelete(id);
}
@@ -397,12 +455,12 @@
@Override
protected void remove() {
- queuedIndexTasks.remove(this);
+ queuedIndexTasks.remove(id);
}
}
// Not AbstractIndexTask as it doesn't need a request context.
- private class DeleteTask implements Callable<Void> {
+ private class DeleteTask implements Callable<ChangeData> {
private final Change.Id id;
private DeleteTask(Change.Id id) {
@@ -410,7 +468,7 @@
}
@Override
- public Void call() {
+ public ChangeData call() {
logger.atFine().log("Delete change %d from index.", id.get());
// Don't bother setting a RequestContext to provide the DB.
// Implementations should not need to access the DB in order to delete a
diff --git a/java/com/google/gerrit/server/mail/receive/MailProcessor.java b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
index df38118..87a41e0 100644
--- a/java/com/google/gerrit/server/mail/receive/MailProcessor.java
+++ b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
@@ -65,7 +65,7 @@
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.ManualRequestContext;
@@ -352,7 +352,7 @@
}
@Override
- public void postUpdate(Context ctx) throws Exception {
+ public void postUpdate(PostUpdateContext ctx) throws Exception {
String patchSetComment = null;
if (parsedComments.get(0).getType() == MailComment.CommentType.CHANGE_MESSAGE) {
patchSetComment = parsedComments.get(0).getMessage();
diff --git a/java/com/google/gerrit/server/restapi/change/DeleteAssignee.java b/java/com/google/gerrit/server/restapi/change/DeleteAssignee.java
index 842ed2a..3f603a4 100644
--- a/java/com/google/gerrit/server/restapi/change/DeleteAssignee.java
+++ b/java/com/google/gerrit/server/restapi/change/DeleteAssignee.java
@@ -34,7 +34,7 @@
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.Inject;
@@ -113,7 +113,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(PostUpdateContext ctx) {
assigneeChanged.fire(change, ctx.getAccount(), deletedAssignee, ctx.getWhen());
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/DeleteVote.java b/java/com/google/gerrit/server/restapi/change/DeleteVote.java
index 4b813df..a405b4d 100644
--- a/java/com/google/gerrit/server/restapi/change/DeleteVote.java
+++ b/java/com/google/gerrit/server/restapi/change/DeleteVote.java
@@ -53,7 +53,7 @@
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.LabelVote;
import com.google.gerrit.server.util.time.TimeUtil;
@@ -220,7 +220,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(PostUpdateContext ctx) {
if (changeMessage == null) {
return;
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index 58321e9..49fce0f 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -123,7 +123,7 @@
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.CommentsRejectedException;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.LabelVote;
import com.google.gerrit.server.util.time.TimeUtil;
@@ -928,7 +928,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(PostUpdateContext ctx) {
if (message == null) {
return;
}
diff --git a/java/com/google/gerrit/server/restapi/change/Restore.java b/java/com/google/gerrit/server/restapi/change/Restore.java
index 7faf8e0..6f49e4b 100644
--- a/java/com/google/gerrit/server/restapi/change/Restore.java
+++ b/java/com/google/gerrit/server/restapi/change/Restore.java
@@ -47,7 +47,7 @@
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.Inject;
@@ -148,7 +148,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(PostUpdateContext ctx) {
try {
ReplyToChangeSender emailSender =
restoredSenderFactory.create(ctx.getProject(), change.getId());
diff --git a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
index cb91faa..7c47864 100644
--- a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
+++ b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
@@ -75,7 +75,7 @@
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.CommitMessageUtil;
import com.google.gerrit.server.util.time.TimeUtil;
@@ -614,7 +614,7 @@
}
@Override
- public void postUpdate(Context ctx) throws Exception {
+ public void postUpdate(PostUpdateContext ctx) throws Exception {
changeReverted.fire(
change,
changeNotesFactory.createChecked(ctx.getProject(), revertChangeId).getChange(),
diff --git a/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java b/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
index db48cce..cc3b75d 100644
--- a/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
+++ b/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
@@ -35,7 +35,7 @@
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.RepoContext;
import java.io.IOException;
import java.util.ArrayList;
@@ -270,7 +270,7 @@
}
@Override
- public void postUpdateImpl(Context ctx) {
+ public void postUpdateImpl(PostUpdateContext ctx) {
if (rebaseOp != null) {
rebaseOp.postUpdate(ctx);
}
diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
index 69207ac..0baa7d0 100644
--- a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
+++ b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
@@ -48,7 +48,7 @@
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.RepoContext;
import java.io.IOException;
import java.util.ArrayList;
@@ -463,7 +463,7 @@
}
@Override
- public final void postUpdate(Context ctx) throws Exception {
+ public final void postUpdate(PostUpdateContext ctx) throws Exception {
if (changeAlreadyMerged) {
// TODO(dborowitz): This is suboptimal behavior in the presence of retries: postUpdate steps
// will never get run for changes that submitted successfully on any but the final attempt.
@@ -542,10 +542,10 @@
}
/**
- * @see #postUpdate(Context)
+ * @see #postUpdate(PostUpdateContext)
* @param ctx
*/
- protected void postUpdateImpl(Context ctx) throws Exception {}
+ protected void postUpdateImpl(PostUpdateContext ctx) throws Exception {}
/**
* Amend the commit with gitlink update
diff --git a/java/com/google/gerrit/server/update/BatchUpdate.java b/java/com/google/gerrit/server/update/BatchUpdate.java
index 7fdf833..13e46ed 100644
--- a/java/com/google/gerrit/server/update/BatchUpdate.java
+++ b/java/com/google/gerrit/server/update/BatchUpdate.java
@@ -64,6 +64,7 @@
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.NoSuchRefException;
+import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import com.google.inject.Module;
import com.google.inject.assistedinject.Assisted;
@@ -74,9 +75,11 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.Optional;
import java.util.TimeZone;
import java.util.TreeMap;
+import java.util.function.Function;
import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.PersonIdent;
@@ -134,7 +137,7 @@
checkDifferentProject(updates);
try {
- List<ListenableFuture<?>> indexFutures = new ArrayList<>();
+ List<ListenableFuture<ChangeData>> indexFutures = new ArrayList<>();
List<ChangesHandle> changesHandles = new ArrayList<>(updates.size());
try {
for (BatchUpdate u : updates) {
@@ -156,7 +159,11 @@
}
}
- ((ListenableFuture<?>) Futures.allAsList(indexFutures)).get();
+ Map<Change.Id, ChangeData> changeDatas =
+ Futures.allAsList(indexFutures).get().stream()
+ // filter out null values that were returned for change deletions
+ .filter(Objects::nonNull)
+ .collect(toMap(cd -> cd.change().getId(), Function.identity()));
// Fire ref update events only after all mutations are finished, since callers may assume a
// patch set ref being created means the change was created, or a branch advancing meaning
@@ -165,7 +172,7 @@
if (!dryrun) {
for (BatchUpdate u : updates) {
- u.executePostOps();
+ u.executePostOps(changeDatas);
}
}
} catch (Exception e) {
@@ -340,6 +347,19 @@
}
}
+ private class PostUpdateContextImpl extends ContextImpl implements PostUpdateContext {
+ private final Map<Change.Id, ChangeData> changeDatas;
+
+ PostUpdateContextImpl(Map<Change.Id, ChangeData> changeDatas) {
+ this.changeDatas = changeDatas;
+ }
+
+ @Override
+ public ChangeData getChangeData(Project.NameKey projectName, Change.Id changeId) {
+ return changeDatas.computeIfAbsent(changeId, id -> changeDataFactory.create(projectName, id));
+ }
+ }
+
/** Per-change result status from {@link #executeChangeOps}. */
private enum ChangeResult {
SKIPPED,
@@ -348,6 +368,7 @@
}
private final GitRepositoryManager repoManager;
+ private final ChangeData.Factory changeDataFactory;
private final ChangeNotes.Factory changeNotesFactory;
private final ChangeUpdate.Factory changeUpdateFactory;
private final NoteDbUpdateManager.Factory updateManagerFactory;
@@ -377,6 +398,7 @@
BatchUpdate(
GitRepositoryManager repoManager,
@GerritPersonIdent PersonIdent serverIdent,
+ ChangeData.Factory changeDataFactory,
ChangeNotes.Factory changeNotesFactory,
ChangeUpdate.Factory changeUpdateFactory,
NoteDbUpdateManager.Factory updateManagerFactory,
@@ -386,6 +408,7 @@
@Assisted CurrentUser user,
@Assisted Timestamp when) {
this.repoManager = repoManager;
+ this.changeDataFactory = changeDataFactory;
this.changeNotesFactory = changeNotesFactory;
this.changeUpdateFactory = changeUpdateFactory;
this.updateManagerFactory = updateManagerFactory;
@@ -589,12 +612,12 @@
BatchUpdate.this.executed = manager.isExecuted();
}
- List<ListenableFuture<?>> startIndexFutures() {
+ List<ListenableFuture<ChangeData>> startIndexFutures() {
if (dryrun) {
return ImmutableList.of();
}
logDebug("Reindexing %d changes", results.size());
- List<ListenableFuture<?>> indexFutures = new ArrayList<>(results.size());
+ List<ListenableFuture<ChangeData>> indexFutures = new ArrayList<>(results.size());
for (Map.Entry<Change.Id, ChangeResult> e : results.entrySet()) {
Change.Id id = e.getKey();
switch (e.getValue()) {
@@ -687,8 +710,8 @@
return new ChangeContextImpl(notes);
}
- private void executePostOps() throws Exception {
- ContextImpl ctx = new ContextImpl();
+ private void executePostOps(Map<Change.Id, ChangeData> changeDatas) throws Exception {
+ PostUpdateContextImpl ctx = new PostUpdateContextImpl(changeDatas);
for (BatchUpdateOp op : ops.values()) {
try (TraceContext.TraceTimer ignored =
TraceContext.newTimer(op.getClass().getSimpleName() + "#postUpdate", Metadata.empty())) {
diff --git a/java/com/google/gerrit/server/update/PostUpdateContext.java b/java/com/google/gerrit/server/update/PostUpdateContext.java
new file mode 100644
index 0000000..b81d6d9
--- /dev/null
+++ b/java/com/google/gerrit/server/update/PostUpdateContext.java
@@ -0,0 +1,34 @@
+// Copyright (C) 2021 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.update;
+
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.server.query.change.ChangeData;
+
+/** Context for performing the {@link BatchUpdateOp#postUpdate} phase. */
+public interface PostUpdateContext extends Context {
+ /**
+ * Get the change data for the specified change.
+ *
+ * <p>If the change data has been computed previously, because the change has been indexed after
+ * an update or because this method has been invoked before, the cached change data instance is
+ * returned.
+ *
+ * @param projectName the name of the project that contains the change
+ * @param changeId the ID of the change for which the change data should be returned
+ */
+ ChangeData getChangeData(Project.NameKey projectName, Change.Id changeId);
+}
diff --git a/java/com/google/gerrit/server/update/RepoOnlyOp.java b/java/com/google/gerrit/server/update/RepoOnlyOp.java
index 7e9c47e..c3e3948 100644
--- a/java/com/google/gerrit/server/update/RepoOnlyOp.java
+++ b/java/com/google/gerrit/server/update/RepoOnlyOp.java
@@ -35,5 +35,5 @@
* @param ctx context
*/
// TODO(dborowitz): Support async operations?
- default void postUpdate(Context ctx) throws Exception {}
+ default void postUpdate(PostUpdateContext ctx) throws Exception {}
}
