From 06c86046fefc6555b98d81f3726dd664020aeb28 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Sat, 9 Nov 2019 12:38:13 +0100 Subject: [PATCH 1/2] Replace guava caches with caffeine Replacing Guava caches with Caffeine reduces the chances of having the deadlocks and improves the cache performance. This was already attempted in: I8d2b17a94d0, but got reverted in: If65560b4a9b due to recursion in PatchListLoader. This recursion issue is present on current master. While this change replaces all caches with Caffeine backend, the follow-up change in this series will switch back to using Guava backend for PatchListCache implementation. For seamless integration, the caffeine-guava adapter library is used. Given that the final artifact for the adapter is also called guava, there is only the version number that differentiates that artifact from the guava library itself so that we have a danger for naming collision. To avoid potential naming collision risk, rename the library name to caffeine-guava.jar during the fetch from Maven Central. Alternatives considered is not to use the caffeine-guava adapter library. But then the Cache and LoadingCache classes and friends would change the package name from com.google.common.cache package to com.github.benmanes.caffeine.cache package and this change would also affect some gerrit plugins and thus considered to be a quite intrusive change. Still we can consider to do this change in one of the future gerrit releases. Bug: Issue 7645 Bug: Issue 11484 Change-Id: I6af4c15d6c15f438becd62409b7d233c309be8de (cherry picked from commit 0050a9b5f61da67a9f33fda0bb170f8b1df6080a) --- WORKSPACE | 25 ++++++++++ java/com/google/gerrit/server/cache/mem/BUILD | 2 + .../cache/mem/DefaultMemoryCacheFactory.java | 50 ++++++++++--------- lib/BUILD | 25 +++++++++- 4 files changed, 77 insertions(+), 25 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index 9b17514fbb..cc2435a24e 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -292,6 +292,31 @@ maven_jar( sha1 = GUAVA_BIN_SHA1, ) +CAFFEINE_VERS = "2.8.0" + +maven_jar( + name = "caffeine", + artifact = "com.github.ben-manes.caffeine:caffeine:" + CAFFEINE_VERS, + sha1 = "6000774d7f8412ced005a704188ced78beeed2bb", +) + +# TODO(davido): Rename guava.jar to caffeine-guava.jar on fetch to prevent potential +# naming collision between caffeine guava adapater and guava library itself. +# Remove this renaming procedure, once this upstream issue is fixed: +# https://github.com/ben-manes/caffeine/issues/364. +http_file( + name = "caffeine-guava-renamed", + downloaded_file_path = "caffeine-guava-" + CAFFEINE_VERS + ".jar", + sha256 = "3a66ee3ec70971dee0bae6e56bda7b8742bc4bedd7489161bfbbaaf7137d89e1", + urls = [ + "https://repo1.maven.org/maven2/com/github/ben-manes/caffeine/guava/" + + CAFFEINE_VERS + + "/guava-" + + CAFFEINE_VERS + + ".jar", + ], +) + maven_jar( name = "jsch", artifact = "com.jcraft:jsch:0.1.54", diff --git a/java/com/google/gerrit/server/cache/mem/BUILD b/java/com/google/gerrit/server/cache/mem/BUILD index eb0695e1b1..bc5b66ac6b 100644 --- a/java/com/google/gerrit/server/cache/mem/BUILD +++ b/java/com/google/gerrit/server/cache/mem/BUILD @@ -8,6 +8,8 @@ java_library( "//java/com/google/gerrit/common:annotations", "//java/com/google/gerrit/extensions:api", "//java/com/google/gerrit/server", + "//lib:caffeine", + "//lib:caffeine-guava", "//lib:guava", "//lib/guice", "//lib/jgit/org.eclipse.jgit:jgit", diff --git a/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java b/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java index ad1d396668..0a42ce10f8 100644 --- a/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java +++ b/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java @@ -17,12 +17,15 @@ package com.google.gerrit.server.cache.mem; import static java.util.concurrent.TimeUnit.NANOSECONDS; import static java.util.concurrent.TimeUnit.SECONDS; +import com.github.benmanes.caffeine.cache.Caffeine; +import com.github.benmanes.caffeine.cache.RemovalListener; +import com.github.benmanes.caffeine.cache.Weigher; +import com.github.benmanes.caffeine.guava.CaffeinatedGuava; import com.google.common.base.Strings; import com.google.common.cache.Cache; -import com.google.common.cache.CacheBuilder; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; -import com.google.common.cache.Weigher; +import com.google.common.cache.RemovalNotification; import com.google.gerrit.common.Nullable; import com.google.gerrit.server.cache.CacheDef; import com.google.gerrit.server.cache.ForwardingRemovalListener; @@ -47,28 +50,21 @@ class DefaultMemoryCacheFactory implements MemoryCacheFactory { @Override public Cache build(CacheDef def) { - return create(def).build(); + return CaffeinatedGuava.build(create(def)); } @Override public LoadingCache build(CacheDef def, CacheLoader loader) { - return create(def).build(loader); + return CaffeinatedGuava.build(create(def), loader); } - @SuppressWarnings("unchecked") - private CacheBuilder create(CacheDef def) { - CacheBuilder builder = newCacheBuilder(); + private Caffeine create(CacheDef def) { + Caffeine builder = newCacheBuilder(); builder.recordStats(); builder.maximumWeight( cfg.getLong("cache", def.configKey(), "memoryLimit", def.maximumWeight())); - - builder = builder.removalListener(forwardingRemovalListenerFactory.create(def.name())); - - Weigher weigher = def.weigher(); - if (weigher == null) { - weigher = unitWeight(); - } - builder.weigher(weigher); + builder = builder.removalListener(newRemovalListener(def.name())); + builder.weigher(newWeigher(def.weigher())); Duration expireAfterWrite = def.expireAfterWrite(); if (has(def.configKey(), "maxAge")) { @@ -107,16 +103,22 @@ class DefaultMemoryCacheFactory implements MemoryCacheFactory { } @SuppressWarnings("unchecked") - private static CacheBuilder newCacheBuilder() { - return (CacheBuilder) CacheBuilder.newBuilder(); + private static Caffeine newCacheBuilder() { + return (Caffeine) Caffeine.newBuilder(); } - private static Weigher unitWeight() { - return new Weigher() { - @Override - public int weigh(K key, V value) { - return 1; - } - }; + @SuppressWarnings("unchecked") + private RemovalListener newRemovalListener(String cacheName) { + return (k, v, cause) -> + forwardingRemovalListenerFactory + .create(cacheName) + .onRemoval( + RemovalNotification.create( + k, v, com.google.common.cache.RemovalCause.valueOf(cause.name()))); + } + + private static Weigher newWeigher( + com.google.common.cache.Weigher guavaWeigher) { + return guavaWeigher == null ? Weigher.singletonWeigher() : (k, v) -> guavaWeigher.weigh(k, v); } } diff --git a/lib/BUILD b/lib/BUILD index 0474171b21..77b2fcdc3d 100644 --- a/lib/BUILD +++ b/lib/BUILD @@ -1,4 +1,4 @@ -load("@rules_java//java:defs.bzl", "java_library") +load("@rules_java//java:defs.bzl", "java_import", "java_library") exports_files(glob([ "LICENSE-*", @@ -98,6 +98,29 @@ java_library( ], ) +java_library( + name = "caffeine", + data = ["//lib:LICENSE-Apache2.0"], + visibility = [ + "//java/com/google/gerrit/server/cache/mem:__pkg__", + ], + exports = ["@caffeine//jar"], +) + +java_import( + name = "caffeine-guava-renamed", + jars = ["@caffeine-guava-renamed//file"], +) + +java_library( + name = "caffeine-guava", + data = ["//lib:LICENSE-Apache2.0"], + visibility = [ + "//java/com/google/gerrit/server/cache/mem:__pkg__", + ], + exports = [":caffeine-guava-renamed"], +) + java_library( name = "jsch", data = ["//lib:LICENSE-jsch"], From 5ac9a97f2b0aab5a2fa59e2779aa0daec6733f00 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Tue, 12 Nov 2019 05:49:23 -0800 Subject: [PATCH 2/2] Switch PatchListCache to using legacy cache backend The implementation of PatchListLoader is using internally the PatchListCache, to retrieve rebase related edits. The loading function calling back into the cache violates the Caffeine interface contracts. Guava could, theoretically, be more tolerant than Caffeine. This is due to the hashtable design of locking and table expansion. However both are explicit that the cache loader cannot write into the cache. Caffeine will fail a fibonacci test, whereas Guava might pass. But that is taking a lot of risk by relying on implementation details and violating the contracts. Switch to using legacy cache backend for PatchListCache, and consider to switch using Caffeine cache backend and remove the legacy cache support altogether, when the PatchListLoader is fixed to be recursion free. Bug: Issue 11900 Change-Id: I8f129dcf9b5574f024a600162d98aa381b3513cc (cherry picked from commit 1d8f5ca43512ea7bc293520bfdbca23f1ad663fd) --- .../gerrit/server/cache/CacheBackend.java | 25 +++++++ .../gerrit/server/cache/CacheModule.java | 25 +++++-- .../gerrit/server/cache/CacheProvider.java | 13 +++- .../server/cache/MemoryCacheFactory.java | 5 +- .../server/cache/PersistentCacheFactory.java | 5 +- .../server/cache/PersistentCacheProvider.java | 17 ++++- .../server/cache/h2/H2CacheFactory.java | 19 ++++-- .../cache/mem/DefaultMemoryCacheFactory.java | 67 +++++++++++++++++-- .../server/patch/PatchListCacheImpl.java | 5 +- .../server/change/PatchListCacheIT.java | 21 ++++++ 10 files changed, 177 insertions(+), 25 deletions(-) create mode 100644 java/com/google/gerrit/server/cache/CacheBackend.java diff --git a/java/com/google/gerrit/server/cache/CacheBackend.java b/java/com/google/gerrit/server/cache/CacheBackend.java new file mode 100644 index 0000000000..ec9876f93f --- /dev/null +++ b/java/com/google/gerrit/server/cache/CacheBackend.java @@ -0,0 +1,25 @@ +// Copyright (C) 2019 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.google.gerrit.server.cache; + +/** Caffeine is used as default cache backend, but can be overridden with Guava backend. */ +public enum CacheBackend { + CAFFEINE, + GUAVA; + + public boolean isLegacyBackend() { + return this == GUAVA; + } +} diff --git a/java/com/google/gerrit/server/cache/CacheModule.java b/java/com/google/gerrit/server/cache/CacheModule.java index 28786247bd..0fdc6f5ffd 100644 --- a/java/com/google/gerrit/server/cache/CacheModule.java +++ b/java/com/google/gerrit/server/cache/CacheModule.java @@ -68,7 +68,8 @@ public abstract class CacheModule extends FactoryModule { */ protected CacheBinding cache( String name, TypeLiteral keyType, TypeLiteral valType) { - CacheProvider m = new CacheProvider<>(this, name, keyType, valType); + CacheProvider m = + new CacheProvider<>(this, name, keyType, valType, CacheBackend.CAFFEINE); bindCache(m, name, keyType, valType); return m; } @@ -123,7 +124,20 @@ public abstract class CacheModule extends FactoryModule { */ protected PersistentCacheBinding persist( String name, Class keyType, Class valType) { - return persist(name, TypeLiteral.get(keyType), TypeLiteral.get(valType)); + return persist(name, TypeLiteral.get(keyType), TypeLiteral.get(valType), CacheBackend.CAFFEINE); + } + + /** + * Declare a named in-memory/on-disk cache. + * + * @param type of key used to lookup entries. + * @param type of value stored by the cache. + * @param backend cache backend. + * @return binding to describe the cache. + */ + protected PersistentCacheBinding persist( + String name, Class keyType, Class valType, CacheBackend backend) { + return persist(name, TypeLiteral.get(keyType), TypeLiteral.get(valType), backend); } /** @@ -135,7 +149,7 @@ public abstract class CacheModule extends FactoryModule { */ protected PersistentCacheBinding persist( String name, Class keyType, TypeLiteral valType) { - return persist(name, TypeLiteral.get(keyType), valType); + return persist(name, TypeLiteral.get(keyType), valType, CacheBackend.CAFFEINE); } /** @@ -146,8 +160,9 @@ public abstract class CacheModule extends FactoryModule { * @return binding to describe the cache. */ protected PersistentCacheBinding persist( - String name, TypeLiteral keyType, TypeLiteral valType) { - PersistentCacheProvider m = new PersistentCacheProvider<>(this, name, keyType, valType); + String name, TypeLiteral keyType, TypeLiteral valType, CacheBackend backend) { + PersistentCacheProvider m = + new PersistentCacheProvider<>(this, name, keyType, valType, backend); bindCache(m, name, keyType, valType); Type cacheDefType = diff --git a/java/com/google/gerrit/server/cache/CacheProvider.java b/java/com/google/gerrit/server/cache/CacheProvider.java index b1a9b91b45..fe4244c4db 100644 --- a/java/com/google/gerrit/server/cache/CacheProvider.java +++ b/java/com/google/gerrit/server/cache/CacheProvider.java @@ -30,6 +30,7 @@ import java.time.Duration; class CacheProvider implements Provider>, CacheBinding, CacheDef { private final CacheModule module; + private final CacheBackend backend; final String name; private final TypeLiteral keyType; private final TypeLiteral valType; @@ -44,11 +45,17 @@ class CacheProvider implements Provider>, CacheBinding, private MemoryCacheFactory memoryCacheFactory; private boolean frozen; - CacheProvider(CacheModule module, String name, TypeLiteral keyType, TypeLiteral valType) { + CacheProvider( + CacheModule module, + String name, + TypeLiteral keyType, + TypeLiteral valType, + CacheBackend backend) { this.module = module; this.name = name; this.keyType = keyType; this.valType = valType; + this.backend = backend; } @Inject(optional = true) @@ -159,7 +166,9 @@ class CacheProvider implements Provider>, CacheBinding, public Cache get() { freeze(); CacheLoader ldr = loader(); - return ldr != null ? memoryCacheFactory.build(this, ldr) : memoryCacheFactory.build(this); + return ldr != null + ? memoryCacheFactory.build(this, ldr, backend) + : memoryCacheFactory.build(this, backend); } protected void checkNotFrozen() { diff --git a/java/com/google/gerrit/server/cache/MemoryCacheFactory.java b/java/com/google/gerrit/server/cache/MemoryCacheFactory.java index fc55753070..558380d263 100644 --- a/java/com/google/gerrit/server/cache/MemoryCacheFactory.java +++ b/java/com/google/gerrit/server/cache/MemoryCacheFactory.java @@ -19,7 +19,8 @@ import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; public interface MemoryCacheFactory { - Cache build(CacheDef def); + Cache build(CacheDef def, CacheBackend backend); - LoadingCache build(CacheDef def, CacheLoader loader); + LoadingCache build( + CacheDef def, CacheLoader loader, CacheBackend backend); } diff --git a/java/com/google/gerrit/server/cache/PersistentCacheFactory.java b/java/com/google/gerrit/server/cache/PersistentCacheFactory.java index 27fa9cae93..93f91ef4f0 100644 --- a/java/com/google/gerrit/server/cache/PersistentCacheFactory.java +++ b/java/com/google/gerrit/server/cache/PersistentCacheFactory.java @@ -19,9 +19,10 @@ import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; public interface PersistentCacheFactory { - Cache build(PersistentCacheDef def); + Cache build(PersistentCacheDef def, CacheBackend backend); - LoadingCache build(PersistentCacheDef def, CacheLoader loader); + LoadingCache build( + PersistentCacheDef def, CacheLoader loader, CacheBackend backend); void onStop(String plugin); } diff --git a/java/com/google/gerrit/server/cache/PersistentCacheProvider.java b/java/com/google/gerrit/server/cache/PersistentCacheProvider.java index 59d66e3624..4fc107f737 100644 --- a/java/com/google/gerrit/server/cache/PersistentCacheProvider.java +++ b/java/com/google/gerrit/server/cache/PersistentCacheProvider.java @@ -30,6 +30,7 @@ import java.time.Duration; class PersistentCacheProvider extends CacheProvider implements Provider>, PersistentCacheBinding, PersistentCacheDef { + private final CacheBackend backend; private int version; private long diskLimit; private CacheSerializer keySerializer; @@ -39,9 +40,19 @@ class PersistentCacheProvider extends CacheProvider PersistentCacheProvider( CacheModule module, String name, TypeLiteral keyType, TypeLiteral valType) { - super(module, name, keyType, valType); + this(module, name, keyType, valType, CacheBackend.CAFFEINE); + } + + PersistentCacheProvider( + CacheModule module, + String name, + TypeLiteral keyType, + TypeLiteral valType, + CacheBackend backend) { + super(module, name, keyType, valType, backend); version = -1; diskLimit = 128 << 20; + this.backend = backend; } @Inject(optional = true) @@ -130,8 +141,8 @@ class PersistentCacheProvider extends CacheProvider freeze(); CacheLoader ldr = loader(); return ldr != null - ? persistentCacheFactory.build(this, ldr) - : persistentCacheFactory.build(this); + ? persistentCacheFactory.build(this, ldr, backend) + : persistentCacheFactory.build(this, backend); } private static void checkSerializer( diff --git a/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java b/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java index af1228dfe3..2b068aac59 100644 --- a/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java +++ b/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java @@ -21,6 +21,7 @@ import com.google.common.flogger.FluentLogger; import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.google.gerrit.extensions.events.LifecycleListener; import com.google.gerrit.extensions.registration.DynamicMap; +import com.google.gerrit.server.cache.CacheBackend; import com.google.gerrit.server.cache.MemoryCacheFactory; import com.google.gerrit.server.cache.PersistentCacheDef; import com.google.gerrit.server.cache.PersistentCacheFactory; @@ -156,18 +157,21 @@ class H2CacheFactory implements PersistentCacheFactory, LifecycleListener { @SuppressWarnings({"unchecked"}) @Override - public Cache build(PersistentCacheDef in) { + public Cache build(PersistentCacheDef in, CacheBackend backend) { long limit = config.getLong("cache", in.configKey(), "diskLimit", in.diskLimit()); if (cacheDir == null || limit <= 0) { - return memCacheFactory.build(in); + return memCacheFactory.build(in, backend); } H2CacheDefProxy def = new H2CacheDefProxy<>(in); SqlStore store = newSqlStore(def, limit); H2CacheImpl cache = new H2CacheImpl<>( - executor, store, def.keyType(), (Cache>) memCacheFactory.build(def)); + executor, + store, + def.keyType(), + (Cache>) memCacheFactory.build(def, backend)); synchronized (caches) { caches.add(cache); } @@ -176,11 +180,12 @@ class H2CacheFactory implements PersistentCacheFactory, LifecycleListener { @SuppressWarnings("unchecked") @Override - public LoadingCache build(PersistentCacheDef in, CacheLoader loader) { + public LoadingCache build( + PersistentCacheDef in, CacheLoader loader, CacheBackend backend) { long limit = config.getLong("cache", in.configKey(), "diskLimit", in.diskLimit()); if (cacheDir == null || limit <= 0) { - return memCacheFactory.build(in, loader); + return memCacheFactory.build(in, loader, backend); } H2CacheDefProxy def = new H2CacheDefProxy<>(in); @@ -188,7 +193,9 @@ class H2CacheFactory implements PersistentCacheFactory, LifecycleListener { Cache> mem = (Cache>) memCacheFactory.build( - def, (CacheLoader) new H2CacheImpl.Loader<>(executor, store, loader)); + def, + (CacheLoader) new H2CacheImpl.Loader<>(executor, store, loader), + backend); H2CacheImpl cache = new H2CacheImpl<>(executor, store, def.keyType(), mem); synchronized (caches) { caches.add(cache); diff --git a/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java b/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java index 0a42ce10f8..9906b3dc40 100644 --- a/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java +++ b/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java @@ -23,10 +23,12 @@ import com.github.benmanes.caffeine.cache.Weigher; import com.github.benmanes.caffeine.guava.CaffeinatedGuava; import com.google.common.base.Strings; import com.google.common.cache.Cache; +import com.google.common.cache.CacheBuilder; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; import com.google.common.cache.RemovalNotification; import com.google.gerrit.common.Nullable; +import com.google.gerrit.server.cache.CacheBackend; import com.google.gerrit.server.cache.CacheDef; import com.google.gerrit.server.cache.ForwardingRemovalListener; import com.google.gerrit.server.cache.MemoryCacheFactory; @@ -49,13 +51,61 @@ class DefaultMemoryCacheFactory implements MemoryCacheFactory { } @Override - public Cache build(CacheDef def) { - return CaffeinatedGuava.build(create(def)); + public Cache build(CacheDef def, CacheBackend backend) { + return backend.isLegacyBackend() + ? createLegacy(def).build() + : CaffeinatedGuava.build(create(def)); } @Override - public LoadingCache build(CacheDef def, CacheLoader loader) { - return CaffeinatedGuava.build(create(def), loader); + public LoadingCache build( + CacheDef def, CacheLoader loader, CacheBackend backend) { + return backend.isLegacyBackend() + ? createLegacy(def).build(loader) + : CaffeinatedGuava.build(create(def), loader); + } + + @SuppressWarnings("unchecked") + private CacheBuilder createLegacy(CacheDef def) { + CacheBuilder builder = newLegacyCacheBuilder(); + builder.recordStats(); + builder.maximumWeight( + cfg.getLong("cache", def.configKey(), "memoryLimit", def.maximumWeight())); + + builder = builder.removalListener(forwardingRemovalListenerFactory.create(def.name())); + + com.google.common.cache.Weigher weigher = def.weigher(); + if (weigher == null) { + weigher = unitWeight(); + } + builder.weigher(weigher); + + Duration expireAfterWrite = def.expireAfterWrite(); + if (has(def.configKey(), "maxAge")) { + builder.expireAfterWrite( + ConfigUtil.getTimeUnit( + cfg, "cache", def.configKey(), "maxAge", toSeconds(expireAfterWrite), SECONDS), + SECONDS); + } else if (expireAfterWrite != null) { + builder.expireAfterWrite(expireAfterWrite.toNanos(), NANOSECONDS); + } + + Duration expireAfterAccess = def.expireFromMemoryAfterAccess(); + if (has(def.configKey(), "expireFromMemoryAfterAccess")) { + builder.expireAfterAccess( + ConfigUtil.getTimeUnit( + cfg, + "cache", + def.configKey(), + "expireFromMemoryAfterAccess", + toSeconds(expireAfterAccess), + SECONDS), + SECONDS); + } else if (expireAfterAccess != null) { + builder.expireAfterAccess(expireAfterAccess.toNanos(), NANOSECONDS); + } + + return builder; } private Caffeine create(CacheDef def) { @@ -102,6 +152,15 @@ class DefaultMemoryCacheFactory implements MemoryCacheFactory { return !Strings.isNullOrEmpty(cfg.getString("cache", name, var)); } + @SuppressWarnings("unchecked") + private static CacheBuilder newLegacyCacheBuilder() { + return (CacheBuilder) CacheBuilder.newBuilder(); + } + + private static com.google.common.cache.Weigher unitWeight() { + return (key, value) -> 1; + } + @SuppressWarnings("unchecked") private static Caffeine newCacheBuilder() { return (Caffeine) Caffeine.newBuilder(); diff --git a/java/com/google/gerrit/server/patch/PatchListCacheImpl.java b/java/com/google/gerrit/server/patch/PatchListCacheImpl.java index 6039fff793..20f0f720b8 100644 --- a/java/com/google/gerrit/server/patch/PatchListCacheImpl.java +++ b/java/com/google/gerrit/server/patch/PatchListCacheImpl.java @@ -22,6 +22,7 @@ import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.server.cache.CacheBackend; import com.google.gerrit.server.cache.CacheModule; import com.google.gerrit.server.config.GerritServerConfig; import com.google.inject.Inject; @@ -45,7 +46,9 @@ public class PatchListCacheImpl implements PatchListCache { @Override protected void configure() { factory(PatchListLoader.Factory.class); - persist(FILE_NAME, PatchListKey.class, PatchList.class) + // TODO(davido): Switch off using legacy cache backend, after fixing PatchListLoader + // to be recursion free. + persist(FILE_NAME, PatchListKey.class, PatchList.class, CacheBackend.GUAVA) .maximumWeight(10 << 20) .weigher(PatchListWeigher.class); diff --git a/javatests/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java b/javatests/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java index 18925665a7..424dbac422 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java +++ b/javatests/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java @@ -39,7 +39,9 @@ import com.google.gerrit.server.patch.PatchListKey; import com.google.gerrit.server.patch.Text; import com.google.inject.Inject; import com.google.inject.name.Named; +import java.lang.reflect.Field; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashSet; import java.util.List; import java.util.Optional; @@ -65,6 +67,25 @@ public class PatchListCacheIT extends AbstractDaemonTest { @Named("diff") private Cache abstractPatchListCache; + @Test + public void ensureLegacyBackendIsUsedForFileCacheBackend() throws Exception { + Field fileCacheField = patchListCache.getClass().getDeclaredField("fileCache"); + fileCacheField.setAccessible(true); + // Use the reflection to access "localCache" field that is only present in Guava backend. + assertThat( + Arrays.stream(fileCacheField.get(patchListCache).getClass().getDeclaredFields()) + .anyMatch(f -> f.getName().equals("localCache"))) + .isTrue(); + + // intraCache (and all other cache backends) should use Caffeine backend. + Field intraCacheField = patchListCache.getClass().getDeclaredField("intraCache"); + intraCacheField.setAccessible(true); + assertThat( + Arrays.stream(intraCacheField.get(patchListCache).getClass().getDeclaredFields()) + .noneMatch(f -> f.getName().equals("localCache"))) + .isTrue(); + } + @Test public void listPatchesAgainstBase() throws Exception { commitBuilder().add(FILE_D, "4").message(SUBJECT_1).create();