From 5a5bd9ae66a70b3a4a02beca942f833a32b59cd3 Mon Sep 17 00:00:00 2001
From: Shawn Pearce <sop@google.com>
Date: Tue, 8 Mar 2016 10:42:59 -0800
Subject: [PATCH] Don't delete missing changes from secondary index

This is a partial revert of 903be04b024755c73156f3c0d7e66a856512dfb4.

With the NoteDb migration ChangeFinder is querying the secondary
index to map from Change.Id to verify a change exists and locate
the project the change's metadata is stored in. If a change is not
found in the secondary index there is no need to delete the change,
as it does not exist in the secondary index.

Some secondary index implementations (notably the one at Google
behind gerrit-review) implement delete by appending a negative
assertion into the index, stating the document does not exist.
Each failed lookup appends more negative assertions into the index,
which can cause problems when there is replication delay between
multi-master instances.

Stop sending delete calls to the secondary index when ChangeFinder
got no results. Leave the index alone. The parse() methods are
typically invoked by GET calls, which should not be modifying
server state.

Change-Id: Iafd89324f9945dd827c37fec2d2ac54b89fc4e4a
---
 .../server/change/ChangesCollection.java      | 33 +++----------------
 .../gerrit/server/index/ChangeIndexer.java    |  9 -----
 2 files changed, 5 insertions(+), 37 deletions(-)

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java
index af3c576e2b..058c7d532a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java
@@ -14,7 +14,6 @@
 
 package com.google.gerrit.server.change;
 
-import com.google.common.primitives.Ints;
 import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.extensions.restapi.AcceptsPost;
 import com.google.gerrit.extensions.restapi.IdString;
@@ -27,7 +26,6 @@ import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ChangeFinder;
 import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.index.ChangeIndexer;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.query.change.QueryChanges;
 import com.google.gwtorm.server.OrmException;
@@ -35,7 +33,6 @@ import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
 
-import java.io.IOException;
 import java.util.List;
 
 @Singleton
@@ -48,7 +45,6 @@ public class ChangesCollection implements
   private final DynamicMap<RestView<ChangeResource>> views;
   private final ChangeFinder changeFinder;
   private final CreateChange createChange;
-  private final ChangeIndexer changeIndexer;
 
   @Inject
   ChangesCollection(
@@ -57,15 +53,13 @@ public class ChangesCollection implements
       Provider<QueryChanges> queryFactory,
       DynamicMap<RestView<ChangeResource>> views,
       ChangeFinder changeFinder,
-      CreateChange createChange,
-      ChangeIndexer changeIndexer) {
+      CreateChange createChange) {
     this.db = db;
     this.user = user;
     this.queryFactory = queryFactory;
     this.views = views;
     this.changeFinder = changeFinder;
     this.createChange = createChange;
-    this.changeIndexer = changeIndexer;
   }
 
   @Override
@@ -81,22 +75,10 @@ public class ChangesCollection implements
   @Override
   public ChangeResource parse(TopLevelResource root, IdString id)
       throws ResourceNotFoundException, OrmException {
-    List<ChangeControl> ctls =
-        changeFinder.find(id.encoded(), user.get());
-    if (ctls.isEmpty()) {
-      Integer changeId = Ints.tryParse(id.get());
-      if (changeId != null) {
-        try {
-          changeIndexer.delete(new Change.Id(changeId));
-        } catch (IOException e) {
-          throw new ResourceNotFoundException(id.get(), e);
-        }
-      }
-    }
+    List<ChangeControl> ctls = changeFinder.find(id.encoded(), user.get());
     if (ctls.isEmpty()) {
       throw new ResourceNotFoundException(id);
-    }
-    if (ctls.size() != 1) {
+    } else if (ctls.size() != 1) {
       throw new ResourceNotFoundException("Multiple changes found for " + id);
     }
 
@@ -111,16 +93,11 @@ public class ChangesCollection implements
       throws ResourceNotFoundException, OrmException {
     List<ChangeControl> ctls = changeFinder.find(id, user.get());
     if (ctls.isEmpty()) {
-      try {
-        changeIndexer.delete(id);
-      } catch (IOException e) {
-        throw new ResourceNotFoundException(toIdString(id).get(), e);
-      }
       throw new ResourceNotFoundException(toIdString(id));
-    }
-    if (ctls.size() != 1) {
+    } else if (ctls.size() != 1) {
       throw new ResourceNotFoundException("Multiple changes found for " + id);
     }
+
     ChangeControl ctl = ctls.get(0);
     if (!ctl.isVisible(db.get())) {
       throw new ResourceNotFoundException(toIdString(id));
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeIndexer.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeIndexer.java
index a0860c7afc..c96ed657d1 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeIndexer.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeIndexer.java
@@ -184,15 +184,6 @@ public class ChangeIndexer {
         : Futures.<Object, IOException> immediateCheckedFuture(null);
   }
 
-  /**
-   * Synchronously delete a change.
-   *
-   * @param id change ID to delete.
-   */
-  public void delete(Change.Id id) throws IOException {
-    new DeleteTask(id).call();
-  }
-
   private Collection<ChangeIndex> getWriteIndexes() {
     return indexes != null
         ? indexes.getWriteIndexes()