Schedule reindexing when patchset ref is missing
com.google.gerrit.server.index.change.ChangeIndexer is hiding the fact
that change wasn't indexed because of missing patchset ref. Add check to
schedule the reindexing if the patchset ref is missing
Bug: Issue 16123
Change-Id: Ib86a454a57556d8573be1b70165ab3fb8b82eb87
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/forwarder/ForwardedIndexChangeHandler.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/forwarder/ForwardedIndexChangeHandler.java
index 8d41500..522a78f 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/multisite/forwarder/ForwardedIndexChangeHandler.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/forwarder/ForwardedIndexChangeHandler.java
@@ -63,17 +63,27 @@
protected void attemptToIndex(String id, Optional<ChangeIndexEvent> indexEvent, int retryCount) {
ChangeChecker checker = changeCheckerFactory.create(id);
Optional<ChangeNotes> changeNotes = checker.getChangeNotes();
- if (changeNotes.isPresent()) {
+ boolean changeIsPresent = changeNotes.isPresent();
+ boolean changeIsConsistent = checker.isChangeConsistent();
+ if (changeIsPresent && changeIsConsistent) {
reindexAndCheckIsUpToDate(id, indexEvent, checker, retryCount);
} else {
log.warn(
- "Change {} not present yet in local Git repository (event={}) after {} attempt(s)",
+ "Change {} {} in local Git repository (event={}) after {} attempt(s)",
id,
+ !changeIsPresent
+ ? "not present yet"
+ : (changeIsConsistent ? "is" : "is not") + " consistent",
indexEvent,
retryCount);
if (!rescheduleIndex(id, indexEvent, retryCount + 1)) {
log.error(
- "Change {} could not be found in the local Git repository (event={})", id, indexEvent);
+ "Change {} {} in the local Git repository (event={})",
+ id,
+ !changeIsPresent
+ ? "could not be found"
+ : (changeIsConsistent ? "was" : "was not") + " consistent",
+ indexEvent);
}
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/index/ChangeChecker.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/index/ChangeChecker.java
index 9ee59eb..3277150 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/multisite/index/ChangeChecker.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/index/ChangeChecker.java
@@ -59,4 +59,11 @@
* @throws IOException if an I/O error occurred while reading the local Change
*/
public Optional<Long> getComputedChangeTs() throws IOException;
+
+ /**
+ * Check if the local Change contains current patchset refs
+ *
+ * @return true if local change contains meta and current patchset refs
+ */
+ public boolean isChangeConsistent();
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/index/ChangeCheckerImpl.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/index/ChangeCheckerImpl.java
index 983a07b..4ef5b7f 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/multisite/index/ChangeCheckerImpl.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/index/ChangeCheckerImpl.java
@@ -31,8 +31,11 @@
import java.sql.Timestamp;
import java.util.Objects;
import java.util.Optional;
+import org.eclipse.jgit.errors.MissingObjectException;
+import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevWalk;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -145,6 +148,34 @@
}
}
+ @Override
+ public boolean isChangeConsistent() {
+ Optional<ChangeNotes> notes = getChangeNotes();
+ if (!notes.isPresent()) {
+ log.warn("Unable to compute change notes for change {}", changeId);
+ return true;
+ }
+ ObjectId currentPatchSetCommitId = notes.get().getCurrentPatchSet().commitId();
+ try (Repository repo = gitRepoMgr.openRepository(changeNotes.get().getProjectName());
+ RevWalk walk = new RevWalk(repo)) {
+ walk.parseCommit(currentPatchSetCommitId);
+ } catch (StorageException | MissingObjectException e) {
+ log.warn(
+ String.format(
+ "Consistency check failed for change %s, missing current patchset commit %s",
+ changeId, currentPatchSetCommitId.getName()),
+ e);
+ return false;
+ } catch (IOException e) {
+ log.warn(
+ String.format(
+ "Cannot check consistency for change %s, current patchset commit %s. Assuming change is consistent",
+ changeId, currentPatchSetCommitId.getName()),
+ e);
+ }
+ return true;
+ }
+
private Optional<Long> computeLastChangeTs() {
return getChangeNotes().map(notes -> getTsFromChangeAndDraftComments(notes));
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/multisite/forwarder/ForwardedIndexChangeHandlerTest.java b/src/test/java/com/googlesource/gerrit/plugins/multisite/forwarder/ForwardedIndexChangeHandlerTest.java
index 452a0d2..b827cdf 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/multisite/forwarder/ForwardedIndexChangeHandlerTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/multisite/forwarder/ForwardedIndexChangeHandlerTest.java
@@ -17,6 +17,7 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.never;
@@ -49,6 +50,7 @@
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;
import org.mockito.stubbing.Answer;
+import org.mockito.stubbing.OngoingStubbing;
@RunWith(MockitoJUnitRunner.class)
public class ForwardedIndexChangeHandlerTest {
@@ -62,6 +64,8 @@
private static final boolean THROW_STORAGE_EXCEPTION = true;
private static final boolean CHANGE_UP_TO_DATE = true;
private static final boolean CHANGE_OUTDATED = false;
+ private static final boolean CHANGE_CONSISTENT = true;
+ private static final boolean CHANGE_INCONSISTENT = false;
@Rule public ExpectedException exception = ExpectedException.none();
@Mock private ChangeIndexer indexerMock;
@@ -88,6 +92,7 @@
when(changeCheckerFactoryMock.create(any())).thenReturn(changeCheckerAbsentMock);
when(configurationMock.index()).thenReturn(index);
when(index.numStripedLocks()).thenReturn(10);
+ when(index.maxTries()).thenReturn(1);
handler =
new ForwardedIndexChangeHandler(
indexerMock, configurationMock, indexExecutorMock, ctxMock, changeCheckerFactoryMock);
@@ -95,14 +100,36 @@
@Test
public void changeIsIndexedWhenUpToDate() throws Exception {
- setupChangeAccessRelatedMocks(CHANGE_EXISTS, CHANGE_UP_TO_DATE);
+ setupChangeAccessRelatedMocks(CHANGE_EXISTS, CHANGE_UP_TO_DATE, CHANGE_CONSISTENT);
handler.index(TEST_CHANGE_ID, Operation.INDEX, Optional.empty());
verify(indexerMock, times(1)).index(any(Change.class));
}
@Test
public void changeIsStillIndexedEvenWhenOutdated() throws Exception {
- setupChangeAccessRelatedMocks(CHANGE_EXISTS, CHANGE_OUTDATED);
+ setupChangeAccessRelatedMocks(CHANGE_EXISTS, CHANGE_OUTDATED, CHANGE_CONSISTENT);
+ handler.index(
+ TEST_CHANGE_ID,
+ Operation.INDEX,
+ Optional.of(new ChangeIndexEvent("foo", 1, false, "instance-id")));
+ verify(indexerMock, times(1)).index(any(Change.class));
+ }
+
+ @Test
+ public void changeIsIndexeAtFirstRetryWhenInitiallyInconsistent() throws Exception {
+ setupChangeAccessRelatedMocks(
+ CHANGE_EXISTS,
+ DO_NOT_THROW_STORAGE_EXCEPTION,
+ CHANGE_UP_TO_DATE,
+ CHANGE_INCONSISTENT,
+ CHANGE_CONSISTENT);
+ handler.index(
+ TEST_CHANGE_ID,
+ Operation.INDEX,
+ Optional.of(new ChangeIndexEvent("foo", 1, false, "instance-id")));
+ verify(indexerMock, never()).index(any(Change.class));
+ verify(indexExecutorMock, times(1)).schedule(any(Runnable.class), anyLong(), any());
+
handler.index(
TEST_CHANGE_ID,
Operation.INDEX,
@@ -126,7 +153,8 @@
@Test
public void indexerThrowsStorageExceptionTryingToIndexChange() throws Exception {
- setupChangeAccessRelatedMocks(CHANGE_EXISTS, THROW_STORAGE_EXCEPTION, CHANGE_UP_TO_DATE);
+ setupChangeAccessRelatedMocks(
+ CHANGE_EXISTS, THROW_STORAGE_EXCEPTION, CHANGE_UP_TO_DATE, CHANGE_CONSISTENT);
assertThrows(
StorageException.class,
() -> handler.index(TEST_CHANGE_ID, Operation.INDEX, Optional.empty()));
@@ -178,11 +206,21 @@
private void setupChangeAccessRelatedMocks(boolean changeExist, boolean changeUpToDate)
throws Exception {
- setupChangeAccessRelatedMocks(changeExist, DO_NOT_THROW_STORAGE_EXCEPTION, changeUpToDate);
+ setupChangeAccessRelatedMocks(
+ changeExist, DO_NOT_THROW_STORAGE_EXCEPTION, changeUpToDate, CHANGE_CONSISTENT);
}
private void setupChangeAccessRelatedMocks(
- boolean changeExists, boolean storageException, boolean changeIsUpToDate)
+ boolean changeExist, boolean changeUpToDate, boolean changeConsistent) throws Exception {
+ setupChangeAccessRelatedMocks(
+ changeExist, DO_NOT_THROW_STORAGE_EXCEPTION, changeUpToDate, changeConsistent);
+ }
+
+ private void setupChangeAccessRelatedMocks(
+ boolean changeExists,
+ boolean storageException,
+ boolean changeIsUpToDate,
+ boolean... changeConsistentReturnValues)
throws StorageException {
if (changeExists) {
when(changeCheckerFactoryMock.create(TEST_CHANGE_ID)).thenReturn(changeCheckerPresentMock);
@@ -193,5 +231,11 @@
}
when(changeCheckerPresentMock.isUpToDate(any())).thenReturn(changeIsUpToDate);
+
+ OngoingStubbing<Boolean> changeConsistentCall =
+ when(changeCheckerPresentMock.isChangeConsistent());
+ for (boolean changeConsistent : changeConsistentReturnValues) {
+ changeConsistentCall = changeConsistentCall.thenReturn(changeConsistent);
+ }
}
}
