opendev/gerrit
Code Issues Proposed changes

Don't use CacheModule for binding the external ID cache

Browse Source 
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>
This commit is contained in:
Edwin Kempin
2017-04-10 15:12:55 +02:00
parent 7c5c3a5940
commit c1be869e1d
2 changed files with 25 additions and 39 deletions
Download Patch File Download Diff File Expand all files Collapse all files

33
gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java
View File

@@ -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<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 @@ class ExternalIdCacheImpl implements ExternalIdCache {
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;
}

31
gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdModule.java
View File

@@ -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);
}