Merge "Remove accounts_byname cache"

Documentation/cmd-show-caches.txt

@@ -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%     |

Documentation/rest-api-config.txt

@@ -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",

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

@@ -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;
   }
@@ -140,10 +136,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);

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

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

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

@@ -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);
+      ExternalId extId = externalIds.get(ExternalId.Key.create(SCHEME_USERNAME, username));
-      return id != null && id.isPresent() ? getOrNull(id.get()) : null;
+      if (extId == null) {
-    } catch (ExecutionException e) {
+        return null;
-      log.warn("Cannot load AccountState for " + username, e);
+      }
+      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);
+        return 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;
       }
     }
 

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

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

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

@@ -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;
@@ -197,8 +194,6 @@ public class CreateAccount implements RestModifyView<TopLevelResource, AccountIn
       }
     }
 
-    accountCache.evictByUsername(username);
-
     AccountLoader loader = infoLoader.create(true);
     AccountInfo info = loader.get(id);
     loader.fill();

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

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