Merge branch 'stable-3.1' into stable-3.2
* stable-3.1:
Remove index.waitTimeout configuration property
Fix indexing locks on the sender side
Align Project.NameKey serialization to gerrit core
Lock on index element id rather than on index task id
Do not change Gerrit files ownership
Change-Id: Ia8ebf930847f5b0d02246cc81de9a2d854c21678
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 99f0dd0..83e0599 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -3,7 +3,7 @@
load("//:bazlets.bzl", "load_bazlets")
load_bazlets(
- commit = "91ea277efd6d9f24c18d6ddeba65bed866b00872",
+ commit = "cf0bbc90e09a8a1d7c042d79f1555e3fa40984e1",
#local_path = "/home/<user>/projects/bazlets",
)
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/Configuration.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/Configuration.java
index 4743bee..801bc9a 100644
--- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/Configuration.java
+++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/Configuration.java
@@ -17,6 +17,7 @@
import static java.util.concurrent.TimeUnit.HOURS;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.CharMatcher;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
@@ -76,8 +77,9 @@
STATIC
}
+ @VisibleForTesting
@Inject
- Configuration(
+ public Configuration(
PluginConfigFactory pluginConfigFactory, @PluginName String pluginName, SitePaths site) {
Config cfg = pluginConfigFactory.getGlobalPluginConfig(pluginName);
main = new Main(site, cfg);
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/cache/CachePatternMatcher.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/cache/CachePatternMatcher.java
index 0f43eaf..baf512f 100644
--- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/cache/CachePatternMatcher.java
+++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/cache/CachePatternMatcher.java
@@ -26,7 +26,7 @@
@Singleton
class CachePatternMatcher {
private static final List<String> DEFAULT_PATTERNS =
- ImmutableList.of("accounts", "^groups.*", "ldap_usernames", "projects", "sshkeys");
+ ImmutableList.of("^groups.*", "ldap_usernames", "projects", "sshkeys");
private final Pattern pattern;
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 68fa8eb..9ee2e92 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.
@@ -229,3 +229,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/CacheEvictionHandlerTest.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/CacheEvictionHandlerTest.java
new file mode 100644
index 0000000..7077319
--- /dev/null
+++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/CacheEvictionHandlerTest.java
@@ -0,0 +1,63 @@
+// Copyright (C) 2020 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.cache;
+
+import static org.mockito.Mockito.verifyZeroInteractions;
+import static org.mockito.Mockito.when;
+
+import com.ericsson.gerrit.plugins.highavailability.Configuration;
+import com.ericsson.gerrit.plugins.highavailability.forwarder.Forwarder;
+import com.google.common.cache.RemovalCause;
+import com.google.common.cache.RemovalNotification;
+import com.google.gerrit.server.config.PluginConfigFactory;
+import com.google.gerrit.server.config.SitePaths;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.concurrent.Executor;
+import org.eclipse.jgit.lib.Config;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class CacheEvictionHandlerTest {
+ @Mock private Executor executorMock;
+ @Mock private Forwarder forwarder;
+ @Mock private PluginConfigFactory pluginConfigFactoryMock;
+
+ private static final String PLUGIN_NAME = "high-availability";
+ private static final Path SITE_PATH = Paths.get("/site_path");
+ private CachePatternMatcher defaultCacheMatcher;
+
+ @Before
+ public void setUp() throws IOException {
+ when(pluginConfigFactoryMock.getGlobalPluginConfig(PLUGIN_NAME)).thenReturn(new Config());
+ defaultCacheMatcher =
+ new CachePatternMatcher(
+ new Configuration(pluginConfigFactoryMock, PLUGIN_NAME, new SitePaths(SITE_PATH)));
+ }
+
+ @Test
+ public void shouldNotPublishAccountsCacheEvictions() {
+ CacheEvictionHandler<String, String> handler =
+ new CacheEvictionHandler<>(forwarder, executorMock, PLUGIN_NAME, defaultCacheMatcher);
+ handler.onRemoval(
+ "test", "accounts", RemovalNotification.create("test", "accounts", RemovalCause.EXPLICIT));
+ verifyZeroInteractions(executorMock);
+ }
+}
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/CachePatternMatcherTest.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/CachePatternMatcherTest.java
index 9b6e66b..caf50b9 100644
--- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/CachePatternMatcherTest.java
+++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/CachePatternMatcherTest.java
@@ -38,7 +38,6 @@
CachePatternMatcher matcher = new CachePatternMatcher(configurationMock);
for (String cache :
ImmutableList.of(
- "accounts",
"groups",
"groups_byinclude",
"groups_byname",
@@ -55,6 +54,7 @@
}
for (String cache :
ImmutableList.of(
+ "accounts",
"adv_bases",
"change_kind",
"change_notes",
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;
