opendev/gerrit
Code Issues Proposed changes

Remove accounts_byname cache

Browse Source 
Instead of caching accounts by username let AccountCache#getByUsername
do:

1. Load the external ID for the username from NoteDb.
2. Lookup the account from the byId cache using the account ID of the
   external ID

By removing this cache we no longer need to do cache evictions when the
username changes.

Change-Id: I5285dbdf110e71dc7912e53eaff44be9ec9c08b0
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2017-08-25 13:37:28 +02:00
parent 93b74fbeaf
commit 0115124853
8 changed files with 23 additions and 67 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
Documentation/cmd-show-caches.txt
View File

@@ -58,7 +58,6 @@ Intended for interactive use only.
| Mem Disk Space| |Mem Disk|
--------------------------------+---------------------+---------+---------+
accounts | 4096 | 3.4ms | 99% |
accounts_byname | 4096 | 11.3ms | 99% |
adv_bases | | | |
changes | | 27.1ms | 0% |
groups | 5646 | 11.8ms | 97% |

10
Documentation/rest-api-config.txt
View File

@@ -298,15 +298,6 @@ The entries in the map are sorted by cache name.
"mem": 94
}
},
"accounts_byname": {
"type": "MEM",
"entries": {
"mem": 4
},
"hit_ratio": {
"mem": 100
}
},
"adv_bases": {
"type": "MEM",
"entries": {},
@@ -504,7 +495,6 @@ The cache names are lexicographically sorted.
)]}'
[
"accounts",
"accounts_byname",
"adv_bases",
"change_kind",
"changes",

8
gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AccountCreator.java
View File

@@ -23,7 +23,6 @@ import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.Sequences;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountsUpdate;
import com.google.gerrit.server.account.VersionedAuthorizedKeys;
import com.google.gerrit.server.account.externalids.ExternalId;
@@ -58,7 +57,6 @@ public class AccountCreator {
private final Groups groups;
private final Provider<GroupsUpdate> groupsUpdateProvider;
private final SshKeyCache sshKeyCache;
private final AccountCache accountCache;
private final ExternalIdsUpdate.Server externalIdsUpdate;
private final boolean sshEnabled;
@@ -71,7 +69,6 @@ public class AccountCreator {
Groups groups,
@ServerInitiated Provider<GroupsUpdate> groupsUpdateProvider,
SshKeyCache sshKeyCache,
AccountCache accountCache,
ExternalIdsUpdate.Server externalIdsUpdate,
@SshEnabled boolean sshEnabled) {
accounts = new HashMap<>();
@@ -82,7 +79,6 @@ public class AccountCreator {
this.groups = groups;
this.groupsUpdateProvider = groupsUpdateProvider;
this.sshKeyCache = sshKeyCache;
this.accountCache = accountCache;
this.externalIdsUpdate = externalIdsUpdate;
this.sshEnabled = sshEnabled;
}
@@ -141,10 +137,6 @@ public class AccountCreator {
sshKeyCache.evict(username);
}
if (username != null) {
accountCache.evictByUsername(username);
}
account = new TestAccount(id, username, email, fullName, sshKey, httpPass);
if (username != null) {
accounts.put(username, account);

12
gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCache.java
View File

@@ -42,6 +42,16 @@ public interface AccountCache {
@Nullable
AccountState getOrNull(Account.Id accountId);
/**
* Returns an {@code AccountState} instance for the given username.
*
* <p>This method first loads the external ID for the username and then uses the account ID of the
* external ID to lookup the account from the cache.
*
* @param username username of the account that should be retrieved
* @return {@code AccountState} instance for the given username, if no account with this username
* exists or if loading the external ID fails {@code null} is returned
*/
AccountState getByUsername(String username);
/**
@@ -52,8 +62,6 @@ public interface AccountCache {
*/
void evict(Account.Id accountId) throws IOException;
void evictByUsername(String username);
/** Evict all accounts from the cache, but doesn't trigger reindex of all accounts. */
void evictAllNoReindex();
}

44
gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java
View File

@@ -28,6 +28,7 @@ import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.account.WatchConfig.NotifyType;
import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey;
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIds;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.config.AllUsersName;
@@ -59,7 +60,6 @@ public class AccountCacheImpl implements AccountCache {
private static final Logger log = LoggerFactory.getLogger(AccountCacheImpl.class);
private static final String BYID_NAME = "accounts";
private static final String BYUSER_NAME = "accounts_byname";
public static Module module() {
return new CacheModule() {
@@ -68,9 +68,6 @@ public class AccountCacheImpl implements AccountCache {
cache(BYID_NAME, Account.Id.class, new TypeLiteral<Optional<AccountState>>() {})
.loader(ByIdLoader.class);
cache(BYUSER_NAME, String.class, new TypeLiteral<Optional<Account.Id>>() {})
.loader(ByNameLoader.class);
bind(AccountCacheImpl.class);
bind(AccountCache.class).to(AccountCacheImpl.class);
}
@@ -78,19 +75,19 @@ public class AccountCacheImpl implements AccountCache {
}
private final AllUsersName allUsersName;
private final ExternalIds externalIds;
private final LoadingCache<Account.Id, Optional<AccountState>> byId;
private final LoadingCache<String, Optional<Account.Id>> byName;
private final Provider<AccountIndexer> indexer;
@Inject
AccountCacheImpl(
AllUsersName allUsersName,
ExternalIds externalIds,
@Named(BYID_NAME) LoadingCache<Account.Id, Optional<AccountState>> byId,
@Named(BYUSER_NAME) LoadingCache<String, Optional<Account.Id>> byUsername,
Provider<AccountIndexer> indexer) {
this.allUsersName = allUsersName;
this.externalIds = externalIds;
this.byId = byId;
this.byName = byUsername;
this.indexer = indexer;
}
@@ -110,7 +107,7 @@ public class AccountCacheImpl implements AccountCache {
try {
return byId.get(accountId).orElse(null);
} catch (ExecutionException e) {
log.warn("Cannot load AccountState for " + accountId, e);
log.warn("Cannot load AccountState for ID " + accountId, e);
return null;
}
}
@@ -118,10 +115,13 @@ public class AccountCacheImpl implements AccountCache {
@Override
public AccountState getByUsername(String username) {
try {
Optional<Account.Id> id = byName.get(username);
return id != null && id.isPresent() ? getOrNull(id.get()) : null;
} catch (ExecutionException e) {
log.warn("Cannot load AccountState for " + username, e);
ExternalId extId = externalIds.get(ExternalId.Key.create(SCHEME_USERNAME, username));
if (extId == null) {
return null;
}
return getOrNull(extId.accountId());
} catch (IOException | ConfigInvalidException e) {
log.warn("Cannot load AccountState for username " + username, e);
return null;
}
}
@@ -139,13 +139,6 @@ public class AccountCacheImpl implements AccountCache {
byId.invalidateAll();
}
@Override
public void evictByUsername(String username) {
if (username != null) {
byName.invalidate(username);
}
}
private AccountState missing(Account.Id accountId) {
Account account = new Account(accountId, TimeUtil.nowTs());
account.setActive(false);
@@ -165,7 +158,6 @@ public class AccountCacheImpl implements AccountCache {
private final GroupCache groupCache;
private final Groups groups;
private final GeneralPreferencesLoader loader;
private final LoadingCache<String, Optional<Account.Id>> byName;
private final Provider<WatchConfig.Accessor> watchConfig;
private final ExternalIds externalIds;
@@ -177,7 +169,6 @@ public class AccountCacheImpl implements AccountCache {
GroupCache groupCache,
Groups groups,
GeneralPreferencesLoader loader,
@Named(BYUSER_NAME) LoadingCache<String, Optional<Account.Id>> byUsername,
Provider<WatchConfig.Accessor> watchConfig,
ExternalIds externalIds) {
this.schema = sf;
@@ -186,7 +177,6 @@ public class AccountCacheImpl implements AccountCache {
this.groupCache = groupCache;
this.groups = groups;
this.loader = loader;
this.byName = byUsername;
this.watchConfig = watchConfig;
this.externalIds = externalIds;
}
@@ -194,15 +184,7 @@ public class AccountCacheImpl implements AccountCache {
@Override
public Optional<AccountState> load(Account.Id key) throws Exception {
try (ReviewDb db = schema.open()) {
Optional<AccountState> state = load(db, key);
if (!state.isPresent()) {
return state;
}
String user = state.get().getUserName();
if (user != null) {
byName.put(user, Optional.of(state.get().getAccount().getId()));
}
return state;
return load(db, key);
}
}

5
gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java
View File

@@ -46,7 +46,6 @@ public class ChangeUserName implements Callable<VoidResult> {
ChangeUserName create(IdentifiedUser user, String newUsername);
}
private final AccountCache accountCache;
private final SshKeyCache sshKeyCache;
private final ExternalIds externalIds;
private final ExternalIdsUpdate.Server externalIdsUpdateFactory;
@@ -56,13 +55,11 @@ public class ChangeUserName implements Callable<VoidResult> {
@Inject
ChangeUserName(
AccountCache accountCache,
SshKeyCache sshKeyCache,
ExternalIds externalIds,
ExternalIdsUpdate.Server externalIdsUpdateFactory,
@Assisted IdentifiedUser user,
@Nullable @Assisted String newUsername) {
this.accountCache = accountCache;
this.sshKeyCache = sshKeyCache;
this.externalIds = externalIds;
this.externalIdsUpdateFactory = externalIdsUpdateFactory;
@@ -113,10 +110,8 @@ public class ChangeUserName implements Callable<VoidResult> {
externalIdsUpdate.delete(old);
for (ExternalId extId : old) {
sshKeyCache.evict(extId.key().id());
accountCache.evictByUsername(extId.key().id());
}
accountCache.evictByUsername(newUsername);
sshKeyCache.evict(newUsername);
return VoidResult.INSTANCE;
}

5
gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java
View File

@@ -67,7 +67,6 @@ public class CreateAccount implements RestModifyView<TopLevelResource, AccountIn
private final GroupsCollection groupsCollection;
private final VersionedAuthorizedKeys.Accessor authorizedKeys;
private final SshKeyCache sshKeyCache;
private final AccountCache accountCache;
private final AccountsUpdate.User accountsUpdate;
private final AccountLoader.Factory infoLoader;
private final DynamicSet<AccountExternalIdCreator> externalIdCreators;
@@ -84,7 +83,6 @@ public class CreateAccount implements RestModifyView<TopLevelResource, AccountIn
GroupsCollection groupsCollection,
VersionedAuthorizedKeys.Accessor authorizedKeys,
SshKeyCache sshKeyCache,
AccountCache accountCache,
AccountsUpdate.User accountsUpdate,
AccountLoader.Factory infoLoader,
DynamicSet<AccountExternalIdCreator> externalIdCreators,
@@ -98,7 +96,6 @@ public class CreateAccount implements RestModifyView<TopLevelResource, AccountIn
this.groupsCollection = groupsCollection;
this.authorizedKeys = authorizedKeys;
this.sshKeyCache = sshKeyCache;
this.accountCache = accountCache;
this.accountsUpdate = accountsUpdate;
this.infoLoader = infoLoader;
this.externalIdCreators = externalIdCreators;
@@ -198,8 +195,6 @@ public class CreateAccount implements RestModifyView<TopLevelResource, AccountIn
}
}
accountCache.evictByUsername(username);
AccountLoader loader = infoLoader.create(true);
AccountInfo info = loader.get(id);
loader.fill();

5
gerrit-server/src/test/java/com/google/gerrit/testutil/FakeAccountCache.java
View File

@@ -60,11 +60,6 @@ public class FakeAccountCache implements AccountCache {
byId.remove(accountId);
}
@Override
public synchronized void evictByUsername(String username) {
byUsername.remove(username);
}
@Override
public synchronized void evictAllNoReindex() {
byId.clear();