Merge branch 'stable-3.7' into stable-3.8
* stable-3.7:
Add acceptance test for change up-to-date checker
Indexing retries: lower normal condition logs to debug
Check for existence of change's target SHA1 for reindexing
Change-Id: Iaf0ea1830e70b0ed61745c1a84b4ccb7d7ba2f7d
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/forwarder/ForwardedIndexingHandlerWithRetries.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/forwarder/ForwardedIndexingHandlerWithRetries.java
index f3d4e70..d135e39 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/multisite/forwarder/ForwardedIndexingHandlerWithRetries.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/forwarder/ForwardedIndexingHandlerWithRetries.java
@@ -64,7 +64,7 @@
protected boolean rescheduleIndex(T id) {
IndexingRetry retry = indexingRetryTaskMap.get(id);
if (retry == null) {
- log.info(
+ log.debug(
"{} {} successfully indexed by different task, rescheduling isn't needed",
indexName(),
id);
@@ -110,7 +110,7 @@
IndexingRetry retry = new IndexingRetry(event);
if (indexingRetryTaskMap.put(id, retry) != null) {
indexOnce.accept(id);
- log.info(
+ log.debug(
"Skipping indexing because there is already a running task for the specified id. Index name: {}, task id: {}",
indexName(),
id);
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 063d7bf..4c5e8c9 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
@@ -22,11 +22,12 @@
import com.google.gerrit.server.util.ManualRequestContext;
import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.inject.Inject;
+import com.google.inject.Module;
import com.google.inject.assistedinject.Assisted;
+import com.google.inject.assistedinject.FactoryModuleBuilder;
import com.googlesource.gerrit.plugins.multisite.forwarder.events.ChangeIndexEvent;
import java.io.IOException;
import java.sql.Timestamp;
-import java.util.Objects;
import java.util.Optional;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.ObjectId;
@@ -50,6 +51,12 @@
public ChangeChecker create(String changeId);
}
+ public static Module module() {
+ return new FactoryModuleBuilder()
+ .implement(ChangeChecker.class, ChangeCheckerImpl.class)
+ .build(ChangeCheckerImpl.Factory.class);
+ }
+
@Inject
public ChangeCheckerImpl(
GitRepositoryManager gitRepoMgr,
@@ -102,8 +109,7 @@
.map(
e ->
(computedChangeTs.get() > e.eventCreatedOn)
- || ((computedChangeTs.get() == e.eventCreatedOn)
- && (Objects.equals(getBranchTargetSha(), e.targetSha))))
+ || ((computedChangeTs.get() == e.eventCreatedOn) && repositoryHas(e.targetSha)))
.orElse(true);
}
@@ -142,6 +148,15 @@
}
}
+ private boolean repositoryHas(String targetSha) {
+ try (Repository repo = gitRepoMgr.openRepository(changeNotes.get().getProjectName())) {
+ return repo.parseCommit(ObjectId.fromString(targetSha)) != null;
+ } catch (IOException e) {
+ log.warn("Unable to find SHA1 {} for change {}", targetSha, changeId, e);
+ return false;
+ }
+ }
+
@Override
public boolean isChangeConsistent() {
Optional<ChangeNotes> notes = getChangeNotes();
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/index/IndexModule.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/index/IndexModule.java
index 075388f..bc669af 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/multisite/index/IndexModule.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/index/IndexModule.java
@@ -20,7 +20,6 @@
import com.google.gerrit.extensions.events.ProjectIndexedListener;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.lifecycle.LifecycleModule;
-import com.google.inject.assistedinject.FactoryModuleBuilder;
import java.util.concurrent.Executor;
import java.util.concurrent.ScheduledExecutorService;
@@ -41,9 +40,6 @@
bind(ProjectChecker.class).to(ProjectCheckerImpl.class);
bind(GroupChecker.class).to(GroupCheckerImpl.class);
- install(
- new FactoryModuleBuilder()
- .implement(ChangeChecker.class, ChangeCheckerImpl.class)
- .build(ChangeCheckerImpl.Factory.class));
+ install(ChangeCheckerImpl.module());
}
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/multisite/index/ChangeCheckerIT.java b/src/test/java/com/googlesource/gerrit/plugins/multisite/index/ChangeCheckerIT.java
new file mode 100644
index 0000000..d3c5e8d
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/plugins/multisite/index/ChangeCheckerIT.java
@@ -0,0 +1,114 @@
+// Copyright (C) 2024 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.googlesource.gerrit.plugins.multisite.index;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.TestPlugin;
+import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.inject.AbstractModule;
+import com.googlesource.gerrit.plugins.multisite.forwarder.events.ChangeIndexEvent;
+import java.util.Optional;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.junit.Test;
+
+@TestPlugin(
+ name = "multi-site",
+ sysModule = "com.googlesource.gerrit.plugins.multisite.index.ChangeCheckerIT$TestModule")
+public class ChangeCheckerIT extends LightweightPluginDaemonTest {
+ private static final String TEST_BRANCH_REFS_HEADS = RefNames.fullName("master");
+ private static final String NONEXISTENTSHA1 = "d3d192b8f704aec0c764cca427fdaca88db71513";
+ private ChangeCheckerImpl.Factory changeCheckerFactory;
+
+ public static class TestModule extends AbstractModule {
+ @Override
+ protected void configure() {
+ install(ChangeCheckerImpl.module());
+ }
+ }
+
+ @Override
+ public void setUpTestPlugin() throws Exception {
+ super.setUpTestPlugin();
+
+ changeCheckerFactory = plugin.getSysInjector().getInstance(ChangeCheckerImpl.Factory.class);
+ }
+
+ @Test
+ @GerritConfig(name = "gerrit.instanceId", value = "test-instance")
+ public void shouldBeUpToDateIfTargetBranchSHA1HasAdvanced() throws Exception {
+ RevCommit headCommit = createCommit();
+ int changeNum = newChangeNum();
+ String changeId = changeProjectAndNum(changeNum);
+ long changeCommitTs = changeCommitTs(changeNum);
+
+ ChangeIndexEvent indexChangeEvent = newIndexChangeEvent(changeNum);
+ indexChangeEvent.eventCreatedOn = changeCommitTs / 1000L;
+ indexChangeEvent.targetSha = headCommit.getId().getName();
+
+ RevCommit secondCommit = createCommit();
+ assertThat(secondCommit.getId().getName()).isNotEqualTo(indexChangeEvent.targetSha);
+ assertThat(changeCheckerFactory.create(changeId).isUpToDate(Optional.of(indexChangeEvent)))
+ .isTrue();
+ }
+
+ @Test
+ @GerritConfig(name = "gerrit.instanceId", value = "test-instance")
+ public void shouldNotBeUpToDateIfTargetSHA1Absent() throws Exception {
+ int changeNum = newChangeNum();
+ String changeId = changeProjectAndNum(changeNum);
+ long changeCommitTs = changeCommitTs(changeNum);
+
+ ChangeIndexEvent indexChangeEvent = newIndexChangeEvent(changeNum);
+ indexChangeEvent.eventCreatedOn = changeCommitTs / 1000L;
+ indexChangeEvent.targetSha = NONEXISTENTSHA1;
+
+ assertThat(changeCheckerFactory.create(changeId).isUpToDate(Optional.of(indexChangeEvent)))
+ .isFalse();
+ }
+
+ private ChangeIndexEvent newIndexChangeEvent(int changeNum) {
+ ChangeIndexEvent indexChangeEvent =
+ new ChangeIndexEvent(project.get(), changeNum, false, instanceId);
+ return indexChangeEvent;
+ }
+
+ private int newChangeNum() throws Exception {
+ PushOneCommit.Result changeRes = createChange();
+ changeRes.assertOkStatus();
+ int changeNum = changeRes.getChange().getId().get();
+ return changeNum;
+ }
+
+ private String changeProjectAndNum(int changeNum) {
+ String changeId = String.format("%s~%d", project.get(), changeNum);
+ return changeId;
+ }
+
+ private long changeCommitTs(int changeNum) throws RestApiException {
+ long changeCommitTs = gApi.changes().id(project.get(), changeNum).get().updated.getTime();
+ return changeCommitTs;
+ }
+
+ private RevCommit createCommit() throws Exception {
+ PushOneCommit.Result result = pushTo(TEST_BRANCH_REFS_HEADS);
+ result.assertOkStatus();
+ return result.getCommit();
+ }
+}
