From 903be04b024755c73156f3c0d7e66a856512dfb4 Mon Sep 17 00:00:00 2001 From: Olga Grinberg Date: Wed, 22 Oct 2014 10:05:15 -0400 Subject: [PATCH 1/2] Delete a change from the index when it is not in the DB If for some reason the secondary index is out of date, i.e. the change was deleted from the database but wasn't deleted from the secondary index, it was impossible to re-index (remove) that change. Add logic to automatically remove the change from the secondary index if this change is requested through the Change REST API endpoint and doesn't exist in the database. If a user click on search result from a stale change, he will a get a 404 page and the change will be removed from the index. To fix the problem without opening the change page, run a command like: curl --user -X POST http://:/a/changes//index Issue: 2996 Change-Id: I1db5373e31585e99c5f45e05274d86d69b4f24e6 --- .../gerrit/lucene/LuceneChangeIndex.java | 13 ++++++++++ .../google/gerrit/lucene/QueryBuilder.java | 4 ++++ .../server/change/ChangesCollection.java | 20 +++++++++++++++- .../gerrit/server/index/ChangeIndex.java | 9 +++++++ .../gerrit/server/index/ChangeIndexer.java | 11 +++++++++ .../gerrit/server/index/DummyIndex.java | 4 ++++ .../google/gerrit/server/index/FakeIndex.java | 6 +++++ .../google/gerrit/solr/SolrChangeIndex.java | 24 +++++++++++++++---- 8 files changed, 85 insertions(+), 6 deletions(-) diff --git a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java index c396ae6b7b..d03e202538 100644 --- a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java +++ b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java @@ -322,6 +322,19 @@ public class LuceneChangeIndex implements ChangeIndex { } } + @SuppressWarnings("unchecked") + @Override + public void delete(int id) throws IOException { + Term idTerm = QueryBuilder.idTerm(id); + try { + Futures.allAsList( + openIndex.delete(idTerm), + closedIndex.delete(idTerm)).get(); + } catch (ExecutionException | InterruptedException e) { + throw new IOException(e); + } + } + @Override public void deleteAll() throws IOException { openIndex.deleteAll(); diff --git a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/QueryBuilder.java b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/QueryBuilder.java index 3221b8ae9f..392d1645ca 100644 --- a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/QueryBuilder.java +++ b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/QueryBuilder.java @@ -55,6 +55,10 @@ public class QueryBuilder { return intTerm(ID_FIELD, cd.getId().get()); } + public static Term idTerm(int id) { + return intTerm(ID_FIELD, id); + } + private final Schema schema; private final org.apache.lucene.util.QueryBuilder queryBuilder; 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 f7038549df..454ec00cb9 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 @@ -15,6 +15,7 @@ package com.google.gerrit.server.change; import com.google.common.collect.ImmutableList; +import com.google.common.primitives.Ints; import com.google.gerrit.extensions.registration.DynamicMap; import com.google.gerrit.extensions.restapi.IdString; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; @@ -23,7 +24,9 @@ import com.google.gerrit.extensions.restapi.RestView; import com.google.gerrit.extensions.restapi.TopLevelResource; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.ChangeUtil; 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.project.NoSuchChangeException; import com.google.gerrit.server.query.change.QueryChanges; @@ -31,6 +34,7 @@ import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; +import java.io.IOException; import java.util.Collections; import java.util.List; @@ -41,6 +45,7 @@ public class ChangesCollection implements private final ChangeControl.GenericFactory changeControlFactory; private final Provider queryFactory; private final DynamicMap> views; + private final ChangeIndexer changeIndexer; @Inject ChangesCollection( @@ -48,12 +53,15 @@ public class ChangesCollection implements Provider user, ChangeControl.GenericFactory changeControlFactory, Provider queryFactory, - DynamicMap> views) { + DynamicMap> views, + ChangeUtil changeUtil, + ChangeIndexer changeIndexer) { this.db = dbProvider; this.user = user; this.changeControlFactory = changeControlFactory; this.queryFactory = queryFactory; this.views = views; + this.changeIndexer = changeIndexer; } @Override @@ -70,6 +78,16 @@ public class ChangesCollection implements public ChangeResource parse(TopLevelResource root, IdString id) throws ResourceNotFoundException, OrmException { List changes = findChanges(id.encoded()); + if (changes.isEmpty()) { + Integer changeId = Ints.tryParse(id.get()); + if (changeId != null) { + try { + changeIndexer.delete(changeId); + } catch (IOException e) { + throw new ResourceNotFoundException(id.get(), e); + } + } + } if (changes.size() != 1) { throw new ResourceNotFoundException(id); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeIndex.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeIndex.java index a710a108a3..167367898a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeIndex.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeIndex.java @@ -72,6 +72,15 @@ public interface ChangeIndex { */ public void delete(ChangeData cd) throws IOException; + /** + * Delete a change document from the index by id. + * + * @param id change document id + * + * @throws IOException + */ + public void delete(int id) throws IOException; + /** * Delete all change documents from the index. * 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 437f55927d..72cb88c90a 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 @@ -165,6 +165,17 @@ public class ChangeIndexer { } } + /** + * 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. * diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/DummyIndex.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/DummyIndex.java index 9f1c353876..ff554ff0ef 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/DummyIndex.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/DummyIndex.java @@ -57,4 +57,8 @@ public class DummyIndex implements ChangeIndex { @Override public void markReady(boolean ready) throws IOException { } + + @Override + public void delete(int id) throws IOException { + } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/index/FakeIndex.java b/gerrit-server/src/test/java/com/google/gerrit/server/index/FakeIndex.java index 4db3b27764..a14bbf13f7 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/index/FakeIndex.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/index/FakeIndex.java @@ -22,6 +22,8 @@ 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 V1 = new Schema(1, false, ImmutableList.> of( @@ -106,4 +108,8 @@ class FakeIndex implements ChangeIndex { public void markReady(boolean ready) { throw new UnsupportedOperationException(); } + + @Override + public void delete(int id) throws IOException { + } } diff --git a/gerrit-solr/src/main/java/com/google/gerrit/solr/SolrChangeIndex.java b/gerrit-solr/src/main/java/com/google/gerrit/solr/SolrChangeIndex.java index 8c8b0071ac..830db27106 100644 --- a/gerrit-solr/src/main/java/com/google/gerrit/solr/SolrChangeIndex.java +++ b/gerrit-solr/src/main/java/com/google/gerrit/solr/SolrChangeIndex.java @@ -189,13 +189,27 @@ class SolrChangeIndex implements ChangeIndex, LifecycleListener { String id = cd.getId().toString(); try { if (cd.change().getStatus().isOpen()) { - openIndex.deleteById(id); - commit(openIndex); + delete(id, openIndex); } else { - closedIndex.deleteById(id); - commit(closedIndex); + delete(id, closedIndex); } - } catch (OrmException | SolrServerException e) { + } catch (OrmException e) { + throw new IOException(e); + } + } + + @Override + public void delete(int id) throws IOException { + String idString = Integer.toString(id); + delete(idString, openIndex); + delete(idString, closedIndex); + } + + private void delete(String id, CloudSolrServer index) throws IOException { + try { + index.deleteById(id); + commit(index); + } catch (SolrServerException e) { throw new IOException(e); } } From 1faef9018d98d5cc2e9dbf020596aa72498c495e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Wed, 12 Nov 2014 12:49:13 -0500 Subject: [PATCH 2/2] Remove uneeded dependency in ChangesCollection Change I1db5373e31585e99c5f45e05274d86d69b4f24e6 added an unneeded dependency in ChangesCollection. Change-Id: I2f8609416518495624eda9e08309a6e14863fbe8 --- .../java/com/google/gerrit/server/change/ChangesCollection.java | 2 -- 1 file changed, 2 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 454ec00cb9..cfec2e595c 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 @@ -24,7 +24,6 @@ import com.google.gerrit.extensions.restapi.RestView; import com.google.gerrit.extensions.restapi.TopLevelResource; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.server.ReviewDb; -import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.index.ChangeIndexer; import com.google.gerrit.server.project.ChangeControl; @@ -54,7 +53,6 @@ public class ChangesCollection implements ChangeControl.GenericFactory changeControlFactory, Provider queryFactory, DynamicMap> views, - ChangeUtil changeUtil, ChangeIndexer changeIndexer) { this.db = dbProvider; this.user = user;