Cache Registrars in memory

This replaces the memcache caching, which we think is overall a bad idea.
We load all registrars at once instead of caching each as needed, so that
the loadAllCached() methods can be cached as well, and therefore will
always produce results consistent with loadByClientIdCached()'s view of the
registrar's values. All of our prod registrars together total 300k of data
right now, so this is hardly worth optimizing further, and in any case this
will likely reduce latency even further since most requests will be
served out of memory.

While I was in the Registrar file I standardized the error messages for incorrect
password and clientId length to be the same format, and cleaned up a few
random things I noticed in the code.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=156151828

java/google/registry/export/PublishDetailReportAction.java

@@ -91,7 +91,7 @@ public final class PublishDetailReportAction implements Runnable, JsonAction {
     try {
       logger.infofmt("Publishing detail report for parameters: %s", json);
       String registrarId = getParam(json, REGISTRAR_ID_PARAM);
-      Registrar registrar = checkArgumentNotNull(Registrar.loadByClientId(registrarId),
+      Registrar registrar = checkArgumentNotNull(Registrar.loadByClientIdCached(registrarId),
           "Registrar %s not found", registrarId);
       String driveFolderId = checkArgumentNotNull(registrar.getDriveFolderId(),
           "No drive folder associated with registrar " + registrarId);

java/google/registry/export/SyncGroupMembersAction.java

@@ -113,13 +113,13 @@ public final class SyncGroupMembersAction implements Runnable {
    */
   @Override
   public void run() {
-    List<Registrar> dirtyRegistrars = Registrar
+    List<Registrar> dirtyRegistrars = FluentIterable.from(Registrar.loadAllCached())
-        .loadAllActive()
         .filter(new Predicate<Registrar>() {
           @Override
           public boolean apply(Registrar registrar) {
-            // Only grab registrars that require syncing and are of the correct type.
+            // Only grab active registrars that require syncing and are of the correct type.
-            return registrar.getContactsRequireSyncing()
+            return registrar.isActive()
+                && registrar.getContactsRequireSyncing()
                 && registrar.getType() == Registrar.Type.REAL;
           }})
         .toList();

java/google/registry/export/sheet/SyncRegistrarsSheet.java

@@ -65,7 +65,7 @@ class SyncRegistrarsSheet {
   boolean wereRegistrarsModified() {
     Cursor cursor = ofy().load().key(Cursor.createGlobalKey(SYNC_REGISTRAR_SHEET)).now();
     DateTime lastUpdateTime = (cursor == null) ? START_OF_TIME : cursor.getCursorTime();
-    for (Registrar registrar : Registrar.loadAll()) {
+    for (Registrar registrar : Registrar.loadAllCached()) {
       if (DateTimeUtils.isAtOrAfter(registrar.getLastUpdateTime(), lastUpdateTime)) {
         return true;
       }
@@ -85,7 +85,7 @@ class SyncRegistrarsSheet {
                   public int compare(Registrar left, Registrar right) {
                     return left.getClientId().compareTo(right.getClientId());
                   }
-                }.immutableSortedCopy(Registrar.loadAll()))
+                }.immutableSortedCopy(Registrar.loadAllCached()))
             .filter(
                 new Predicate<Registrar>() {
                   @Override

java/google/registry/flows/domain/DomainFlowUtils.java

@@ -230,7 +230,7 @@ public class DomainFlowUtils {
   /** Check if the registrar running the flow has access to the TLD in question. */
   public static void checkAllowedAccessToTld(String clientId, String tld)
       throws EppException {
-    if (!Registrar.loadByClientId(clientId).getAllowedTlds().contains(tld)) {
+    if (!Registrar.loadByClientIdCached(clientId).getAllowedTlds().contains(tld)) {
       throw new DomainFlowUtils.NotAuthorizedForTldException(tld);
     }
   }
@@ -434,7 +434,7 @@ public class DomainFlowUtils {
     if (isDomainPremium(domainName, priceTime)) {
       // NB: The load of the Registar object is transactionless, which means that it should hit
       // memcache most of the time.
-      if (Registrar.loadByClientId(clientId).getBlockPremiumNames()) {
+      if (Registrar.loadByClientIdCached(clientId).getBlockPremiumNames()) {
         throw new PremiumNameBlockedException();
       }
     }

java/google/registry/flows/session/LoginFlow.java

@@ -116,7 +116,7 @@ public class LoginFlow implements Flow {
       }
       serviceExtensionUrisBuilder.add(uri);
     }
-    Registrar registrar = Registrar.loadByClientId(login.getClientId());
+    Registrar registrar = Registrar.loadByClientIdCached(login.getClientId());
     if (registrar == null) {
       throw new BadRegistrarClientIdException(login.getClientId());
     }

java/google/registry/model/registrar/Registrar.java

@@ -16,7 +16,6 @@ package google.registry.model.registrar;
 
 import static com.google.common.base.MoreObjects.firstNonNull;
 import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Predicates.equalTo;
 import static com.google.common.base.Predicates.in;
 import static com.google.common.base.Predicates.notNull;
@@ -26,6 +25,7 @@ import static com.google.common.collect.Sets.immutableEnumSet;
 import static com.google.common.io.BaseEncoding.base64;
 import static google.registry.config.RegistryConfig.getDefaultRegistrarReferralUrl;
 import static google.registry.config.RegistryConfig.getDefaultRegistrarWhoisServer;
+import static google.registry.model.CacheUtils.memoizeWithShortExpiration;
 import static google.registry.model.common.EntityGroupRoot.getCrossTldKey;
 import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.model.ofy.Ofy.RECOMMENDED_MEMCACHE_EXPIRATION;
@@ -45,6 +45,7 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedSet;
+import com.google.common.collect.Range;
 import com.google.common.collect.Sets;
 import com.google.re2j.Pattern;
 import com.googlecode.objectify.Key;
@@ -200,6 +201,27 @@ public class Registrar extends ImmutableObject implements Buildable, Jsonifiable
         }
       };
 
+  /**
+   * A caching {@link Supplier} of a clientId to {@link Registrar} map.
+   *
+   * <p>The supplier's get() method enters a transactionless context briefly to avoid enrolling the
+   * query inside an unrelated client-affecting transaction.
+   */
+  private static final Supplier<ImmutableMap<String, Registrar>> CACHE_BY_CLIENT_ID =
+      memoizeWithShortExpiration(new Supplier<ImmutableMap<String, Registrar>>() {
+        @Override
+        public ImmutableMap<String, Registrar> get() {
+          return ofy().doTransactionless(new Work<ImmutableMap<String, Registrar>>() {
+            @Override
+            public ImmutableMap<String, Registrar> run() {
+              ImmutableMap.Builder<String, Registrar> builder = new ImmutableMap.Builder<>();
+              for (Registrar registrar : loadAll()) {
+                builder.put(registrar.getClientId(), registrar);
+              }
+              return builder.build();
+            }});
+        }});
+
   @Parent
   Key<EntityGroupRoot> parent = getCrossTldKey();
 
@@ -306,9 +328,11 @@ public class Registrar extends ImmutableObject implements Buildable, Jsonifiable
    * @see <a href="http://www.iana.org/assignments/registrar-ids/registrar-ids.txt">Registrar IDs</a>
    */
   @Index
+  @Nullable
   Long ianaIdentifier;
 
   /** Identifier of registrar used in external billing system (e.g. Oracle). */
+  @Nullable
   Long billingIdentifier;
 
   /**
@@ -413,10 +437,12 @@ public class Registrar extends ImmutableObject implements Buildable, Jsonifiable
     return creationTime.getTimestamp();
   }
 
+  @Nullable
   public Long getIanaIdentifier() {
     return ianaIdentifier;
   }
 
+  @Nullable
   public Long getBillingIdentifier() {
     return billingIdentifier;
   }
@@ -647,7 +673,8 @@ public class Registrar extends ImmutableObject implements Buildable, Jsonifiable
     public Builder setClientId(String clientId) {
       // Client id must be [3,16] chars long. See "clIDType" in the base EPP schema of RFC 5730.
       // (Need to validate this here as there's no matching EPP XSD for validation.)
-      checkArgument(clientId.length() >= 3 && clientId.length() <= 16,
+      checkArgument(
+          Range.closed(3,  16).contains(clientId.length()),
           "Client identifier must be 3-16 characters long.");
       getInstance().clientIdentifier = clientId;
       return this;
@@ -831,8 +858,9 @@ public class Registrar extends ImmutableObject implements Buildable, Jsonifiable
 
     public Builder setPassword(String password) {
       // Passwords must be [6,16] chars long. See "pwType" in the base EPP schema of RFC 5730.
-      checkArgument(password != null && password.length() >= 6 && password.length() <= 16,
+      checkArgument(
-          "Password must be [6,16] characters long.");
+          Range.closed(6, 16).contains(nullToEmpty(password).length()),
+          "Password must be 6-16 characters long.");
       getInstance().salt = base64().encode(saltSupplier.get());
       getInstance().passwordHash = getInstance().hashPassword(password);
       return this;
@@ -862,124 +890,25 @@ public class Registrar extends ImmutableObject implements Buildable, Jsonifiable
     }
   }
 
-  /** Load a registrar entity by its client id outside of a transaction. */
+  /** Loads all registrar entities directly from Datastore. */
-  @Nullable
-  public static Registrar loadByClientId(final String clientId) {
-    checkNotNull(clientId, "Client ID cannot be null");
-    return ofy().doTransactionless(new Work<Registrar>() {
-      @Override
-      public Registrar run() {
-        return ofy().loadWithMemcache()
-            .type(Registrar.class)
-            .parent(getCrossTldKey())
-            .id(clientId)
-            .now();
-      }});
-  }
-
-  /**
-   * Load registrar entities by client id range outside of a transaction.
-   *
-   * @param clientIdStart returned registrars will have a client id greater than or equal to this
-   * @param clientIdAfterEnd returned registrars will have a client id less than this
-   * @param resultSetMaxSize the maximum number of registrar entities to be returned
-   */
-  public static Iterable<Registrar> loadByClientIdRange(
-      final String clientIdStart, final String clientIdAfterEnd, final int resultSetMaxSize) {
-    return ofy().doTransactionless(new Work<Iterable<Registrar>>() {
-      @Override
-      public Iterable<Registrar> run() {
-        return ofy().load()
-            .type(Registrar.class)
-            .filterKey(">=", Key.create(getCrossTldKey(), Registrar.class, clientIdStart))
-            .filterKey("<", Key.create(getCrossTldKey(), Registrar.class, clientIdAfterEnd))
-            .limit(resultSetMaxSize);
-      }});
-  }
-
-  /** Load a registrar entity by its name outside of a transaction. */
-  @Nullable
-  public static Registrar loadByName(final String name) {
-    return ofy().doTransactionless(new Work<Registrar>() {
-      @Override
-      public Registrar run() {
-        return ofy().load()
-            .type(Registrar.class)
-            .filter("registrarName", name)
-            .first()
-            .now();
-      }});
-  }
-
-  /**
-   * Load registrar entities by registrar name range, inclusive of the start but not the end,
-   * outside of a transaction.
-   *
-   * @param nameStart returned registrars will have a name greater than or equal to this
-   * @param nameAfterEnd returned registrars will have a name less than this
-   * @param resultSetMaxSize the maximum number of registrar entities to be returned
-   */
-  public static Iterable<Registrar> loadByNameRange(
-      final String nameStart, final String nameAfterEnd, final int resultSetMaxSize) {
-    return ofy().doTransactionless(new Work<Iterable<Registrar>>() {
-      @Override
-      public Iterable<Registrar> run() {
-        return ofy().load()
-            .type(Registrar.class)
-            .filter("registrarName >=", nameStart)
-            .filter("registrarName <", nameAfterEnd)
-            .limit(resultSetMaxSize);
-      }});
-  }
-
-  /**
-   * Load registrar entities by IANA identifier range outside of a transaction.
-   *
-   * @param ianaIdentifierStart returned registrars will have an IANA id greater than or equal to
-   *        this
-   * @param ianaIdentifierAfterEnd returned registrars will have an IANA id less than this
-   * @param resultSetMaxSize the maximum number of registrar entities to be returned
-   */
-  public static Iterable<Registrar> loadByIanaIdentifierRange(
-      final Long ianaIdentifierStart,
-      final Long ianaIdentifierAfterEnd,
-      final int resultSetMaxSize) {
-    return ofy().doTransactionless(new Work<Iterable<Registrar>>() {
-      @Override
-      public Iterable<Registrar> run() {
-        return ofy().load()
-            .type(Registrar.class)
-            .filter("ianaIdentifier >=", ianaIdentifierStart)
-            .filter("ianaIdentifier <", ianaIdentifierAfterEnd)
-            .limit(resultSetMaxSize);
-      }});
-  }
-
-  /** Loads all registrar entities. */
   public static Iterable<Registrar> loadAll() {
-    return ofy().load().type(Registrar.class).ancestor(getCrossTldKey());
+    return ImmutableList.copyOf(ofy().load().type(Registrar.class).ancestor(getCrossTldKey()));
   }
 
-  /** Loads all active registrar entities. */
+  /** Loads all registrar entities using an in-memory cache. */
-  public static FluentIterable<Registrar> loadAllActive() {
+  public static Iterable<Registrar> loadAllCached() {
-    return FluentIterable.from(loadAll()).filter(IS_ACTIVE);
+    return CACHE_BY_CLIENT_ID.get().values();
   }
 
-  private static final Predicate<Registrar> IS_ACTIVE = new Predicate<Registrar>() {
+  /** Load a registrar entity by its client id directly from Datastore. */
-    @Override
+  @Nullable
-    public boolean apply(Registrar registrar) {
+  public static Registrar loadByClientId(String clientId) {
-      return registrar.isActive();
+    return ofy().load().type(Registrar.class).parent(getCrossTldKey()).id(clientId).now();
-    }};
-
-  /** Loads all active registrar entities. */
-  public static FluentIterable<Registrar> loadAllActiveAndPubliclyVisible() {
-    return FluentIterable.from(loadAll()).filter(IS_ACTIVE_AND_PUBLICLY_VISIBLE);
   }
 
-  private static final Predicate<Registrar> IS_ACTIVE_AND_PUBLICLY_VISIBLE =
+  /** Load a registrar entity by its client id using an in-memory cache. */
-      new Predicate<Registrar>() {
+  @Nullable
-        @Override
+  public static Registrar loadByClientIdCached(String clientId) {
-        public boolean apply(Registrar registrar) {
+    return CACHE_BY_CLIENT_ID.get().get(clientId);
-          return registrar.isActiveAndPubliclyVisible();
+  }
-        }};
 }

java/google/registry/rdap/RdapEntityAction.java

@@ -15,12 +15,13 @@
 package google.registry.rdap;
 
 import static google.registry.model.ofy.ObjectifyService.ofy;
+import static google.registry.rdap.RdapUtils.getRegistrarByIanaIdentifier;
 import static google.registry.request.Action.Method.GET;
 import static google.registry.request.Action.Method.HEAD;
 
 import com.google.common.base.Optional;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Iterables;
+import com.google.common.primitives.Longs;
 import com.google.re2j.Pattern;
 import com.googlecode.objectify.Key;
 import google.registry.model.contact.ContactResource;
@@ -96,18 +97,14 @@ public class RdapEntityAction extends RdapActionBase {
             OutputDataType.FULL);
       }
     }
-    try {
+    Long ianaIdentifier = Longs.tryParse(pathSearchString);
-      Long ianaIdentifier = Long.parseLong(pathSearchString);
+    if (ianaIdentifier != null) {
       wasValidKey = true;
-      Registrar registrar = Iterables.getOnlyElement(
+      Optional<Registrar> registrar = getRegistrarByIanaIdentifier(ianaIdentifier);
-          Registrar.loadByIanaIdentifierRange(ianaIdentifier, ianaIdentifier + 1, 1), null);
+      if ((registrar.isPresent()) && registrar.get().isActiveAndPubliclyVisible()) {
-      if ((registrar != null) && registrar.isActiveAndPubliclyVisible()) {
         return rdapJsonFormatter.makeRdapJsonForRegistrar(
-            registrar, true, rdapLinkBase, rdapWhoisServer, now, OutputDataType.FULL);
+            registrar.get(), true, rdapLinkBase, rdapWhoisServer, now, OutputDataType.FULL);
       }
-    } catch (NumberFormatException e) {
-      // Although the search string was not a valid IANA identifier, it might still have been a
-      // valid ROID.
     }
     // At this point, we have failed to find either a contact or a registrar.
     throw wasValidKey

java/google/registry/rdap/RdapEntitySearchAction.java

@@ -16,14 +16,18 @@ package google.registry.rdap;
 
 import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.rdap.RdapIcannStandardInformation.TRUNCATION_NOTICES;
+import static google.registry.rdap.RdapUtils.getRegistrarByIanaIdentifier;
 import static google.registry.request.Action.Method.GET;
 import static google.registry.request.Action.Method.HEAD;
 import static google.registry.util.DateTimeUtils.END_OF_TIME;
 
 import com.google.common.base.Optional;
+import com.google.common.base.Predicate;
+import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.primitives.Booleans;
+import com.google.common.primitives.Longs;
 import com.googlecode.objectify.Key;
 import google.registry.config.RegistryConfig.Config;
 import google.registry.model.contact.ContactResource;
@@ -138,19 +142,16 @@ public class RdapEntitySearchAction extends RdapActionBase {
       throw new UnprocessableEntityException("Suffixes not allowed in entity name searches");
     }
     // Get the registrar matches, depending on whether there's a wildcard.
-    ImmutableList<Registrar> registrarMatches;
+    ImmutableList<Registrar> registrarMatches =
-    if (!partialStringQuery.getHasWildcard()) {
+        FluentIterable.from(Registrar.loadAllCached())
-      Registrar registrar = Registrar.loadByName(partialStringQuery.getInitialString());
+            .filter( 
-      registrarMatches = (registrar == null)
+                new Predicate<Registrar>() {
-          ? ImmutableList.<Registrar>of()
+                  @Override
-          : ImmutableList.of(registrar);
+                  public boolean apply(Registrar registrar) {
-    } else {
+                    return partialStringQuery.matches(registrar.getRegistrarName());
-      // Fetch an additional registrar, so we can detect result set truncation.
+                  }})
-      registrarMatches = ImmutableList.copyOf(Registrar.loadByNameRange(
+            .limit(rdapResultSetMaxSize + 1)
-          partialStringQuery.getInitialString(),
+            .toList();
-          partialStringQuery.getNextInitialString(),
-          rdapResultSetMaxSize + 1));
-    }
     // Get the contact matches and return the results, fetching an additional contact to detect
     // truncation.
     return makeSearchResults(
@@ -169,7 +170,8 @@ public class RdapEntitySearchAction extends RdapActionBase {
           .type(ContactResource.class)
           .id(partialStringQuery.getInitialString())
           .now();
-      ImmutableList<Registrar> registrars = getMatchingRegistrars(partialStringQuery);
+      ImmutableList<Registrar> registrars = 
+          getMatchingRegistrars(partialStringQuery.getInitialString());
       return makeSearchResults(
           ((contactResource == null) || !contactResource.getDeletionTime().isEqual(END_OF_TIME))
               ? ImmutableList.<ContactResource>of() : ImmutableList.of(contactResource),
@@ -201,17 +203,15 @@ public class RdapEntitySearchAction extends RdapActionBase {
   }
 
   /** Looks up registrars by handle (i.e. IANA identifier). */
-  private ImmutableList<Registrar>
+  private ImmutableList<Registrar> getMatchingRegistrars(final String ianaIdentifierString) {
-      getMatchingRegistrars(final RdapSearchPattern partialStringQuery) {
+    Long ianaIdentifier = Longs.tryParse(ianaIdentifierString);
-    Long ianaIdentifier;
+    if (ianaIdentifier != null) {
-    try {
+      Optional<Registrar> registrar = getRegistrarByIanaIdentifier(ianaIdentifier);
-      ianaIdentifier = Long.parseLong(partialStringQuery.getInitialString());
+      if (registrar.isPresent()) {
-    } catch (NumberFormatException e) {
+        return ImmutableList.of(registrar.get());
-      return ImmutableList.of();
+      }
     }
-    // Fetch an additional registrar to detect result set truncation.
+    return ImmutableList.of();
-    return ImmutableList.copyOf(Registrar.loadByIanaIdentifierRange(
-        ianaIdentifier, ianaIdentifier + 1, rdapResultSetMaxSize + 1));
   }
 
   /** Builds a JSON array of entity info maps based on the specified contacts and registrars. */

java/google/registry/rdap/RdapUtils.java

@@ -0,0 +1,38 @@
+// Copyright 2017 The Nomulus Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package google.registry.rdap;
+
+import static com.google.common.collect.Iterables.tryFind;
+
+import com.google.common.base.Optional;
+import com.google.common.base.Predicate;
+import google.registry.model.registrar.Registrar;
+
+/** Utility functions for RDAP. */
+public final class RdapUtils {
+
+  private RdapUtils() {}
+
+  /** Looks up a registrar by its IANA identifier. */
+  static Optional<Registrar> getRegistrarByIanaIdentifier(final long ianaIdentifier) {
+    return tryFind(
+        Registrar.loadAllCached(),
+        new Predicate<Registrar>() {
+          @Override
+          public boolean apply(Registrar registrar) {
+            return registrar.getIanaIdentifier() == ianaIdentifier;
+          }});
+  }
+}

java/google/registry/rde/RdeStagingMapper.java

@@ -62,7 +62,7 @@ public final class RdeStagingMapper extends Mapper<EppResource, PendingDeposit,
     // emitted from the mapper. Without this, a cursor might never advance because no EppResource
     // entity exists at the watermark.
     if (resource == null) {
-      for (Registrar registrar : Registrar.loadAll()) {
+      for (Registrar registrar : Registrar.loadAllCached()) {
         DepositFragment fragment = marshaller.marshalRegistrar(registrar);
         for (PendingDeposit pending : pendings.values()) {
           emit(pending, fragment);

java/google/registry/rde/imports/RdeImportUtils.java

@@ -163,7 +163,7 @@ public class RdeImportUtils {
         // validate that all registrars exist
         while (parser.nextRegistrar()) {
           XjcRdeRegistrar registrar = parser.getRegistrar();
-          if (Registrar.loadByClientId(registrar.getId()) == null) {
+          if (Registrar.loadByClientIdCached(registrar.getId()) == null) {
             throw new IllegalArgumentException(
                 String.format("Registrar '%s' not found in the registry", registrar.getId()));
           }

java/google/registry/tmch/LordnTask.java

@@ -151,7 +151,7 @@ public class LordnTask {
   /** Retrieves the IANA identifier for a registrar based on the client id. */
   private static String getIanaIdentifier(String clientId) {
      Registrar registrar = checkNotNull(
-         Registrar.loadByClientId(clientId),
+         Registrar.loadByClientIdCached(clientId),
          "No registrar found for client id: %s", clientId);
     // Return the string "null" for null identifiers, since some Registrar.Types such as OTE will
     // have null iana ids.

java/google/registry/tools/VerifyOteCommand.java

@@ -23,6 +23,7 @@ import com.beust.jcommander.Parameter;
 import com.beust.jcommander.Parameters;
 import com.google.common.base.Function;
 import com.google.common.base.Strings;
+import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import google.registry.config.RegistryEnvironment;
@@ -103,10 +104,13 @@ final class VerifyOteCommand implements ServerSideCommand {
    * prefixes of those accounts (in this case, regname).
    */
   private ImmutableSet<String> getAllRegistrarNames() {
-    return Registrar.loadAllActive()
+    return FluentIterable.from(Registrar.loadAll())
       .transform(new Function<Registrar, String>() {
           @Override
           public String apply(Registrar registrar) {
+            if (!registrar.isActive()) {
+              return null;
+            }
             String name = registrar.getClientId();
             // Look for names of the form "regname-1", "regname-2", etc. and strip the -# suffix.
             String replacedName = name.replaceFirst("^(.*)-[1234]$", "$1");

java/google/registry/ui/server/registrar/ConsoleUiAction.java

@@ -115,7 +115,7 @@ public final class ConsoleUiAction implements Runnable {
               .render());
       return;
     }
-    Registrar registrar = Registrar.loadByClientId(sessionUtils.getRegistrarClientId(req));
+    Registrar registrar = Registrar.loadByClientIdCached(sessionUtils.getRegistrarClientId(req));
     data.put(
         "xsrfToken",
         xsrfTokenManager.generateToken(userService.getCurrentUser().getEmail()));

java/google/registry/ui/server/registrar/SessionUtils.java

@@ -122,7 +122,9 @@ public class SessionUtils {
     if (contact == null) {
       return Optional.absent();
     }
-    Optional<Registrar> result = Optional.fromNullable(ofy().load().key(contact.getParent()).now());
+    String registrarClientId = contact.getParent().getName();
+    Optional<Registrar> result =
+        Optional.fromNullable(Registrar.loadByClientIdCached(registrarClientId));
     if (!result.isPresent()) {
       logger.severefmt(
           "A contact record exists for non-existent registrar: %s.", Key.create(contact));
@@ -132,7 +134,7 @@ public class SessionUtils {
 
   /** @see #hasAccessToRegistrar(Registrar, String) */
   private static boolean hasAccessToRegistrar(String clientId, final String gaeUserId) {
-    Registrar registrar = Registrar.loadByClientId(clientId);
+    Registrar registrar = Registrar.loadByClientIdCached(clientId);
     if (registrar == null) {
       logger.warningfmt("Registrar '%s' disappeared from Datastore!", clientId);
       return false;

java/google/registry/whois/RegistrarLookupCommand.java

@@ -25,6 +25,7 @@ import com.google.common.base.CharMatcher;
 import com.google.common.base.Joiner;
 import com.google.common.base.Splitter;
 import com.google.common.base.Supplier;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import google.registry.model.registrar.Registrar;
 import google.registry.util.FormattingLogger;
@@ -48,10 +49,9 @@ final class RegistrarLookupCommand implements WhoisCommand {
         @Override
         public Map<String, Registrar> get() {
           Map<String, Registrar> map = new HashMap<>();
-          // Use the normalized registrar name as a key.
+          // Use the normalized registrar name as a key, and ignore inactive and hidden registrars.
-          Iterable<Registrar> registrars = Registrar.loadAllActiveAndPubliclyVisible();
+          for (Registrar registrar : Registrar.loadAllCached()) {
-          for (Registrar registrar : registrars) {
+            if (!registrar.isActiveAndPubliclyVisible() || registrar.getRegistrarName() == null) {
-            if (registrar.getRegistrarName() == null) {
               continue;
             }
             String normalized = normalizeRegistrarName(registrar.getRegistrarName());
@@ -65,7 +65,7 @@ final class RegistrarLookupCommand implements WhoisCommand {
           // if there isn't already a mapping for this string, so that if there's a registrar with a
           // two word name (Go Daddy) and no business-type suffix and another registrar with just
           // that first word as its name (Go), the latter will win.
-          for (Registrar registrar : registrars) {
+          for (Registrar registrar : ImmutableList.copyOf(map.values())) {
             if (registrar.getRegistrarName() == null) {
               continue;
             }

javatests/google/registry/model/registrar/RegistrarTest.java

@@ -142,19 +142,19 @@ public class RegistrarTest extends EntityTestCase {
 
   @Test
   public void testFailure_passwordNull() throws Exception {
-    thrown.expect(IllegalArgumentException.class, "Password must be [6,16] characters long.");
+    thrown.expect(IllegalArgumentException.class, "Password must be 6-16 characters long.");
     new Registrar.Builder().setPassword(null);
   }
 
   @Test
   public void testFailure_passwordTooShort() throws Exception {
-    thrown.expect(IllegalArgumentException.class, "Password must be [6,16] characters long.");
+    thrown.expect(IllegalArgumentException.class, "Password must be 6-16 characters long.");
     new Registrar.Builder().setPassword("abcde");
   }
 
   @Test
   public void testFailure_passwordTooLong() throws Exception {
-    thrown.expect(IllegalArgumentException.class, "Password must be [6,16] characters long.");
+    thrown.expect(IllegalArgumentException.class, "Password must be 6-16 characters long.");
     new Registrar.Builder().setPassword("abcdefghijklmnopq");
   }
 
@@ -397,11 +397,11 @@ public class RegistrarTest extends EntityTestCase {
   }
 
   @Test
-  public void testLoadByClientId_isTransactionless() {
+  public void testLoadByClientIdCached_isTransactionless() {
     ofy().transact(new VoidWork() {
       @Override
       public void vrun() {
-        assertThat(Registrar.loadByClientId("registrar")).isNotNull();
+        assertThat(Registrar.loadByClientIdCached("registrar")).isNotNull();
         // Load something as a control to make sure we are seeing loaded keys in the session cache.
         ofy().load().entity(abuseAdminContact).now();
         assertThat(ofy().getSessionKeys()).contains(Key.create(abuseAdminContact));
@@ -409,7 +409,7 @@ public class RegistrarTest extends EntityTestCase {
       }});
     ofy().clearSessionCache();
     // Conversely, loads outside of a transaction should end up in the session cache.
-    assertThat(Registrar.loadByClientId("registrar")).isNotNull();
+    assertThat(Registrar.loadByClientIdCached("registrar")).isNotNull();
     assertThat(ofy().getSessionKeys()).contains(Key.create(registrar));
   }
 }