commit f12cb6974b557faf9739cad2ed5fd27a9709b7ee
author David Pursehouse <dpursehouse@collab.net> Wed Sep 11 23:25:02 2019 +0000
committer Gerrit Code Review <noreply-gerritcodereview@google.com> Wed Sep 11 23:25:02 2019 +0000
tree f50536b28b273c8231c50eccfb46b1287c795303
parent 0aa0a3d71cf8513f854d84f2addda22119a65689
parent a01293a142460850bf8494b37d60d2db810b2a68

Merge "AccountCacheImpl: Stop caching negative results"

java/com/google/gerrit/server/account/AccountCacheImpl.java

diff --git a/java/com/google/gerrit/server/account/AccountCacheImpl.java b/java/com/google/gerrit/server/account/AccountCacheImpl.java
index af8d8b0..ad9d009 100644
--- a/java/com/google/gerrit/server/account/AccountCacheImpl.java
+++ b/java/com/google/gerrit/server/account/AccountCacheImpl.java
@@ -59,7 +59,7 @@
     return new CacheModule() {
       @Override
       protected void configure() {
-        cache(BYID_NAME, Account.Id.class, new TypeLiteral<Optional<AccountState>>() {})
+        cache(BYID_NAME, Account.Id.class, new TypeLiteral<AccountState>() {})
             .loader(ByIdLoader.class);
 
         bind(AccountCacheImpl.class);
@@ -69,13 +69,13 @@
   }
 
   private final ExternalIds externalIds;
-  private final LoadingCache<Account.Id, Optional<AccountState>> byId;
+  private final LoadingCache<Account.Id, AccountState> byId;
   private final ExecutorService executor;
 
   @Inject
   AccountCacheImpl(
       ExternalIds externalIds,
-      @Named(BYID_NAME) LoadingCache<Account.Id, Optional<AccountState>> byId,
+      @Named(BYID_NAME) LoadingCache<Account.Id, AccountState> byId,
       @FanOutExecutor ExecutorService executor) {
     this.externalIds = externalIds;
     this.byId = byId;
@@ -85,9 +85,11 @@
   @Override
   public AccountState getEvenIfMissing(Account.Id accountId) {
     try {
-      return byId.get(accountId).orElse(missing(accountId));
+      return byId.get(accountId);
     } catch (ExecutionException e) {
-      logger.atWarning().withCause(e).log("Cannot load AccountState for %s", accountId);
+      if (!(e.getCause() instanceof AccountNotFoundException)) {
+        logger.atWarning().withCause(e).log("Cannot load AccountState for %s", accountId);
+      }
       return missing(accountId);
     }
   }
@@ -95,9 +97,11 @@
   @Override
   public Optional<AccountState> get(Account.Id accountId) {
     try {
-      return byId.get(accountId);
+      return Optional.ofNullable(byId.get(accountId));
     } catch (ExecutionException e) {
-      logger.atWarning().withCause(e).log("Cannot load AccountState for ID %s", accountId);
+      if (!(e.getCause() instanceof AccountNotFoundException)) {
+        logger.atWarning().withCause(e).log("Cannot load AccountState for %s", accountId);
+      }
       return Optional.empty();
     }
   }
@@ -107,10 +111,10 @@
     Map<Account.Id, AccountState> accountStates = new HashMap<>(accountIds.size());
     List<Callable<Optional<AccountState>>> callables = new ArrayList<>();
     for (Account.Id accountId : accountIds) {
-      Optional<AccountState> state = byId.getIfPresent(accountId);
+      AccountState state = byId.getIfPresent(accountId);
       if (state != null) {
         // The value is in-memory, so we just get the state
-        state.ifPresent(s -> accountStates.put(accountId, s));
+        accountStates.put(accountId, state);
       } else {
         // Queue up a callable so that we can load accounts in parallel
         callables.add(() -> get(accountId));
@@ -170,7 +174,7 @@
     return AccountState.forAccount(account.build());
   }
 
-  static class ByIdLoader extends CacheLoader<Account.Id, Optional<AccountState>> {
+  static class ByIdLoader extends CacheLoader<Account.Id, AccountState> {
     private final Accounts accounts;
 
     @Inject
@@ -179,12 +183,21 @@
     }
 
     @Override
-    public Optional<AccountState> load(Account.Id who) throws Exception {
+    public AccountState load(Account.Id who) throws Exception {
       try (TraceTimer timer =
           TraceContext.newTimer(
               "Loading account", Metadata.builder().accountId(who.get()).build())) {
-        return accounts.get(who);
+        return accounts
+            .get(who)
+            .orElseThrow(() -> new AccountNotFoundException(who + " not found"));
       }
     }
   }
+
+  /** Signals that the account was not found in the primary storage. */
+  private static class AccountNotFoundException extends Exception {
+    public AccountNotFoundException(String message) {
+      super(message);
+    }
+  }
 }

javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java

diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index c1e83d4..48c9995 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -225,7 +225,7 @@
 
   @Inject
   @Named("accounts")
-  private LoadingCache<Account.Id, Optional<AccountState>> accountsCache;
+  private LoadingCache<Account.Id, AccountState> accountsCache;
 
   @Inject private AccountOperations accountOperations;