Merge branch stable-3.14 into master Change-Id: I6892d63055c2d7008fca06b9e722a46ad7e3a972
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/cache/Constants.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/cache/Constants.java index d4fab98..66208e2 100644 --- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/cache/Constants.java +++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/cache/Constants.java
@@ -20,6 +20,7 @@ public static final String ACCOUNTS = "accounts"; public static final String GROUPS = "groups"; public static final String GROUPS_BYINCLUDE = "groups_byinclude"; + public static final String GROUPS_BYMEMBER = "groups_bymember"; public static final String GROUPS_MEMBERS = "groups_members"; public static final String PROJECTS = "projects"; public static final String TOKENS = "tokens";
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedIndexChangeHandler.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedIndexChangeHandler.java index 88cd72f..3bf5872 100644 --- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedIndexChangeHandler.java +++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedIndexChangeHandler.java
@@ -85,18 +85,17 @@ changeNotes = Optional.empty(); } if (changeNotes.isPresent()) { - ChangeNotes notes = changeNotes.get(); - reindex(notes); - - if (checker.isChangeUpToDate(indexEvent)) { - log.atFine().log("Change %s successfully indexed", id); - return true; + if (!checker.isChangeUpToDate(indexEvent)) { + log.atFine().log( + "Change %s is not yet up to date with the event (event=%s, change=%s)", + id, indexEvent, checker); + return false; } - log.atFine().log( - "Change %s seems too old compared to the event timestamp (event-Ts=%s >> change-Ts=%s)", - id, indexEvent, checker); - return false; + ChangeNotes notes = changeNotes.get(); + reindex(notes); + log.atFine().log("Change %s successfully indexed", id); + return true; } log.atFine().log(
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/CacheKeyJsonParser.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/CacheKeyJsonParser.java index 6202f48..f3eb71e 100644 --- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/CacheKeyJsonParser.java +++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/CacheKeyJsonParser.java
@@ -17,11 +17,9 @@ import com.ericsson.gerrit.plugins.highavailability.cache.Constants; import com.google.common.base.CharMatcher; import com.google.common.base.Strings; -import com.google.common.base.Supplier; -import com.google.common.base.Suppliers; -import com.google.gerrit.entities.Account; -import com.google.gerrit.entities.AccountGroup; import com.google.gerrit.entities.Project; +import com.google.gerrit.extensions.registration.DynamicMap; +import com.google.gerrit.server.cache.CacheDef; import com.google.gson.Gson; import com.google.gson.JsonElement; import com.google.inject.Inject; @@ -30,37 +28,44 @@ @Singleton public class CacheKeyJsonParser { private final Gson gson; + private final DynamicMap<CacheDef<?, ?>> cachesMap; @Inject - public CacheKeyJsonParser(@RestGson Gson gson) { + public CacheKeyJsonParser(@RestGson Gson gson, DynamicMap<CacheDef<?, ?>> cachesMap) { this.gson = gson; + this.cachesMap = cachesMap; } public Object fromJson(String cacheName, String jsonString) { JsonElement json = gson.fromJson(Strings.nullToEmpty(jsonString), JsonElement.class); - Supplier<JsonElement> id = Suppliers.memoize(() -> json.getAsJsonObject().get("id")); - Supplier<JsonElement> uuid = Suppliers.memoize(() -> json.getAsJsonObject().get("uuid")); - - // Need to add a case for 'adv_bases' switch (cacheName) { - case Constants.ACCOUNTS: - case Constants.TOKENS: - return id.get() == null ? null : Account.id(id.get().getAsInt()); - case Constants.GROUPS: - return id.get() == null ? null : AccountGroup.id(id.get().getAsInt()); - case Constants.GROUPS_BYINCLUDE: - case Constants.GROUPS_MEMBERS: - return uuid.get() == null ? null : AccountGroup.uuid(uuid.get().getAsString()); case Constants.PROJECT_LIST: return gson.fromJson(json, Object.class); case Constants.PROJECTS: return Project.nameKey(CharMatcher.is('\"').trimFrom(json.getAsString())); default: try { - return gson.fromJson(json, String.class); + return gson.fromJson(json, getCacheKeyType(cacheName)); } catch (Exception e) { return gson.fromJson(json, Object.class); } } } + + private Class<?> getCacheKeyType(String cacheName) { + int dot = cacheName.indexOf('.'); + String pluginName = Constants.GERRIT; + String pluginCacheName = cacheName; + if (dot > 0) { + pluginName = cacheName.substring(0, dot); + pluginCacheName = cacheName.substring(dot + 1); + } + + CacheDef<?, ?> cacheDef = cachesMap.get(pluginName, pluginCacheName); + if (cacheDef == null) { + throw new IllegalStateException("Unable to find definition for cache '" + cacheName + "'"); + } + + return cacheDef.keyType().getRawType(); + } }
diff --git a/src/test/docker/gerrit/Dockerfile b/src/test/docker/gerrit/Dockerfile index d18e060..3db728b 100644 --- a/src/test/docker/gerrit/Dockerfile +++ b/src/test/docker/gerrit/Dockerfile
@@ -11,7 +11,7 @@ nfs-utils \ && yum -y clean all -ENV GERRIT_BRANCH stable-3.13 +ENV GERRIT_BRANCH stable-3.14 # Add gerrit user RUN adduser -p -m --uid 1000 gerrit --home-dir /home/gerrit @@ -27,7 +27,7 @@ /tmp/gerrit.war ADD --chown=gerrit \ - "https://gerrit-ci.gerritforge.com/view/Plugins-master/job/plugin-javamelody-bazel-master-$GERRIT_BRANCH/lastSuccessfulBuild/artifact/bazel-bin/plugins/javamelody/javamelody.jar" \ + "https://gerrit-ci.gerritforge.com/view/Plugins-master/job/plugin-javamelody-bazel-$GERRIT_BRANCH/lastSuccessfulBuild/artifact/bazel-bin/plugins/javamelody/javamelody.jar" \ /var/gerrit/plugins/javamelody.jar ADD --chown=gerrit \
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedIndexChangeHandlerTest.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedIndexChangeHandlerTest.java index ea936bf..2ad8fed 100644 --- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedIndexChangeHandlerTest.java +++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedIndexChangeHandlerTest.java
@@ -20,8 +20,8 @@ import static java.util.concurrent.TimeUnit.SECONDS; import static org.mockito.Answers.RETURNS_DEEP_STUBS; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -101,11 +101,30 @@ } @Test - public void changeIsStillIndexedEvenWhenOutdated() throws Exception { + public void changeIsNotReindexedWhenShaIsNeverVisible() throws Exception { setupChangeAccessRelatedMocks(CHANGE_EXISTS, CHANGE_OUTDATED); handler.index(TEST_CHANGE_ID, Operation.INDEX, Optional.of(new IndexEvent())).get(10, SECONDS); - verify(indexerMock, atLeast(1)) - .reindexIfStale(any(Project.NameKey.class), any(Change.Id.class)); + verify(indexerMock, never()).reindexIfStale(any(Project.NameKey.class), any(Change.Id.class)); + } + + @Test + public void changeIsEventuallyIndexedWhenShaBecomesVisible() throws Exception { + // First attempt: sha not visible yet (outdated); second attempt: sha visible (up-to-date). + when(changeCheckerFactoryMock.create(TEST_CHANGE_ID)) + .thenReturn(changeCheckerAbsentMock) + .thenReturn(changeCheckerPresentMock); + + when(changeCheckerAbsentMock.getChangeNotes()).thenReturn(Optional.of(changeNotes)); + when(changeCheckerAbsentMock.isChangeUpToDate(any())).thenReturn(CHANGE_OUTDATED); + + when(changeCheckerPresentMock.getChangeNotes()).thenReturn(Optional.of(changeNotes)); + when(changeCheckerPresentMock.isChangeUpToDate(any())).thenReturn(CHANGE_UP_TO_DATE); + + when(changeNotes.getChangeId()).thenReturn(id); + when(changeNotes.getProjectName()).thenReturn(projectName); + + handler.index(TEST_CHANGE_ID, Operation.INDEX, Optional.of(new IndexEvent())).get(10, SECONDS); + verify(indexerMock, times(1)).reindexIfStale(any(Project.NameKey.class), any(Change.Id.class)); } @Test
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/commands/CommandDeserializerTest.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/commands/CommandDeserializerTest.java index 7c3a886..8818905 100644 --- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/commands/CommandDeserializerTest.java +++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/commands/CommandDeserializerTest.java
@@ -20,6 +20,7 @@ import com.ericsson.gerrit.plugins.highavailability.forwarder.EventType; import com.ericsson.gerrit.plugins.highavailability.forwarder.rest.CacheKeyJsonParser; import com.google.gerrit.entities.Project; +import com.google.gerrit.extensions.registration.DynamicMap; import com.google.gerrit.server.events.Event; import com.google.gerrit.server.events.EventGsonProvider; import com.google.gerrit.server.events.ProjectCreatedEvent; @@ -36,7 +37,7 @@ public void setUp() { Gson eventGson = new EventGsonProvider().get(); this.gson = new ForwarderCommandsModule().buildCommandsGson(eventGson); - this.cacheKeyParser = new CacheKeyJsonParser(eventGson); + this.cacheKeyParser = new CacheKeyJsonParser(eventGson, DynamicMap.emptyMap()); } @Test
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/CacheKeyJsonParserTest.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/CacheKeyJsonParserTest.java index d661cad..c08fef0 100644 --- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/CacheKeyJsonParserTest.java +++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/CacheKeyJsonParserTest.java
@@ -17,23 +17,117 @@ import static com.google.common.truth.Truth.assertThat; import com.ericsson.gerrit.plugins.highavailability.cache.Constants; +import com.google.common.cache.CacheLoader; +import com.google.common.cache.Weigher; import com.google.gerrit.entities.Account; import com.google.gerrit.entities.AccountGroup; import com.google.gerrit.entities.Project; +import com.google.gerrit.extensions.registration.DynamicMap; +import com.google.gerrit.extensions.registration.PrivateInternals_DynamicMapImpl; +import com.google.gerrit.extensions.registration.RegistrationHandle; +import com.google.gerrit.server.cache.CacheDef; import com.google.gerrit.server.events.EventGsonProvider; import com.google.gson.Gson; +import com.google.inject.TypeLiteral; +import com.google.inject.util.Providers; +import java.time.Duration; +import org.junit.Before; import org.junit.Test; public class CacheKeyJsonParserTest { private static final Object EMPTY_JSON = "{}"; private final Gson gson = RestForwarderModule.buildRestGson(new EventGsonProvider().get()); - private final CacheKeyJsonParser objectUnderTest = new CacheKeyJsonParser(gson); + private CacheKeyJsonParser objectUnderTest; + + private PrivateInternals_DynamicMapImpl<CacheDef<?, ?>> cacheDefMap; + + @Before + public void setUp() throws Exception { + cacheDefMap = + (PrivateInternals_DynamicMapImpl<CacheDef<?, ?>>) DynamicMap.<CacheDef<?, ?>>emptyMap(); + + defineCache(Constants.GROUPS_BYMEMBER, Account.Id.class); + defineCache(Constants.ACCOUNTS, Account.Id.class); + defineCache(Constants.TOKENS, Account.Id.class); + defineCache(Constants.GROUPS, AccountGroup.Id.class); + defineCache(Constants.GROUPS_BYINCLUDE, AccountGroup.UUID.class); + defineCache(Constants.GROUPS_MEMBERS, AccountGroup.UUID.class); + + objectUnderTest = new CacheKeyJsonParser(gson, cacheDefMap); + } + + private void defineCache(String cacheName, Class<?> keyClass) { + RegistrationHandle unused = + cacheDefMap.put( + Constants.GERRIT, cacheName, Providers.of(new TestCacheDef<>(cacheName, keyClass))); + } + + static class TestCacheDef<K> implements CacheDef<K, Object> { + private final Class<K> keyClass; + private final String name; + + TestCacheDef(String name, Class<K> keyClass) { + this.name = name; + this.keyClass = keyClass; + } + + @Override + public String name() { + return name; + } + + @Override + public String configKey() { + return ""; + } + + @Override + public TypeLiteral<K> keyType() { + return TypeLiteral.get(keyClass); + } + + @Override + public TypeLiteral<Object> valueType() { + return null; + } + + @Override + public long maximumWeight() { + return 0; + } + + @Override + public Duration expireAfterWrite() { + return null; + } + + @Override + public Duration expireFromMemoryAfterAccess() { + return null; + } + + @Override + public Duration refreshAfterWrite() { + return null; + } + + @Override + public Weigher<K, Object> weigher() { + return null; + } + + @Override + public CacheLoader<K, Object> loader() { + return null; + } + } @Test public void accountIDParse() { Account.Id accountId = Account.id(1); String json = gson.toJson(accountId); assertThat(accountId).isEqualTo(objectUnderTest.fromJson(Constants.ACCOUNTS, json)); + assertThat(accountId).isEqualTo(objectUnderTest.fromJson(Constants.GROUPS_BYMEMBER, json)); } @Test
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/CacheRestApiServletTest.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/CacheRestApiServletTest.java index 68d348c..88a325e 100644 --- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/CacheRestApiServletTest.java +++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/CacheRestApiServletTest.java
@@ -26,6 +26,7 @@ import com.ericsson.gerrit.plugins.highavailability.forwarder.ForwardedCacheEvictionHandler; import com.ericsson.gerrit.plugins.highavailability.forwarder.ProcessorMetrics; import com.ericsson.gerrit.plugins.highavailability.forwarder.ProcessorMetricsRegistry; +import com.google.gerrit.extensions.registration.DynamicMap; import com.google.gson.Gson; import java.io.BufferedReader; import java.io.IOException; @@ -52,7 +53,9 @@ when(metricsRegistry.get(any())).thenReturn(metrics); servlet = new CacheRestApiServlet( - forwardedCacheEvictionHandlerMock, new CacheKeyJsonParser(new Gson()), metricsRegistry); + forwardedCacheEvictionHandlerMock, + new CacheKeyJsonParser(new Gson(), DynamicMap.emptyMap()), + metricsRegistry); } @Test