Merge branch 'stable-3.1' into stable-3.2 * stable-3.1: Stop occupying threads during retry phase Fix issue with indexing tasks queue Change-Id: Ifba0ce701eb1b32385d3eb6783712a71ffca9264
diff --git a/BUILD b/BUILD index c3f5a11..3b896ff 100644 --- a/BUILD +++ b/BUILD
@@ -25,6 +25,7 @@ junit_tests( name = "high-availability_tests", srcs = glob(["src/test/java/**/*.java"]), + javacopts = ["-Xep:DoNotMock:OFF"], resources = glob(["src/test/resources/**/*"]), tags = [ "high-availability",
diff --git a/WORKSPACE b/WORKSPACE index 039af8d..7e3bf7c 100644 --- a/WORKSPACE +++ b/WORKSPACE
@@ -3,7 +3,7 @@ load("//:bazlets.bzl", "load_bazlets") load_bazlets( - commit = "3f9dadc615dc4053369a42d9ada37dafd8d4763c", + commit = "0f81174e3d1b892a1342ebc75bb4bbb158ae0efe", #local_path = "/home/<user>/projects/bazlets", )
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/autoreindex/IndexTs.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/autoreindex/IndexTs.java index 6255986..ff42a4e 100644 --- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/autoreindex/IndexTs.java +++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/autoreindex/IndexTs.java
@@ -113,12 +113,12 @@ currCtx.onlyWithContext( (ctx) -> { try { - ChangeNotes changeNotes = changeFinder.findOne(projectName + "~" + id); + Optional<ChangeNotes> changeNotes = changeFinder.findOne(projectName + "~" + id); update( IndexName.CHANGE, - changeNotes == null + !changeNotes.isPresent() ? LocalDateTime.now() - : changeNotes.getChange().getLastUpdatedOn().toLocalDateTime()); + : changeNotes.get().getChange().getLastUpdatedOn().toLocalDateTime()); } catch (Exception e) { log.atWarning().withCause(e).log("Unable to update the latest TS for change %d", id); }
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedAwareEventBroker.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedAwareEventBroker.java index f156da4..fb651b7 100644 --- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedAwareEventBroker.java +++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedAwareEventBroker.java
@@ -14,6 +14,7 @@ package com.ericsson.gerrit.plugins.highavailability.forwarder; +import com.google.gerrit.server.config.GerritInstanceId; import com.google.gerrit.server.events.Event; import com.google.gerrit.server.events.EventBroker; import com.google.gerrit.server.events.EventListener; @@ -23,6 +24,8 @@ import com.google.gerrit.server.plugincontext.PluginSetContext; import com.google.gerrit.server.project.ProjectCache; import com.google.inject.Inject; +import java.util.Objects; +import javax.annotation.Nullable; class ForwardedAwareEventBroker extends EventBroker { @@ -32,12 +35,30 @@ PluginSetContext<EventListener> unrestrictedListeners, PermissionBackend permissionBackend, ProjectCache projectCache, - Factory notesFactory) { - super(listeners, unrestrictedListeners, permissionBackend, projectCache, notesFactory); + Factory notesFactory, + @Nullable @GerritInstanceId String gerritInstanceId) { + super( + listeners, + unrestrictedListeners, + permissionBackend, + projectCache, + notesFactory, + gerritInstanceId); + } + + private boolean isProducedByLocalInstance(Event event) { + return Objects.equals(event.instanceId, gerritInstanceId); } @Override protected void fireEventForUnrestrictedListeners(Event event) { + // An event should not be dispatched when it is "forwarded". + // Meaning, it was either produced somewhere else + if (!isProducedByLocalInstance(event)) { + Context.setForwardedEvent(true); + } + // or it was consumed by the high-availability rest endpoint and + // thus the context of its consumption has already been set to "forwarded". if (!Context.isForwardedEvent()) { super.fireEventForUnrestrictedListeners(event); }
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 4a0cad7..fe04ee9 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
@@ -76,7 +76,7 @@ @Override public Optional<ChangeNotes> getChangeNotes() { try (ManualRequestContext ctx = oneOffReqCtx.open()) { - changeNotes = Optional.ofNullable(changeFinder.findOne(changeId)); + changeNotes = changeFinder.findOne(changeId); return changeNotes; } }
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md index 4ca2db7..cdb7b47 100644 --- a/src/main/resources/Documentation/config.md +++ b/src/main/resources/Documentation/config.md
@@ -3,8 +3,8 @@ ========================= The @PLUGIN@ plugin must be installed on all the instances. Each instance should -be configured with the same [gerrit.serverId](https://gerrit-documentation.storage.googleapis.com/Documentation/3.1.0/config-gerrit.html#gerrit.serverId). -If there are existing changes in [NoteDb](https://gerrit-documentation.storage.googleapis.com/Documentation/3.1.0/note-db.html) +be configured with the same [gerrit.serverId](https://gerrit-documentation.storage.googleapis.com/Documentation/3.2.0/config-gerrit.html#gerrit.serverId). +If there are existing changes in [NoteDb](https://gerrit-documentation.storage.googleapis.com/Documentation/3.2.0/note-db.html) made with another `serverId`, then this plugin might not be able to access them. Likewise, if the HA gerrit.serverIds differ, then changes conveyed by one instance will not be accessible by the other. @@ -238,3 +238,23 @@ ```healthcheck.enable``` : Whether to enable the health check endpoint. Defaults to 'true'. + +File 'gerrit.config' +-------------------- + +```gerrit.instanceId``` +: Optional identifier for this Gerrit instance. +[[docs](https://gerrit-documentation.storage.googleapis.com/Documentation/3.2.0/config-gerrit.html#gerrit.instanceId)]. + +Whilst this is not, specifically, a high-availability plugin configuration, it plays +an important role on which events are forwarded to peers. + +If `instanceId` is set, events produced by that Gerrit instance will contain its +origin in the `instanceId` field, allowing to track where they are coming from. + +The high-availability plugin will check the `instanceId` value to decide whether +an event should be forwarded: events that originated from different Gerrit instances +will not be forwarded. + +When neither `gerrit.instanceId` nor `event.instanceId` are set, it is not possible +to identify the origin of the event and thus the event is always forwarded.
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/CacheEvictionIT.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/CacheEvictionIT.java index e91e2ae..180c68f 100644 --- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/CacheEvictionIT.java +++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/CacheEvictionIT.java
@@ -26,13 +26,13 @@ import com.github.tomakehurst.wiremock.junit.WireMockRule; import com.google.common.cache.LoadingCache; -import com.google.gerrit.acceptance.GerritConfig; -import com.google.gerrit.acceptance.GlobalPluginConfig; import com.google.gerrit.acceptance.LightweightPluginDaemonTest; import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.TestPlugin; import com.google.gerrit.acceptance.UseLocalDisk; import com.google.gerrit.acceptance.UseSsh; +import com.google.gerrit.acceptance.config.GerritConfig; +import com.google.gerrit.acceptance.config.GlobalPluginConfig; import com.google.gerrit.entities.AccountGroup; import com.google.inject.Inject; import com.google.inject.name.Named;
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/ProjectListIT.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/ProjectListIT.java index 22ef89e..98cdce5 100644 --- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/ProjectListIT.java +++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/ProjectListIT.java
@@ -25,11 +25,11 @@ import static com.google.common.truth.Truth.assertThat; import com.github.tomakehurst.wiremock.junit.WireMockRule; -import com.google.gerrit.acceptance.GlobalPluginConfig; import com.google.gerrit.acceptance.LightweightPluginDaemonTest; import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.TestPlugin; import com.google.gerrit.acceptance.UseLocalDisk; +import com.google.gerrit.acceptance.config.GlobalPluginConfig; import com.google.gerrit.extensions.restapi.Url; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit;
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedAwareEventBrokerTest.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedAwareEventBrokerTest.java index 36cc953..cfe0475 100644 --- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedAwareEventBrokerTest.java +++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedAwareEventBrokerTest.java
@@ -30,7 +30,9 @@ private EventListener listenerMock; private ForwardedAwareEventBroker broker; - private Event event = new TestEvent(); + private ForwardedAwareEventBroker brokerWithGerritInstanceId; + private Event event; + private String gerritInstanceId = "gerrit-instance-id"; @Before public void setUp() { @@ -38,8 +40,11 @@ listenerMock = mock(EventListener.class); DynamicSet<EventListener> set = DynamicSet.emptySet(); set.add("high-availability", listenerMock); + event = new TestEvent(); PluginSetContext<EventListener> listeners = new PluginSetContext<>(set, mockMetrics); - broker = new ForwardedAwareEventBroker(null, listeners, null, null, null); + broker = new ForwardedAwareEventBroker(null, listeners, null, null, null, null); + brokerWithGerritInstanceId = + new ForwardedAwareEventBroker(null, listeners, null, null, null, gerritInstanceId); } @Test @@ -58,4 +63,43 @@ } verifyZeroInteractions(listenerMock); } + + @Test + public void shouldNotDispatchEventWhenEventInstanceIdIsDefinedButGerritInstanceIdIsNot() { + event.instanceId = gerritInstanceId; + try { + broker.fireEventForUnrestrictedListeners(event); + } finally { + Context.unsetForwardedEvent(); + } + verifyZeroInteractions(listenerMock); + } + + @Test + public void shouldNotDispatchEventWhenGerritInstanceIdIsDefinedButEventInstanceIdIsNot() { + try { + brokerWithGerritInstanceId.fireEventForUnrestrictedListeners(event); + } finally { + Context.unsetForwardedEvent(); + } + verifyZeroInteractions(listenerMock); + } + + @Test + public void shouldNotDispatchEventWhenInstanceIdsAreDifferent() { + event.instanceId = "some-other-gerrit-instance-id"; + try { + brokerWithGerritInstanceId.fireEventForUnrestrictedListeners(event); + } finally { + Context.unsetForwardedEvent(); + } + verifyZeroInteractions(listenerMock); + } + + @Test + public void shouldDispatchEventWhenInstanceIdsAreEqual() { + event.instanceId = gerritInstanceId; + brokerWithGerritInstanceId.fireEventForUnrestrictedListeners(event); + verify(listenerMock).onEvent(event); + } }
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/index/AbstractIndexForwardingIT.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/index/AbstractIndexForwardingIT.java index af4637e..5843908 100644 --- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/index/AbstractIndexForwardingIT.java +++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/index/AbstractIndexForwardingIT.java
@@ -26,11 +26,11 @@ import static com.google.common.truth.Truth.assertThat; import com.github.tomakehurst.wiremock.junit.WireMockRule; -import com.google.gerrit.acceptance.GlobalPluginConfig; import com.google.gerrit.acceptance.LightweightPluginDaemonTest; import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.TestPlugin; import com.google.gerrit.acceptance.UseLocalDisk; +import com.google.gerrit.acceptance.config.GlobalPluginConfig; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import org.apache.http.HttpStatus;