Merge branch 'stable-3.1' into stable-3.2
* stable-3.1:
Handle missing unknown during loading of change notes
IndexEventHandler: Fix FloggerFormatString warning
GroupReindexRunnable: Fix FloggerFormatString warning
RestForwarder: Add missing override annotation
AbstractIndexRestApiServlet: Fix resource leak warning
Upgrade bazlets to latest stable-3.1 to build with 3.1.12 API
Change-Id: Id5983f0bcd1d24b0c5a1f3d9100d2dea85c4503e
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 6969594..acca707 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -3,7 +3,7 @@
load("//:bazlets.bzl", "load_bazlets")
load_bazlets(
- commit = "87fd5f0d0a89d01df13deaf2d21a4bdb3bc03cfd",
+ commit = "8dc0767541f16b35d2136eccebffd9ebe2b81133",
#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 093628e..af1c4c3 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;
@@ -77,8 +78,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 561bb19..62d9c10 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
@@ -133,12 +133,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 23712be..945cefa 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
@@ -87,7 +87,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 5186bd1..3b1fee2 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/autoreindex/GroupReindexRunnableTest.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/autoreindex/GroupReindexRunnableTest.java
index 5139a66..ce11242 100644
--- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/autoreindex/GroupReindexRunnableTest.java
+++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/autoreindex/GroupReindexRunnableTest.java
@@ -194,13 +194,16 @@
}
private Optional<InternalGroup> getInternalGroup(Timestamp timestamp) {
- AccountGroup accountGroup =
- new AccountGroup(AccountGroup.nameKey("Test"), AccountGroup.id(1), uuid, timestamp);
return Optional.ofNullable(
- InternalGroup.create(
- accountGroup,
- ImmutableSet.<Id>builder().build(),
- ImmutableSet.<UUID>builder().build(),
- null));
+ InternalGroup.builder()
+ .setId(AccountGroup.Id.parse("1"))
+ .setNameKey(AccountGroup.nameKey("Test"))
+ .setOwnerGroupUUID(uuid)
+ .setVisibleToAll(true)
+ .setGroupUUID(UUID.parse("12"))
+ .setCreatedOn(timestamp)
+ .setMembers(ImmutableSet.<Id>builder().build())
+ .setSubgroups(ImmutableSet.<UUID>builder().build())
+ .build());
}
}
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;
