Remove accounts_byname cache
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>
diff --git a/Documentation/cmd-show-caches.txt b/Documentation/cmd-show-caches.txt
index 5286d43..e47ae81 100644
--- a/Documentation/cmd-show-caches.txt
+++ b/Documentation/cmd-show-caches.txt
@@ -58,7 +58,6 @@
| 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% |
diff --git a/Documentation/rest-api-config.txt b/Documentation/rest-api-config.txt
index 6c8848b..871a4d2 100644
--- a/Documentation/rest-api-config.txt
+++ b/Documentation/rest-api-config.txt
@@ -298,15 +298,6 @@
"mem": 94
}
},
- "accounts_byname": {
- "type": "MEM",
- "entries": {
- "mem": 4
- },
- "hit_ratio": {
- "mem": 100
- }
- },
"adv_bases": {
"type": "MEM",
"entries": {},
@@ -504,7 +495,6 @@
)]}'
[
"accounts",
- "accounts_byname",
"adv_bases",
"change_kind",
"changes",
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AccountCreator.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AccountCreator.java
index 806469e..a781555 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AccountCreator.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AccountCreator.java
@@ -23,7 +23,6 @@
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 @@
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 @@
Groups groups,
@ServerInitiated Provider<GroupsUpdate> groupsUpdateProvider,
SshKeyCache sshKeyCache,
- AccountCache accountCache,
ExternalIdsUpdate.Server externalIdsUpdate,
@SshEnabled boolean sshEnabled) {
accounts = new HashMap<>();
@@ -82,7 +79,6 @@
this.groups = groups;
this.groupsUpdateProvider = groupsUpdateProvider;
this.sshKeyCache = sshKeyCache;
- this.accountCache = accountCache;
this.externalIdsUpdate = externalIdsUpdate;
this.sshEnabled = sshEnabled;
}
@@ -141,10 +137,6 @@
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);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCache.java
index b44de2d..bbc4f5f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCache.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCache.java
@@ -42,6 +42,16 @@
@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 @@
*/
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();
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java
index 16901ed..a1c19fc 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java
@@ -28,6 +28,7 @@
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 @@
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 @@
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 @@
}
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 @@
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 @@
@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 @@
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 @@
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 @@
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 @@
this.groupCache = groupCache;
this.groups = groups;
this.loader = loader;
- this.byName = byUsername;
this.watchConfig = watchConfig;
this.externalIds = externalIds;
}
@@ -194,15 +184,7 @@
@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);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java
index 230e516..3d1a5f6 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java
@@ -46,7 +46,6 @@
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 @@
@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 @@
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;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java
index 1812ef4..c66f779 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java
@@ -67,7 +67,6 @@
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 @@
GroupsCollection groupsCollection,
VersionedAuthorizedKeys.Accessor authorizedKeys,
SshKeyCache sshKeyCache,
- AccountCache accountCache,
AccountsUpdate.User accountsUpdate,
AccountLoader.Factory infoLoader,
DynamicSet<AccountExternalIdCreator> externalIdCreators,
@@ -98,7 +96,6 @@
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 @@
}
}
- accountCache.evictByUsername(username);
-
AccountLoader loader = infoLoader.create(true);
AccountInfo info = loader.get(id);
loader.fill();
diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/FakeAccountCache.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/FakeAccountCache.java
index c3b588b..51f5268 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/testutil/FakeAccountCache.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/FakeAccountCache.java
@@ -61,11 +61,6 @@
}
@Override
- public synchronized void evictByUsername(String username) {
- byUsername.remove(username);
- }
-
- @Override
public synchronized void evictAllNoReindex() {
byId.clear();
byUsername.clear();
