Check meta ref sha1 during the index propagation
For NoteDb check if meta ref is up to date before indexing the change
during the index propagation. Because of the NFS cache NoteDb change can
be invisible for some nodes during the indexing. Checking sha1 of the
meta ref, so that plugin can ensure the indexing operation has the same
view of the change across the nodes.
Bug: Issue 13847
Change-Id: Ibec9ee0a64cda7fa50b6356ad4a766673053d432
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/IndexEvent.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/IndexEvent.java
index 037c1c6..71cf044 100644
--- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/IndexEvent.java
+++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/IndexEvent.java
@@ -21,10 +21,14 @@
public class IndexEvent {
public long eventCreatedOn = System.currentTimeMillis() / 1000;
public String targetSha;
+ public String metaSha;
@Override
public String toString() {
- return "IndexEvent@" + format(eventCreatedOn) + ((targetSha != null) ? "/" + targetSha : "");
+ return "IndexEvent@"
+ + format(eventCreatedOn)
+ + ((targetSha != null) ? "/target:" + targetSha : "")
+ + ((metaSha != null) ? "/meta:" + metaSha : "");
}
public static String format(long eventTs) {
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeCheckerImpl.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeCheckerImpl.java
index 4cdf431..bd8493c 100644
--- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeCheckerImpl.java
+++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeCheckerImpl.java
@@ -18,11 +18,13 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Comment;
+import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.change.ChangeFinder;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.util.ManualRequestContext;
import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.gwtorm.server.OrmException;
@@ -68,14 +70,16 @@
@Override
public Optional<IndexEvent> newIndexEvent() throws IOException, OrmException {
- return getComputedChangeTs()
- .map(
- ts -> {
- IndexEvent event = new IndexEvent();
- event.eventCreatedOn = ts;
- event.targetSha = getBranchTargetSha();
- return event;
- });
+ Optional<Long> changeTs = getComputedChangeTs();
+ if (changeTs.isPresent()) {
+ IndexEvent event = new IndexEvent();
+ event.eventCreatedOn = changeTs.get();
+ event.targetSha = getBranchTargetSha();
+ event.metaSha = getMetaSha();
+ return Optional.of(event);
+ }
+
+ return Optional.empty();
}
@Override
@@ -87,26 +91,30 @@
}
@Override
- public boolean isChangeUpToDate(Optional<IndexEvent> indexEvent)
+ public boolean isChangeUpToDate(Optional<IndexEvent> indexEventOption)
throws IOException, OrmException {
getComputedChangeTs();
- log.atFine().log("Checking change %s against index event %s", this, indexEvent);
+ log.atFine().log("Checking change %s against index event %s", this, indexEventOption);
if (!computedChangeTs.isPresent()) {
log.atWarning().log("Unable to compute last updated ts for change %s", changeId);
return false;
}
+ try {
+ if (indexEventOption.isPresent()) {
+ IndexEvent indexEvent = indexEventOption.get();
+ return (computedChangeTs.get() > indexEvent.eventCreatedOn)
+ || (computedChangeTs.get() == indexEvent.eventCreatedOn)
+ && (Objects.isNull(indexEvent.targetSha)
+ || Objects.equals(getBranchTargetSha(), indexEvent.targetSha))
+ && (Objects.isNull(indexEvent.metaSha)
+ || Objects.equals(getMetaSha(), indexEvent.metaSha));
+ }
+ return true;
- if (indexEvent.isPresent() && indexEvent.get().targetSha == null) {
- return indexEvent.map(e -> (computedChangeTs.get() >= e.eventCreatedOn)).orElse(true);
+ } catch (IOException ex) {
+ log.atWarning().log("Unable to read meta sha for change %s", changeId);
+ return false;
}
-
- return indexEvent
- .map(
- e ->
- (computedChangeTs.get() > e.eventCreatedOn)
- || (computedChangeTs.get() == e.eventCreatedOn)
- && (Objects.equals(getBranchTargetSha(), e.targetSha)))
- .orElse(true);
}
@Override
@@ -124,8 +132,10 @@
+ changeId
+ "@"
+ getComputedChangeTs().map(IndexEvent::format)
- + "/"
- + getBranchTargetSha();
+ + "/target:"
+ + getBranchTargetSha()
+ + "/meta:"
+ + getMetaSha();
} catch (IOException | OrmException e) {
log.atSevere().withCause(e).log("Unable to render change %s", changeId);
return "change-id=" + changeId;
@@ -148,6 +158,22 @@
}
}
+ private String getMetaSha() throws IOException {
+ if (PrimaryStorage.NOTE_DB.equals(PrimaryStorage.of(changeNotes.get().getChange()))) {
+ try (Repository repo = gitRepoMgr.openRepository(changeNotes.get().getProjectName())) {
+ String refName = RefNames.changeMetaRef(changeNotes.get().getChange().getId());
+ Ref ref = repo.exactRef(refName);
+ if (ref == null) {
+ throw new IOException(
+ String.format("Unable to find meta ref %s for change %s", refName, changeId));
+ }
+ return ref.getTarget().getObjectId().getName();
+ }
+ }
+
+ return null;
+ }
+
private Optional<Long> computeLastChangeTs() throws OrmException {
try (ReviewDb db = changeDb.open()) {
return getChangeNotes().map(notes -> getTsFromChangeAndDraftComments(db, notes));
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeCheckerNoteDbIT.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeCheckerNoteDbIT.java
new file mode 100644
index 0000000..b691034
--- /dev/null
+++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeCheckerNoteDbIT.java
@@ -0,0 +1,135 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.ericsson.gerrit.plugins.highavailability.index;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.ericsson.gerrit.plugins.highavailability.forwarder.IndexEvent;
+import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit.Result;
+import com.google.gerrit.acceptance.TestPlugin;
+import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.testing.NoteDbMode;
+import com.google.gwtorm.server.OrmException;
+import java.io.IOException;
+import java.util.Optional;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+@TestPlugin(
+ name = "high-availability",
+ sysModule = "com.ericsson.gerrit.plugins.highavailability.Module",
+ httpModule = "com.ericsson.gerrit.plugins.highavailability.HttpModule")
+public class ChangeCheckerNoteDbIT extends LightweightPluginDaemonTest {
+
+ ChangeCheckerImpl.Factory changeCheckerFactory;
+
+ @BeforeClass
+ public static void setupTestSuite() {
+ System.setProperty("gerrit.notedb", NoteDbMode.ON.name());
+ }
+
+ @Override
+ public void setUpTestPlugin() throws Exception {
+ super.setUpTestPlugin();
+ changeCheckerFactory = plugin.getSysInjector().getInstance(ChangeCheckerImpl.Factory.class);
+ }
+
+ @Test
+ public void shouldPopulateMetaShaWhenNoteDb() throws Exception {
+ Result change = createChange();
+ ChangeChecker changeChecker = changeCheckerFactory.create(change.getChangeId());
+ Optional<IndexEvent> eventOption = changeChecker.newIndexEvent();
+
+ assertThat(eventOption.isPresent()).isTrue();
+ IndexEvent event = eventOption.get();
+ assertThat(event.metaSha).isNotNull();
+ assertThat(event.metaSha).isEqualTo(readMetaSha(change));
+ }
+
+ @Test
+ public void shouldReturnIsUpToDateTrueWhenEventContainsCorrectMetaAndTargetSha()
+ throws Exception {
+ Result change = createChange();
+ ChangeChecker changeChecker = changeCheckerFactory.create(change.getChangeId());
+ Optional<IndexEvent> event = changeChecker.newIndexEvent();
+
+ assertThat(changeChecker.isChangeUpToDate(event)).isTrue();
+ }
+
+ @Test
+ public void shouldReturnIsUpToDateTrueWhenTargetShaIsNull() throws Exception {
+ Result change = createChange();
+ ChangeChecker changeChecker = changeCheckerFactory.create(change.getChangeId());
+ Optional<IndexEvent> event =
+ changeChecker
+ .newIndexEvent()
+ .map(
+ e -> {
+ e.targetSha = null;
+ return e;
+ });
+
+ assertThat(changeChecker.isChangeUpToDate(event)).isTrue();
+ }
+
+ @Test
+ public void shouldReturnFalseWhenMetaShaIsNotUpToDate() throws Exception {
+ String testMetaRefSha = "6212efebe6e8b9f439a8ad013243e602afab7441";
+ Result change = createChange();
+ ChangeChecker changeChecker = changeCheckerFactory.create(change.getChangeId());
+ Optional<IndexEvent> event =
+ changeChecker
+ .newIndexEvent()
+ .map(
+ e -> {
+ e.metaSha = testMetaRefSha;
+ return e;
+ });
+
+ assertThat(changeChecker.isChangeUpToDate(event)).isFalse();
+ }
+
+ @Test
+ public void shouldReturnFalseWhenTargetShaIsNotUpToDate() throws Exception {
+ String testTargetRefSha = "abed47baf2818a86b68cf712073a748a6b5b293e";
+ Result change = createChange();
+ ChangeChecker changeChecker = changeCheckerFactory.create(change.getChangeId());
+ Optional<IndexEvent> event =
+ changeChecker
+ .newIndexEvent()
+ .map(
+ e -> {
+ e.targetSha = testTargetRefSha;
+ return e;
+ });
+
+ assertThat(changeChecker.isChangeUpToDate(event)).isFalse();
+ }
+
+ private String readMetaSha(Result change) throws IOException, OrmException {
+ try (Repository repo = repoManager.openRepository(change.getChange().change().getProject())) {
+ String refName = RefNames.changeMetaRef(change.getChange().getId());
+ Ref ref = repo.exactRef(refName);
+ if (ref == null) {
+ return null;
+ }
+
+ return ref.getTarget().getObjectId().getName();
+ }
+ }
+}
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeCheckerReviewDbIT.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeCheckerReviewDbIT.java
new file mode 100644
index 0000000..a6cb449
--- /dev/null
+++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeCheckerReviewDbIT.java
@@ -0,0 +1,116 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.ericsson.gerrit.plugins.highavailability.index;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.ericsson.gerrit.plugins.highavailability.forwarder.IndexEvent;
+import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit.Result;
+import com.google.gerrit.acceptance.TestPlugin;
+import com.google.gerrit.testing.NoteDbMode;
+import java.util.Optional;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+@TestPlugin(
+ name = "high-availability",
+ sysModule = "com.ericsson.gerrit.plugins.highavailability.Module",
+ httpModule = "com.ericsson.gerrit.plugins.highavailability.HttpModule")
+public class ChangeCheckerReviewDbIT extends LightweightPluginDaemonTest {
+
+ ChangeCheckerImpl.Factory changeCheckerFactory;
+
+ @BeforeClass
+ public static void setupTestSuite() {
+ System.setProperty("gerrit.notedb", NoteDbMode.OFF.name());
+ }
+
+ @Override
+ public void setUpTestPlugin() throws Exception {
+ super.setUpTestPlugin();
+ changeCheckerFactory = plugin.getSysInjector().getInstance(ChangeCheckerImpl.Factory.class);
+ }
+
+ @Test
+ public void shouldNotPopulateMetaShaWhenReviewDb() throws Exception {
+ Result change = createChange();
+ ChangeChecker changeChecker = changeCheckerFactory.create(change.getChangeId());
+ Optional<IndexEvent> eventOption = changeChecker.newIndexEvent();
+
+ assertThat(eventOption.isPresent()).isTrue();
+ IndexEvent event = eventOption.get();
+ assertThat(event.metaSha).isNull();
+ }
+
+ @Test
+ public void shouldReturnIsUpToDateTrueWhenEventContainsCorrectMetaAndTargetSha()
+ throws Exception {
+ Result change = createChange();
+ ChangeChecker changeChecker = changeCheckerFactory.create(change.getChangeId());
+ Optional<IndexEvent> event = changeChecker.newIndexEvent();
+
+ assertThat(changeChecker.isChangeUpToDate(event)).isTrue();
+ }
+
+ @Test
+ public void shouldReturnIsUpToDateTrueWhenMetaShaIsNull() throws Exception {
+ Result change = createChange();
+ ChangeChecker changeChecker = changeCheckerFactory.create(change.getChangeId());
+ Optional<IndexEvent> event =
+ changeChecker
+ .newIndexEvent()
+ .map(
+ e -> {
+ e.metaSha = null;
+ return e;
+ });
+
+ assertThat(changeChecker.isChangeUpToDate(event)).isTrue();
+ }
+
+ @Test
+ public void shouldReturnIsUpToDateTrueWhenTargetShaIsNull() throws Exception {
+ Result change = createChange();
+ ChangeChecker changeChecker = changeCheckerFactory.create(change.getChangeId());
+ Optional<IndexEvent> event =
+ changeChecker
+ .newIndexEvent()
+ .map(
+ e -> {
+ e.targetSha = null;
+ return e;
+ });
+
+ assertThat(changeChecker.isChangeUpToDate(event)).isTrue();
+ }
+
+ @Test
+ public void shouldReturnFalseWhenTargetShaIsNotUpToDate() throws Exception {
+ String testTargetRefSha = "abed47baf2818a86b68cf712073a748a6b5b293e";
+ Result change = createChange();
+ ChangeChecker changeChecker = changeCheckerFactory.create(change.getChangeId());
+ Optional<IndexEvent> event =
+ changeChecker
+ .newIndexEvent()
+ .map(
+ e -> {
+ e.targetSha = testTargetRefSha;
+ return e;
+ });
+
+ assertThat(changeChecker.isChangeUpToDate(event)).isFalse();
+ }
+}
