Simplify ChangeIndex by removing delete(ChangeData)

Since indexing is generally done after any modification operations,
using a deleted ChangeData when indexing a deleted change is just
asking for trouble; most ChangeData methods (reasonably) assume that
the change exists. Instead, just use the form of the method that
passes in the ID.

Change the method that takes an integer ID to take a Change.Id object.
In almost all cases we already have one of these handy, and this is
more consistent with public interfaces elsewhere in Gerrit.

A fortunate side effect is that async deletes should happen slightly
faster, since we no longer need to open a DB connection and produce a
ChangeData in the delete codepath.

Change-Id: I353522b52188de11353ca26a7ba90a2ca462e404

gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java

@@ -319,20 +319,7 @@ public class LuceneChangeIndex implements ChangeIndex {
 
   @SuppressWarnings("unchecked")
   @Override
-  public void delete(ChangeData cd) throws IOException {
-    Term id = QueryBuilder.idTerm(cd);
-    try {
-      Futures.allAsList(
-          openIndex.delete(id),
-          closedIndex.delete(id)).get();
-    } catch (ExecutionException | InterruptedException e) {
-      throw new IOException(e);
-    }
-  }
-
-  @SuppressWarnings("unchecked")
-  @Override
-  public void delete(int id) throws IOException {
+  public void delete(Change.Id id) throws IOException {
     Term idTerm = QueryBuilder.idTerm(id);
     try {
       Futures.allAsList(

gerrit-lucene/src/main/java/com/google/gerrit/lucene/QueryBuilder.java

@@ -19,6 +19,7 @@ import static org.apache.lucene.search.BooleanClause.Occur.MUST_NOT;
 import static org.apache.lucene.search.BooleanClause.Occur.SHOULD;
 
 import com.google.common.collect.Lists;
+import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.server.index.ChangeField;
 import com.google.gerrit.server.index.FieldType;
 import com.google.gerrit.server.index.IndexPredicate;
@@ -56,8 +57,8 @@ public class QueryBuilder {
     return intTerm(ID_FIELD, cd.getId().get());
   }
 
-  public static Term idTerm(int id) {
-    return intTerm(ID_FIELD, id);
+  public static Term idTerm(Change.Id id) {
+    return intTerm(ID_FIELD, id.get());
   }
 
   private final Schema<ChangeData> schema;

gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java

@@ -466,7 +466,7 @@ public class ChangeUtil {
     db.changeMessages().delete(db.changeMessages().byChange(changeId));
     db.starredChanges().delete(db.starredChanges().byChange(changeId));
     db.changes().delete(Collections.singleton(change));
-    indexer.delete(db, change);
+    indexer.delete(change.getId());
   }
 
   public void deleteOnlyDraftPatchSet(PatchSet patch, Change change)

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java

@@ -86,7 +86,7 @@ public class ChangesCollection implements
       Integer changeId = Ints.tryParse(id.get());
       if (changeId != null) {
         try {
-          changeIndexer.delete(changeId);
+          changeIndexer.delete(new Change.Id(changeId));
         } catch (IOException e) {
           throw new ResourceNotFoundException(id.get(), e);
         }

gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeIndex.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.index;
 
+import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.server.query.Predicate;
 import com.google.gerrit.server.query.QueryParseException;
 import com.google.gerrit.server.query.change.ChangeData;
@@ -52,23 +53,14 @@ public interface ChangeIndex {
    */
   public void replace(ChangeData cd) throws IOException;
 
-  /**
-   * Delete a change document from the index.
-   *
-   * @param cd change document
-   *
-   * @throws IOException
-   */
-  public void delete(ChangeData cd) throws IOException;
-
   /**
    * Delete a change document from the index by id.
    *
-   * @param id change document id
+   * @param id change id
    *
    * @throws IOException
    */
-  public void delete(int id) throws IOException;
+  public void delete(Change.Id id) throws IOException;
 
   /**
    * Delete all change documents from the index.

gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeIndexer.java

@@ -118,7 +118,7 @@ public class ChangeIndexer {
    */
   public CheckedFuture<?, IOException> indexAsync(Change.Id id) {
     return executor != null
-        ? submit(new Task(id, false))
+        ? submit(new IndexTask(id))
         : Futures.<Object, IOException> immediateCheckedFuture(null);
   }
 
@@ -151,40 +151,17 @@ public class ChangeIndexer {
    */
   public CheckedFuture<?, IOException> deleteAsync(Change.Id id) {
     return executor != null
-        ? submit(new Task(id, true))
+        ? submit(new DeleteTask(id))
         : Futures.<Object, IOException> immediateCheckedFuture(null);
   }
 
   /**
    * Synchronously delete a change.
    *
-   * @param cd change to delete.
+   * @param id change ID to delete.
    */
-  public void delete(ChangeData cd) throws IOException {
-    for (ChangeIndex i : getWriteIndexes()) {
-      i.delete(cd);
-    }
-  }
-
-  /**
-   * Synchronously delete a change by id.
-   *
-   * @param id change to delete.
-   */
-  public void delete(int id) throws IOException {
-    for (ChangeIndex i : getWriteIndexes()) {
-      i.delete(id);
-    }
-  }
-
-  /**
-   * Synchronously delete a change.
-   *
-   * @param change change to delete.
-   * @param db review database.
-   */
-  public void delete(ReviewDb db, Change change) throws IOException {
-    delete(changeDataFactory.create(db, change));
+  public void delete(Change.Id id) throws IOException {
+    new DeleteTask(id).call();
   }
 
   private Collection<ChangeIndex> getWriteIndexes() {
@@ -197,13 +174,11 @@ public class ChangeIndexer {
     return Futures.makeChecked(executor.submit(task), MAPPER);
   }
 
-  private class Task implements Callable<Void> {
+  private class IndexTask implements Callable<Void> {
     private final Change.Id id;
-    private final boolean delete;
 
-    private Task(Change.Id id, boolean delete) {
+    private IndexTask(Change.Id id) {
       this.id = id;
-      this.delete = delete;
     }
 
     @Override
@@ -238,15 +213,9 @@ public class ChangeIndexer {
         try {
           ChangeData cd = changeDataFactory.create(
               newCtx.getReviewDbProvider().get(), id);
-          if (delete) {
-            for (ChangeIndex i : getWriteIndexes()) {
-              i.delete(cd);
-            }
-          } else {
           for (ChangeIndex i : getWriteIndexes()) {
             i.replace(cd);
           }
-          }
           return null;
         } finally  {
           context.setContext(oldCtx);
@@ -266,4 +235,23 @@ public class ChangeIndexer {
       return "index-change-" + id.get();
     }
   }
+
+  private class DeleteTask implements Callable<Void> {
+    private final Change.Id id;
+
+    private DeleteTask(Change.Id id) {
+      this.id = id;
+    }
+
+    @Override
+    public Void call() throws IOException {
+      // Don't bother setting a RequestContext to provide the DB.
+      // Implementations should not need to access the DB in order to delete a
+      // change ID.
+      for (ChangeIndex i : getWriteIndexes()) {
+        i.delete(id);
+      }
+      return null;
+    }
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/index/DummyIndex.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.index;
 
+import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.server.query.Predicate;
 import com.google.gerrit.server.query.QueryParseException;
 import com.google.gerrit.server.query.change.ChangeData;
@@ -22,7 +23,6 @@ import com.google.gerrit.server.query.change.ChangeDataSource;
 import java.io.IOException;
 
 public class DummyIndex implements ChangeIndex {
-
   @Override
   public Schema<ChangeData> getSchema() {
     throw new UnsupportedOperationException();
@@ -37,7 +37,7 @@ public class DummyIndex implements ChangeIndex {
   }
 
   @Override
-  public void delete(ChangeData cd) throws IOException {
+  public void delete(Change.Id id) throws IOException {
   }
 
   @Override
@@ -53,8 +53,4 @@ public class DummyIndex implements ChangeIndex {
   @Override
   public void markReady(boolean ready) throws IOException {
   }
-
-  @Override
-  public void delete(int id) throws IOException {
-  }
 }

gerrit-server/src/test/java/com/google/gerrit/server/index/FakeIndex.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.index;
 
 import com.google.common.collect.ImmutableList;
+import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.server.query.Predicate;
 import com.google.gerrit.server.query.QueryParseException;
 import com.google.gerrit.server.query.change.ChangeData;
@@ -22,8 +23,6 @@ import com.google.gerrit.server.query.change.ChangeDataSource;
 import com.google.gwtorm.server.OrmException;
 import com.google.gwtorm.server.ResultSet;
 
-import java.io.IOException;
-
 class FakeIndex implements ChangeIndex {
   static Schema<ChangeData> V1 = new Schema<>(1, false,
     ImmutableList.<FieldDef<ChangeData, ?>> of(
@@ -75,7 +74,7 @@ class FakeIndex implements ChangeIndex {
   }
 
   @Override
-  public void delete(ChangeData cd) {
+  public void delete(Change.Id id) {
     throw new UnsupportedOperationException();
   }
 
@@ -103,8 +102,4 @@ class FakeIndex implements ChangeIndex {
   public void markReady(boolean ready) {
     throw new UnsupportedOperationException();
   }
-
-  @Override
-  public void delete(int id) throws IOException {
-  }
 }

gerrit-solr/src/main/java/com/google/gerrit/solr/SolrChangeIndex.java

@@ -160,22 +160,8 @@ class SolrChangeIndex implements ChangeIndex, LifecycleListener {
   }
 
   @Override
-  public void delete(ChangeData cd) throws IOException {
-    String id = cd.getId().toString();
-    try {
-      if (cd.change().getStatus().isOpen()) {
-        delete(id, openIndex);
-      } else {
-        delete(id, closedIndex);
-      }
-    } catch (OrmException e) {
-      throw new IOException(e);
-    }
-  }
-
-  @Override
-  public void delete(int id) throws IOException {
-    String idString = Integer.toString(id);
+  public void delete(Change.Id id) throws IOException {
+    String idString = Integer.toString(id.get());
     delete(idString, openIndex);
     delete(idString, closedIndex);
   }