From c1be869e1d38c811d28fa201006367daeb73b071 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 10 Apr 2017 15:12:55 +0200 Subject: [PATCH] Don't use CacheModule for binding the external ID cache Using CacheModule is not needed since the external ID cache is not persisted and we don't want to let admins configure its size. Change-Id: I3e823fe2e354ed9c719344fba3132e1563fe6a30 Signed-off-by: Edwin Kempin --- .../externalids/ExternalIdCacheImpl.java | 33 +++++++++++++------ .../account/externalids/ExternalIdModule.java | 31 ++--------------- 2 files changed, 25 insertions(+), 39 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java index 0624e1956c..9db00d5d94 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.account.externalids; import static java.util.stream.Collectors.toSet; +import com.google.common.cache.CacheBuilder; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; import com.google.common.collect.Collections2; @@ -27,7 +28,6 @@ import com.google.common.collect.MultimapBuilder; import com.google.gerrit.reviewdb.client.Account; import com.google.inject.Inject; import com.google.inject.Singleton; -import com.google.inject.name.Named; import java.io.IOException; import java.util.Collection; import java.util.Set; @@ -44,19 +44,32 @@ import org.slf4j.LoggerFactory; class ExternalIdCacheImpl implements ExternalIdCache { private static final Logger log = LoggerFactory.getLogger(ExternalIdCacheImpl.class); - public static final String CACHE_NAME = "external_ids_map"; - private final LoadingCache> extIdsByAccount; private final ExternalIdReader externalIdReader; private final Lock lock; @Inject - ExternalIdCacheImpl( - @Named(CACHE_NAME) - LoadingCache> extIdsByAccount, - ExternalIdReader externalIdReader) { - this.extIdsByAccount = extIdsByAccount; + ExternalIdCacheImpl(ExternalIdReader externalIdReader) { + this.extIdsByAccount = + CacheBuilder.newBuilder() + // The cached data is potentially pretty large and we are always only interested + // in the latest value, hence the maximum cache size is set to 1. + // This can lead to extra cache loads in case of the following race: + // 1. thread 1 reads the notes ref at revision A + // 2. thread 2 updates the notes ref to revision B and stores the derived value + // for B in the cache + // 3. thread 1 attempts to read the data for revision A from the cache, and misses + // 4. later threads attempt to read at B + // In this race unneeded reloads are done in step 3 (reload from revision A) and + // step 4 (reload from revision B, because the value for revision B was lost when the + // reload from revision A was done, since the cache can hold only one entry). + // These reloads could be avoided by increasing the cache size to 2. However the race + // window between reading the ref and looking it up in the cache is small so that + // it's rare that this race happens. Therefore it's not worth to double the memory + // usage of this cache, just to avoid this. + .maximumSize(1) + .build(new Loader(externalIdReader)); this.externalIdReader = externalIdReader; this.lock = new ReentrantLock(true /* fair */); } @@ -249,10 +262,10 @@ class ExternalIdCacheImpl implements ExternalIdCache { Collections2.transform(ids, e -> e.key()).removeAll(toRemove); } - static class Loader extends CacheLoader> { + private static class Loader + extends CacheLoader> { private final ExternalIdReader externalIdReader; - @Inject Loader(ExternalIdReader externalIdReader) { this.externalIdReader = externalIdReader; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdModule.java index 3486b4e340..8c9714474c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdModule.java @@ -14,38 +14,11 @@ package com.google.gerrit.server.account.externalids; -import com.google.common.collect.ImmutableSetMultimap; -import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.server.account.externalids.ExternalIdCacheImpl.Loader; -import com.google.gerrit.server.cache.CacheModule; -import com.google.inject.TypeLiteral; -import org.eclipse.jgit.lib.ObjectId; +import com.google.inject.AbstractModule; -public class ExternalIdModule extends CacheModule { +public class ExternalIdModule extends AbstractModule { @Override protected void configure() { - cache( - ExternalIdCacheImpl.CACHE_NAME, - ObjectId.class, - new TypeLiteral>() {}) - // The cached data is potentially pretty large and we are always only interested - // in the latest value, hence the maximum cache weight is set to 1. - // This can lead to extra cache loads in case of the following race: - // 1. thread 1 reads the notes ref at revision A - // 2. thread 2 updates the notes ref to revision B and stores the derived value - // for B in the cache - // 3. thread 1 attempts to read the data for revision A from the cache, and misses - // 4. later threads attempt to read at B - // In this race unneeded reloads are done in step 3 (reload from revision A) and - // step 4 (reload from revision B, because the value for revision B was lost when the - // reload from revision A was done, since the cache can hold only one entry). - // These reloads could be avoided by increasing the cache size to 2. However the race - // window between reading the ref and looking it up in the cache is small so that - // it's rare that this race happens. Therefore it's not worth to double the memory - // usage of this cache, just to avoid this. - .maximumWeight(1) - .loader(Loader.class); - bind(ExternalIdCacheImpl.class); bind(ExternalIdCache.class).to(ExternalIdCacheImpl.class); }