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 <ekempin@google.com>
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 0624e19..9db00d5 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 @@
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.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 @@
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<ObjectId, ImmutableSetMultimap<Account.Id, ExternalId>>
extIdsByAccount;
private final ExternalIdReader externalIdReader;
private final Lock lock;
@Inject
- ExternalIdCacheImpl(
- @Named(CACHE_NAME)
- LoadingCache<ObjectId, ImmutableSetMultimap<Account.Id, ExternalId>> 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 @@
Collections2.transform(ids, e -> e.key()).removeAll(toRemove);
}
- static class Loader extends CacheLoader<ObjectId, ImmutableSetMultimap<Account.Id, ExternalId>> {
+ private static class Loader
+ extends CacheLoader<ObjectId, ImmutableSetMultimap<Account.Id, ExternalId>> {
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 3486b4e..8c97144 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<ImmutableSetMultimap<Account.Id, ExternalId>>() {})
- // 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);
}
