Merge branch 'stable-3.8' into stable-3.9
* stable-3.8:
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: I93b55dc97518657f522e2fc030fd938ac329f0e5
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 7b90468..f3fa2e3 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
@@ -26,11 +26,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.Config;
@@ -57,6 +58,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,
@@ -114,8 +121,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);
}
@@ -154,6 +160,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 5f47371..0e7a54f 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
@@ -21,7 +21,6 @@
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.lifecycle.LifecycleModule;
import com.google.gerrit.server.events.EventListener;
-import com.google.inject.assistedinject.FactoryModuleBuilder;
import java.util.concurrent.Executor;
import java.util.concurrent.ScheduledExecutorService;
@@ -43,9 +42,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();
+ }
+}
