Don't delete missing changes from secondary index
This is a partial revert of 903be04b02.
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
This commit is contained in:
@@ -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));
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user