Instrument proxy frontend latency metric (#320 )

* Instrument proxy frontend latency metric Backend latency only captures the time spent waiting for the GAE backend to respond to a request, which is not representitive of what clients experience. For instance it does not take into account the time the proxy spends on processing the requests and the time it takes to send a response to the client. This PR adds a metric for frontend latency, which is the time spent on answering a client request. This should serve as a better proxy for the latency clients observe. * Rename AbstractMetrics to BaseMetrics
Restrict "Public Domain" license acceptance (#329 )
2026-07-08 00:56:53 +00:00 · 2019-10-29 16:02:47 -04:00 · 2019-10-28 13:32:42 -04:00 · 2019-10-25 13:54:47 -04:00 · 2019-10-25 13:25:01 -04:00 · 2019-10-25 12:40:43 -04:00
57 changed files with 1016 additions and 242 deletions
@@ -16,7 +16,7 @@ com.jcraft:jsch:0.1.54
 com.jcraft:jzlib:1.1.1
 com.netflix.nebula:gradle-lint-plugin:10.4.2
 com.netflix.nebula:nebula-gradle-interop:1.0.11
-com.netflix.nebula:nebula-test:7.3.0
+com.netflix.nebula:nebula-test:7.4.0
 commons-codec:commons-codec:1.9
 commons-io:commons-io:2.5
 commons-lang:commons-lang:2.6
@@ -41,7 +41,7 @@ org.apache.maven:maven-builder-support:3.6.2
 org.apache.maven:maven-model-builder:3.6.2
 org.apache.maven:maven-model:3.6.2
 org.codehaus.gpars:gpars:1.2.1
-org.codehaus.groovy:groovy-all:2.4.9
+org.codehaus.groovy:groovy-all:2.4.15
 org.codehaus.groovy:groovy-ant:2.1.8
 org.codehaus.groovy:groovy-groovydoc:2.1.8
 org.codehaus.groovy:groovy-templates:2.1.8
@@ -65,4 +65,4 @@ org.multiverse:multiverse-core:0.7.0
 org.objenesis:objenesis:2.4
 org.ow2.asm:asm:7.0
 org.slf4j:slf4j-api:1.7.2
-org.spockframework:spock-core:1.1-groovy-2.4-rc-4
+org.spockframework:spock-core:1.3-groovy-2.4
@@ -0,0 +1,16 @@
+## Summary
+
+This folder contains configuration files for the gradle-license-report plugin:
+
+*   allowed_licenses.json declares the acceptable licenses. A license may have
+    multiple entries in this file, since the 'moduleLicense' property value must
+    match exactly the phrases found in pom or manifest files.
+*   license_normalizer_bundle.json configures normalization rules for license
+    reporting.
+
+## Notes About Adding New Licenses
+
+*   The WTFPL license is not allowed.
+
+*   Each 'Public Domain' license entry must include a specific 'moduleName'. Do
+    not omit moduleName or use wildcards.
@@ -3,6 +3,9 @@
    {
      "moduleLicense": "Apache Software License, Version 1.1"
    },
+    {
+      "moduleLicense": "Apache Software License, version 1.1"
+    },
    {
      "moduleLicense": "Apache 2"
    },
@@ -90,6 +93,9 @@
    {
      "moduleLicense": "The BSD License"
    },
+    {
+      "moduleLicense": "The New BSD License"
+    },
    {
      "moduleLicense": "The PostgreSQL License"
    },
@@ -198,6 +204,9 @@
    {
      "moduleLicense": "The MIT license"
    },
+    {
+      "moduleLicense": "The MIT License (MIT)"
+    },
    {
      "moduleLicense": "The PostgreSQL License"
    },
@@ -205,10 +214,12 @@
      "moduleLicense": "Mozilla Public License Version 2.0"
    },
    {
-      "moduleLicense": "Public Domain"
+      "moduleLicense": "Public Domain",
+      "moduleName": "aopalliance:aopalliance"
    },
    {
-      "moduleLicense": "PUBLIC DOMAIN"
+      "moduleLicense": "Public Domain",
+      "moduleName": "org.tukaani:xz"
    },
    {
      "moduleLicense": "The W3C Software License"
@@ -192,7 +192,7 @@ dependencies {
  compile deps['com.jcraft:jsch']
  testCompile deps['com.thoughtworks.qdox:qdox']
  compile deps['dnsjava:dnsjava']
-  runtime deps['org.glassfish.jaxb:jaxb-runtime']
+  testCompile deps['io.github.classgraph:classgraph']
  testCompile deps['javax.annotation:jsr250-api']
  compile deps['javax.inject:javax.inject']
  compile deps['javax.mail:mail']
@@ -218,6 +218,7 @@ dependencies {
  compile deps['org.bouncycastle:bcpg-jdk15on']
  testCompile deps['org.bouncycastle:bcpkix-jdk15on']
  compile deps['org.bouncycastle:bcprov-jdk15on']
+  runtime deps['org.glassfish.jaxb:jaxb-runtime']
  compile deps['org.joda:joda-money']
  compile deps['org.json:json']
  testCompile deps['org.mortbay.jetty:jetty']
@@ -606,6 +607,15 @@ task outcastTest(type: FilteringTest) {
  maxParallelForks 5
 }

+// Dedicated test suite for schema-dependent tests.
+task sqlIntegrationTest(type: Test) {
+  include 'google/registry/schema/integration/SqlIntegrationTestSuite.*'
+  // Copied from FilteringTest. Not inheriting b/c it excludes TestSuites.
+  if (project.testFilter) {
+    testNameIncludePatterns = project.testFilter.split(',')
+  }
+}
+
 task findGoldenImages(type: JavaExec) {
  classpath = sourceSets.test.runtimeClasspath
  main = 'google.registry.webdriver.GoldenImageFinder'
@@ -130,6 +130,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -129,6 +129,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -17,6 +17,7 @@ package google.registry.model.registry;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static google.registry.model.transaction.TransactionManagerFactory.jpaTm;

+import com.google.common.collect.ImmutableList;
 import google.registry.schema.domain.RegistryLock;
 import javax.persistence.EntityManager;

@@ -45,8 +46,23 @@ public final class RegistryLockDao {
            });
  }

-  public static void save(RegistryLock registryLock) {
+  public static ImmutableList<RegistryLock> getByRegistrarId(String registrarId) {
+    return jpaTm()
+        .transact(
+            () ->
+                ImmutableList.copyOf(
+                    jpaTm()
+                        .getEntityManager()
+                        .createQuery(
+                            "SELECT lock FROM RegistryLock lock WHERE"
+                                + " lock.registrarId = :registrarId",
+                            RegistryLock.class)
+                        .setParameter("registrarId", registrarId)
+                        .getResultList()));
+  }
+
+  public static RegistryLock save(RegistryLock registryLock) {
    checkNotNull(registryLock, "Null registry lock cannot be saved");
-    jpaTm().transact(() -> jpaTm().getEntityManager().persist(registryLock));
+    return jpaTm().transact(() -> jpaTm().getEntityManager().merge(registryLock));
  }
 }
@@ -14,8 +14,15 @@

 package google.registry.model.transaction;

+import static google.registry.config.RegistryEnvironment.ALPHA;
+import static google.registry.config.RegistryEnvironment.CRASH;
+
+import com.google.appengine.api.utils.SystemProperty;
+import com.google.appengine.api.utils.SystemProperty.Environment.Value;
 import com.google.common.annotations.VisibleForTesting;
+import google.registry.config.RegistryEnvironment;
 import google.registry.model.ofy.DatastoreTransactionManager;
+import google.registry.persistence.DaggerPersistenceComponent;

 /** Factory class to create {@link TransactionManager} instance. */
 // TODO: Rename this to PersistenceFactory and move to persistence package.
@@ -27,19 +34,11 @@ public class TransactionManagerFactory {
  private TransactionManagerFactory() {}

  private static JpaTransactionManager createJpaTransactionManager() {
-    // TODO(shicong): There is currently no environment where we want to create a real JPA
-    // transaction manager here.  The unit tests that require one are all set up using
-    // JpaTransactionManagerRule which launches its own PostgreSQL instance.  When we actually have
-    // PostgreSQL tables in production, ensure that all of the test environments are set up
-    // correctly and restore the code that creates a JpaTransactionManager when
-    // RegistryEnvironment.get() != UNITTEST.
-    //
-    // We removed the original code because it didn't work in sandbox (due to the absence of the
-    // persistence.xml file, which has since been added), and then (after adding this) didn't work
-    // in crash because the postgresql password hadn't been set up.  Prior to restoring, we'll need
-    // to do setup in all environments, and we probably only want to do this once we're actually
-    // using Cloud SQL for one of the new tables.
-    return DummyJpaTransactionManager.create();
+    if (shouldEnableJpaTm() && isInAppEngine()) {
+      return DaggerPersistenceComponent.create().appEngineJpaTransactionManager();
+    } else {
+      return DummyJpaTransactionManager.create();
+    }
  }

  private static TransactionManager createTransactionManager() {
@@ -54,8 +53,27 @@ public class TransactionManagerFactory {
   * {@link google.registry.tools.RegistryCli} to initialize jpaTm.
   */
  public static void initForTool() {
-    // TODO(shicong): Uncomment the line below when we set up Cloud SQL instance in all environments
-    // jpaTm = DaggerPersistenceComponent.create().nomulusToolJpaTransactionManager();
+    if (shouldEnableJpaTm()) {
+      jpaTm = DaggerPersistenceComponent.create().nomulusToolJpaTransactionManager();
+    }
+  }
+
+  // TODO(shicong): Enable JpaTm for all environments and remove this function
+  private static boolean shouldEnableJpaTm() {
+    return RegistryEnvironment.get() == ALPHA || RegistryEnvironment.get() == CRASH;
+  }
+
+  /**
+   * This function uses App Engine API to determine if the current runtime environment is App
+   * Engine.
+   *
+   * @see <a
+   *     href="https://cloud.google.com/appengine/docs/standard/java/javadoc/com/google/appengine/api/utils/SystemProperty">App
+   *     Engine API public doc</a>
+   */
+  private static boolean isInAppEngine() {
+    // SystemProperty.environment.value() returns null if the current runtime is local JVM
+    return SystemProperty.environment.value() == Value.Production;
  }

  /** Returns {@link TransactionManager} instance. */
@@ -51,6 +51,10 @@ public class PersistenceModule {
  public static final String HIKARI_MAXIMUM_POOL_SIZE = "hibernate.hikari.maximumPoolSize";
  public static final String HIKARI_IDLE_TIMEOUT = "hibernate.hikari.idleTimeout";

+  public static final String HIKARI_DS_SOCKET_FACTORY = "hibernate.hikari.dataSource.socketFactory";
+  public static final String HIKARI_DS_CLOUD_SQL_INSTANCE =
+      "hibernate.hikari.dataSource.cloudSqlInstance";
+
  @Provides
  @DefaultHibernateConfigs
  public static ImmutableMap<String, String> providesDefaultDatabaseConfigs() {
@@ -79,27 +83,31 @@ public class PersistenceModule {
    return properties.build();
  }

+  @Provides
+  @Singleton
+  @PartialCloudSqlConfigs
+  public static ImmutableMap<String, String> providesPartialCloudSqlConfigs(
+      @Config("cloudSqlJdbcUrl") String jdbcUrl,
+      @Config("cloudSqlInstanceConnectionName") String instanceConnectionName,
+      @DefaultHibernateConfigs ImmutableMap<String, String> defaultConfigs) {
+    HashMap<String, String> overrides = Maps.newHashMap(defaultConfigs);
+    overrides.put(Environment.URL, jdbcUrl);
+    overrides.put(HIKARI_DS_SOCKET_FACTORY, "com.google.cloud.sql.postgres.SocketFactory");
+    overrides.put(HIKARI_DS_CLOUD_SQL_INSTANCE, instanceConnectionName);
+    return ImmutableMap.copyOf(overrides);
+  }
+
  @Provides
  @Singleton
  @AppEngineJpaTm
  public static JpaTransactionManager providesAppEngineJpaTm(
-      @Config("cloudSqlJdbcUrl") String jdbcUrl,
      @Config("cloudSqlUsername") String username,
-      @Config("cloudSqlInstanceConnectionName") String instanceConnectionName,
      KmsKeyring kmsKeyring,
-      @DefaultHibernateConfigs ImmutableMap<String, String> defaultConfigs,
+      @PartialCloudSqlConfigs ImmutableMap<String, String> cloudSqlConfigs,
      Clock clock) {
-    HashMap<String, String> overrides = Maps.newHashMap(defaultConfigs);
-
-    // For Java users, the Cloud SQL JDBC Socket Factory can provide authenticated connections.
-    // See https://github.com/GoogleCloudPlatform/cloud-sql-jdbc-socket-factory for details.
-    overrides.put("socketFactory", "com.google.cloud.sql.postgres.SocketFactory");
-    overrides.put("cloudSqlInstance", instanceConnectionName);
-
-    overrides.put(Environment.URL, jdbcUrl);
+    HashMap<String, String> overrides = Maps.newHashMap(cloudSqlConfigs);
    overrides.put(Environment.USER, username);
    overrides.put(Environment.PASS, kmsKeyring.getCloudSqlPassword());
-
    return new JpaTransactionManagerImpl(create(overrides), clock);
  }

@@ -107,27 +115,13 @@ public class PersistenceModule {
  @Singleton
  @NomulusToolJpaTm
  public static JpaTransactionManager providesNomulusToolJpaTm(
-      @Config("toolsCloudSqlJdbcUrl") String jdbcUrl,
      @Config("toolsCloudSqlUsername") String username,
-      @Config("cloudSqlInstanceConnectionName") String instanceConnectionName,
      KmsKeyring kmsKeyring,
-      @DefaultHibernateConfigs ImmutableMap<String, String> defaultConfigs,
+      @PartialCloudSqlConfigs ImmutableMap<String, String> cloudSqlConfigs,
      Clock clock) {
-
-    // Cloud SQL JDBC Socket Factory library requires the jdbc url to include all connection
-    // information, otherwise the connection initialization will fail. See here for more details:
-    // https://github.com/GoogleCloudPlatform/cloud-sql-jdbc-socket-factory
-    String fullJdbcUrl =
-        new StringBuilder(jdbcUrl)
-            .append("?cloudSqlInstance=" + instanceConnectionName)
-            .append("&socketFactory=com.google.cloud.sql.postgres.SocketFactory")
-            .append("&user=" + username)
-            .append("&password=" + kmsKeyring.getCloudSqlPassword())
-            .toString();
-
-    HashMap<String, String> overrides = Maps.newHashMap(defaultConfigs);
-    overrides.put(Environment.URL, fullJdbcUrl);
-
+    HashMap<String, String> overrides = Maps.newHashMap(cloudSqlConfigs);
+    overrides.put(Environment.USER, username);
+    overrides.put(Environment.PASS, kmsKeyring.getToolsCloudSqlPassword());
    return new JpaTransactionManagerImpl(create(overrides), clock);
  }

@@ -166,6 +160,11 @@ public class PersistenceModule {
  @Documented
  @interface NomulusToolJpaTm {}

+  /** Dagger qualifier for the partial Cloud SQL configs. */
+  @Qualifier
+  @Documented
+  @interface PartialCloudSqlConfigs {}
+
  /** Dagger qualifier for the default Hibernate configurations. */
  // TODO(shicong): Change annotations in this class to none public or put them in a top level
  //  package
@@ -39,6 +39,7 @@ import google.registry.util.Retrier;
 import google.registry.util.SendEmailService;
 import java.io.IOException;
 import java.io.InputStream;
+import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 import javax.inject.Inject;
 import javax.mail.internet.InternetAddress;
@@ -102,7 +103,7 @@ public final class IcannReportingUploadAction implements Runnable {
                  final byte[] payload = readBytesFromGcs(gcsFilename);
                  return icannReporter.send(payload, reportFilename);
                },
-                IOException.class);
+                IcannReportingUploadAction::isUploadFailureRetryable);
      } catch (RuntimeException e) {
        logger.atWarning().withCause(e).log("Upload to %s failed.", gcsFilename);
      }
@@ -115,6 +116,21 @@ public final class IcannReportingUploadAction implements Runnable {
        String.format("OK, attempted uploading %d reports", manifestedFiles.size()));
  }

+  /** Don't retry when reports are already uploaded or can't be uploaded. */
+  private static final String ICANN_UPLOAD_PERMANENT_ERROR_MESSAGE =
+      "A report for that month already exists, the cut-off date already passed.";
+
+  /** Don't retry when the IP address isn't whitelisted, as retries go through the same IP. */
+  private static final Pattern ICANN_UPLOAD_WHITELIST_ERROR =
+      Pattern.compile("Your IP address .+ is not allowed to connect");
+
+  /** Predicate to retry uploads on IOException, so long as they aren't non-retryable errors. */
+  private static boolean isUploadFailureRetryable(Throwable e) {
+    return (e instanceof IOException)
+        && !e.getMessage().contains(ICANN_UPLOAD_PERMANENT_ERROR_MESSAGE)
+        && !ICANN_UPLOAD_WHITELIST_ERROR.matcher(e.getMessage()).matches();
+  }
+
  private void emailUploadResults(ImmutableMap<String, Boolean> reportSummary) {
    String subject = String.format(
        "ICANN Monthly report upload summary: %d/%d succeeded",
@@ -65,7 +65,8 @@ import org.joda.time.DateTime;
          name = "idx_registry_lock_repo_id_revision_id",
          columnList = "repoId, revisionId",
          unique = true),
-      @Index(name = "idx_registry_lock_verification_code", columnList = "verificationCode")
+      @Index(name = "idx_registry_lock_verification_code", columnList = "verificationCode"),
+      @Index(name = "idx_registry_lock_registrar_id", columnList = "registrarId")
    })
 public final class RegistryLock extends ImmutableObject implements Buildable {

@@ -177,10 +178,7 @@ public final class RegistryLock extends ImmutableObject implements Buildable {

  @Override
  public Builder asBuilder() {
-    RegistryLock clone = clone(this);
-    // Revision ID should be different for every object
-    clone.revisionId = null;
-    return new Builder(clone);
+    return new Builder(clone(this));
  }

  /** Builder for {@link google.registry.schema.domain.RegistryLock}. */
@@ -32,7 +32,7 @@ import java.util.List;

 /** A command to upload a {@link ClaimsListShard}. */
@Parameters(separators = " =", commandDescription = "Manually upload a new claims list file")
-final class UploadClaimsListCommand extends ConfirmingCommand implements CommandWithRemoteApi {
+final class UploadClaimsListCommand extends ConfirmingCommand implements CommandWithCloudSql {

  @Parameter(description = "Claims list filename")
  private List<String> mainParameters = new ArrayList<>();
@@ -21,7 +21,9 @@ import static google.registry.util.DomainNameUtils.canonicalizeDomainName;

 import com.google.common.base.Ascii;
 import com.google.common.base.Splitter;
+import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.net.InternetDomainName;
 import com.google.re2j.Pattern;
 import google.registry.model.registrar.Registrar;
@@ -194,12 +196,10 @@ public final class RegistrarFormFields {
          FormField.named("visibleInDomainWhoisAsAbuse", Boolean.class).build();

  public static final FormField<String, String> CONTACT_PHONE_NUMBER_FIELD =
-      FormFields.PHONE_NUMBER.asBuilder()
-          .build();
+      FormFields.PHONE_NUMBER.asBuilder().build();

  public static final FormField<String, String> CONTACT_FAX_NUMBER_FIELD =
-      FormFields.PHONE_NUMBER.asBuilderNamed("faxNumber")
-          .build();
+      FormFields.PHONE_NUMBER.asBuilderNamed("faxNumber").build();

  public static final FormField<String, String> CONTACT_GAE_USER_ID_FIELD =
      FormFields.NAME.asBuilderNamed("gaeUserId").build();
@@ -217,11 +217,8 @@ public final class RegistrarFormFields {
          .asSet(Splitter.on(',').omitEmptyStrings().trimResults())
          .build();

-  public static final FormField<List<Map<String, ?>>, List<RegistrarContact.Builder>>
-      CONTACTS_FIELD = FormField.mapNamed("contacts")
-          .transform(RegistrarContact.Builder.class, RegistrarFormFields::toRegistrarContactBuilder)
-          .asList()
-          .build();
+  public static final FormField<List<Map<String, ?>>, List<Map<String, ?>>> CONTACTS_AS_MAPS =
+      FormField.mapNamed("contacts").asList().build();

  public static final FormField<List<String>, List<String>> I18N_STREET_FIELD =
      FormFields.XS_NORMALIZED_STRING.asBuilderNamed("street")
@@ -344,33 +341,60 @@ public final class RegistrarFormFields {
    }
  }

-  private static @Nullable RegistrarContact.Builder toRegistrarContactBuilder(
-      @Nullable Map<String, ?> args) {
+  public static ImmutableList<RegistrarContact.Builder> getRegistrarContactBuilders(
+      ImmutableSet<RegistrarContact> existingContacts, @Nullable Map<String, ?> args) {
    if (args == null) {
-      return null;
+      return ImmutableList.of();
    }
-    RegistrarContact.Builder builder = new RegistrarContact.Builder();
-    CONTACT_NAME_FIELD.extractUntyped(args).ifPresent(builder::setName);
-    CONTACT_EMAIL_ADDRESS_FIELD.extractUntyped(args).ifPresent(builder::setEmailAddress);
-    CONTACT_VISIBLE_IN_WHOIS_AS_ADMIN_FIELD
-        .extractUntyped(args)
-        .ifPresent(builder::setVisibleInWhoisAsAdmin);
-    CONTACT_VISIBLE_IN_WHOIS_AS_TECH_FIELD
-        .extractUntyped(args)
-        .ifPresent(builder::setVisibleInWhoisAsTech);
-    PHONE_AND_EMAIL_VISIBLE_IN_DOMAIN_WHOIS_AS_ABUSE_FIELD
-        .extractUntyped(args)
-        .ifPresent(builder::setVisibleInDomainWhoisAsAbuse);
-    CONTACT_PHONE_NUMBER_FIELD.extractUntyped(args).ifPresent(builder::setPhoneNumber);
-    CONTACT_FAX_NUMBER_FIELD.extractUntyped(args).ifPresent(builder::setFaxNumber);
-    CONTACT_TYPES.extractUntyped(args).ifPresent(builder::setTypes);
-    CONTACT_GAE_USER_ID_FIELD.extractUntyped(args).ifPresent(builder::setGaeUserId);
-    CONTACT_ALLOWED_TO_SET_REGISTRY_LOCK_PASSWORD
-        .extractUntyped(args)
-        .ifPresent(builder::setAllowedToSetRegistryLockPassword);
+    Optional<List<Map<String, ?>>> contactsAsMaps = CONTACTS_AS_MAPS.extractUntyped(args);
+    if (!contactsAsMaps.isPresent()) {
+      return ImmutableList.of();
+    }
+    ImmutableList.Builder<RegistrarContact.Builder> result = new ImmutableList.Builder<>();
+    for (Map<String, ?> contactAsMap : contactsAsMaps.get()) {
+      String emailAddress =
+          CONTACT_EMAIL_ADDRESS_FIELD
+              .extractUntyped(contactAsMap)
+              .orElseThrow(
+                  () -> new IllegalArgumentException("Contacts from UI must have email addresses"));
+      // Start with a new builder if the contact didn't previously exist
+      RegistrarContact.Builder contactBuilder =
+          existingContacts.stream()
+              .filter(rc -> rc.getEmailAddress().equals(emailAddress))
+              .findFirst()
+              .map(RegistrarContact::asBuilder)
+              .orElse(new RegistrarContact.Builder());
+      applyRegistrarContactArgs(contactBuilder, contactAsMap);
+      result.add(contactBuilder);
+    }
+    return result.build();
+  }
+
+  private static void applyRegistrarContactArgs(
+      RegistrarContact.Builder builder, Map<String, ?> args) {
+    builder.setName(CONTACT_NAME_FIELD.extractUntyped(args).orElse(null));
+    builder.setEmailAddress(CONTACT_EMAIL_ADDRESS_FIELD.extractUntyped(args).orElse(null));
+    builder.setVisibleInWhoisAsAdmin(
+        CONTACT_VISIBLE_IN_WHOIS_AS_ADMIN_FIELD.extractUntyped(args).orElse(false));
+    builder.setVisibleInWhoisAsTech(
+        CONTACT_VISIBLE_IN_WHOIS_AS_TECH_FIELD.extractUntyped(args).orElse(false));
+    builder.setVisibleInDomainWhoisAsAbuse(
+        PHONE_AND_EMAIL_VISIBLE_IN_DOMAIN_WHOIS_AS_ABUSE_FIELD.extractUntyped(args).orElse(false));
+    builder.setPhoneNumber(CONTACT_PHONE_NUMBER_FIELD.extractUntyped(args).orElse(null));
+    builder.setFaxNumber(CONTACT_FAX_NUMBER_FIELD.extractUntyped(args).orElse(null));
+    builder.setTypes(CONTACT_TYPES.extractUntyped(args).orElse(ImmutableSet.of()));
+    builder.setGaeUserId(CONTACT_GAE_USER_ID_FIELD.extractUntyped(args).orElse(null));
+    builder.setAllowedToSetRegistryLockPassword(
+        CONTACT_ALLOWED_TO_SET_REGISTRY_LOCK_PASSWORD.extractUntyped(args).orElse(false));
+
+    // Registry lock password does not need to be set every time
    CONTACT_REGISTRY_LOCK_PASSWORD_FIELD
        .extractUntyped(args)
-        .ifPresent(builder::setRegistryLockPassword);
-    return builder;
+        .ifPresent(
+            password -> {
+              if (!Strings.isNullOrEmpty(password)) {
+                builder.setRegistryLockPassword(password);
+              }
+            });
  }
 }
@@ -60,7 +60,6 @@ import google.registry.util.CollectionUtils;
 import google.registry.util.DiffUtils;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
-import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
@@ -175,8 +174,7 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
  }

  private RegistrarResult update(final Map<String, ?> args, String clientId) {
-    return tm()
-        .transact(
+    return tm().transact(
            () -> {
              // We load the registrar here rather than outside of the transaction - to make
              // sure we have the latest version. This one is loaded inside the transaction, so it's
@@ -215,7 +213,8 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
              updatedRegistrar = checkAndUpdateAdminControlledFields(updatedRegistrar, args);

              // read the contacts from the request.
-              ImmutableSet<RegistrarContact> updatedContacts = readContacts(registrar, args);
+              ImmutableSet<RegistrarContact> updatedContacts =
+                  readContacts(registrar, contacts, args);

              // Save the updated contacts
              if (!updatedContacts.equals(contacts)) {
@@ -240,11 +239,18 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
            });
  }

-  private Map<String, Object> expandRegistrarWithContacts(Iterable<RegistrarContact> contacts,
-                                                          Registrar registrar) {
+  private Map<String, Object> expandRegistrarWithContacts(
+      Iterable<RegistrarContact> contacts, Registrar registrar) {
    ImmutableSet<Map<String, Object>> expandedContacts =
        Streams.stream(contacts)
            .map(RegistrarContact::toDiffableFieldMap)
+            // Note: per the javadoc, toDiffableFieldMap includes sensitive data but we don't want
+            // to display it here
+            .peek(
+                map -> {
+                  map.remove("registryLockPasswordHash");
+                  map.remove("registryLockPasswordSalt");
+                })
            .collect(toImmutableSet());
    // Use LinkedHashMap here to preserve ordering; null values mean we can't use ImmutableMap.
    LinkedHashMap<String, Object> result = new LinkedHashMap<>(registrar.toDiffableFieldMap());
@@ -377,16 +383,10 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA

  /** Reads the contacts from the supplied args. */
  public static ImmutableSet<RegistrarContact> readContacts(
-      Registrar registrar, Map<String, ?> args) {
-
-    ImmutableSet.Builder<RegistrarContact> contacts = new ImmutableSet.Builder<>();
-    Optional<List<RegistrarContact.Builder>> builders =
-        RegistrarFormFields.CONTACTS_FIELD.extractUntyped(args);
-    if (builders.isPresent()) {
-      builders.get().forEach(c -> contacts.add(c.setParent(registrar).build()));
-    }
-
-    return contacts.build();
+      Registrar registrar, ImmutableSet<RegistrarContact> existingContacts, Map<String, ?> args) {
+    return RegistrarFormFields.getRegistrarContactBuilders(existingContacts, args).stream()
+        .map(builder -> builder.setParent(registrar).build())
+        .collect(toImmutableSet());
  }

  /**
@@ -452,8 +452,17 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
                    () ->
                        new FormException(
                            "Not allowed to set registry lock password directly on new contact"));
-        if (!existingContact.isAllowedToSetRegistryLockPassword()) {
-          throw new FormException("Registrar contact not allowed to set registry lock password");
+        if (updatedContact.isRegistryLockAllowed()) {
+          // the password must have been set before or the user was allowed to set it now
+          if (!existingContact.isAllowedToSetRegistryLockPassword()
+              && !existingContact.isRegistryLockAllowed()) {
+            throw new FormException("Registrar contact not allowed to set registry lock password");
+          }
+        }
+        if (updatedContact.isAllowedToSetRegistryLockPassword()) {
+          if (!existingContact.isAllowedToSetRegistryLockPassword()) {
+            throw new FormException("Cannot set isAllowedToSetRegistryLockPassword through UI");
+          }
        }
      }
    }
@@ -14,6 +14,7 @@

 package google.registry.model.registry;

+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.truth.Truth.assertThat;
 import static google.registry.model.transaction.TransactionManagerFactory.jpaTm;
 import static google.registry.testing.JUnitBackports.assertThrows;
@@ -22,6 +23,7 @@ import google.registry.model.transaction.JpaTransactionManagerRule;
 import google.registry.schema.domain.RegistryLock;
 import google.registry.schema.domain.RegistryLock.Action;
 import google.registry.testing.AppEngineRule;
+import java.util.Optional;
 import java.util.UUID;
 import javax.persistence.PersistenceException;
 import org.junit.Rule;
@@ -71,10 +73,10 @@ public final class RegistryLockDaoTest {
    jpaTm()
        .transact(
            () -> {
-              RegistryLock secondLock =
+              RegistryLock updatedLock =
                  RegistryLockDao.getByVerificationCode(lock.getVerificationCode());
-              secondLock.setCompletionTimestamp(jpaTmRule.getTxnClock().nowUtc());
-              RegistryLockDao.save(secondLock);
+              updatedLock.setCompletionTimestamp(jpaTmRule.getTxnClock().nowUtc());
+              RegistryLockDao.save(updatedLock);
            });
    jpaTm()
        .transact(
@@ -86,11 +88,43 @@ public final class RegistryLockDaoTest {
            });
  }

+  @Test
+  public void testUpdateLock_usingSamePrimaryKey() {
+    RegistryLock lock = RegistryLockDao.save(createLock());
+    jpaTmRule.getTxnClock().advanceOneMilli();
+    RegistryLock updatedLock =
+        lock.asBuilder().setCompletionTimestamp(jpaTmRule.getTxnClock().nowUtc()).build();
+    jpaTm().transact(() -> RegistryLockDao.save(updatedLock));
+    jpaTm()
+        .transact(
+            () -> {
+              RegistryLock fromDatabase =
+                  RegistryLockDao.getByVerificationCode(lock.getVerificationCode());
+              assertThat(fromDatabase.getCompletionTimestamp())
+                  .isEqualTo(Optional.of(jpaTmRule.getTxnClock().nowUtc()));
+            });
+  }
+
  @Test
  public void testFailure_saveNull() {
    assertThrows(NullPointerException.class, () -> RegistryLockDao.save(null));
  }

+  @Test
+  public void testLoad_byRegistrarId() {
+    RegistryLock lock = createLock();
+    RegistryLock secondLock = createLock().asBuilder().setDomainName("otherexample.test").build();
+    RegistryLockDao.save(lock);
+    RegistryLockDao.save(secondLock);
+
+    assertThat(
+            RegistryLockDao.getByRegistrarId("TheRegistrar").stream()
+                .map(RegistryLock::getDomainName)
+                .collect(toImmutableSet()))
+        .containsExactly("example.test", "otherexample.test");
+    assertThat(RegistryLockDao.getByRegistrarId("nonexistent")).isEmpty();
+  }
+
  private RegistryLock createLock() {
    return new RegistryLock.Builder()
        .setRepoId("repoId")
@@ -154,6 +154,45 @@ public class IcannReportingUploadActionTest {
                new InternetAddress("recipient@example.com"),
                new InternetAddress("sender@example.com")));
  }
+
+  @Test
+  public void testFailure_quicklySkipsOverNonRetryableUploadException() throws Exception {
+    runTest_nonRetryableException(
+        new IOException(
+            "<msg>A report for that month already exists, the cut-off date already"
+                + " passed.</msg>"));
+  }
+
+  @Test
+  public void testFailure_quicklySkipsOverIpWhitelistException() throws Exception {
+    runTest_nonRetryableException(
+        new IOException("Your IP address 25.147.130.158 is not allowed to connect"));
+  }
+
+  private void runTest_nonRetryableException(Exception nonRetryableException) throws Exception {
+    IcannReportingUploadAction action = createAction();
+    when(mockReporter.send(PAYLOAD_FAIL, "a-activity-201706.csv"))
+        .thenThrow(nonRetryableException)
+        .thenThrow(
+            new AssertionError(
+                "This should never be thrown because the previous exception isn't retryable"));
+    action.run();
+    verify(mockReporter, times(1)).send(PAYLOAD_FAIL, "a-activity-201706.csv");
+    verify(mockReporter).send(PAYLOAD_SUCCESS, "test-transactions-201706.csv");
+    verifyNoMoreInteractions(mockReporter);
+    assertThat(((FakeResponse) action.response).getPayload())
+        .isEqualTo("OK, attempted uploading 2 reports");
+    verify(emailService)
+        .sendEmail(
+            EmailMessage.create(
+                "ICANN Monthly report upload summary: 1/2 succeeded",
+                "Report Filename - Upload status:\n"
+                    + "test-transactions-201706.csv - SUCCESS\n"
+                    + "a-activity-201706.csv - FAILURE",
+                new InternetAddress("recipient@example.com"),
+                new InternetAddress("sender@example.com")));
+  }
+
  @Test
  public void testFail_FileNotFound() throws Exception {
    IcannReportingUploadAction action = createAction();
@@ -0,0 +1,81 @@
+// Copyright 2018 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.schema.integration;
+
+import static com.google.common.truth.Truth.assertWithMessage;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import com.google.common.collect.Sets.SetView;
+import google.registry.model.transaction.JpaTransactionManagerRule;
+import io.github.classgraph.ClassGraph;
+import io.github.classgraph.ScanResult;
+import java.lang.reflect.Field;
+import java.util.stream.Stream;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+import org.junit.runners.Suite.SuiteClasses;
+
+/**
+ * Verifies that all tests that depends on the Cloud SQL schema are included in the project's
+ * sqlIntegrationTest suite. Names of the test classes is set to the 'test.sqlIntergrationTests'
+ * system property as a comma-separated string.
+ *
+ * <p>A test is deemed dependent on the SQL schema iff it has a field with type {@link
+ * JpaTransactionManagerRule}.
+ */
+// TODO(weiminyu): consider generating a TestSuite class instead.
+@RunWith(JUnit4.class)
+public class SqlIntegrationMembershipTest {
+
+  @Test
+  public void sqlIntegrationMembershipComplete() {
+    ImmutableSet<String> sqlDependentTests;
+    try (ScanResult scanResult =
+        new ClassGraph()
+            .enableAnnotationInfo()
+            .whitelistPackages("google.registry")
+            .scan()) {
+      sqlDependentTests =
+          scanResult.getClassesWithAnnotation(RunWith.class.getName()).stream()
+              .filter(clazz -> clazz.getSimpleName().endsWith("Test"))
+              .map(clazz -> clazz.loadClass())
+              .filter(SqlIntegrationMembershipTest::isSqlDependent)
+              .map(Class::getName)
+              .collect(ImmutableSet.toImmutableSet());
+    }
+    ImmutableSet<String> declaredTests =
+        Stream.of(SqlIntegrationTestSuite.class.getAnnotation(SuiteClasses.class).value())
+            .map(Class::getName)
+            .collect(ImmutableSet.toImmutableSet());
+    SetView<String> undeclaredTests = Sets.difference(sqlDependentTests, declaredTests);
+    assertWithMessage(
+            "Undeclared sql-dependent tests found. "
+                + "Please add them to SqlIntegrationTestSuite.java.")
+        .that(undeclaredTests)
+        .isEmpty();
+    SetView<String> unnecessaryDeclarations = Sets.difference(declaredTests, sqlDependentTests);
+    assertWithMessage("Found tests that should not be included in SqlIntegrationTestSuite.java.")
+        .that(unnecessaryDeclarations)
+        .isEmpty();
+  }
+
+  private static boolean isSqlDependent(Class<?> testClass) {
+    return Stream.of(testClass.getDeclaredFields())
+        .map(Field::getType)
+        .anyMatch(JpaTransactionManagerRule.class::equals);
+  }
+}
@@ -0,0 +1,52 @@
+// Copyright 2018 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.schema.integration;
+
+import google.registry.model.registry.RegistryLockDaoTest;
+import google.registry.model.tmch.ClaimsListDaoTest;
+import google.registry.model.transaction.JpaTransactionManagerImplTest;
+import google.registry.model.transaction.JpaTransactionManagerRuleTest;
+import google.registry.persistence.BloomFilterConverterTest;
+import google.registry.persistence.CreateAutoTimestampConverterTest;
+import google.registry.persistence.UpdateAutoTimestampConverterTest;
+import google.registry.persistence.ZonedDateTimeConverterTest;
+import google.registry.schema.tld.PremiumListDaoTest;
+import org.junit.runner.RunWith;
+import org.junit.runners.Suite;
+import org.junit.runners.Suite.SuiteClasses;
+
+/**
+ * Groups all tests that may depends on Cloud SQL schema. They will be run for server-schema
+ * compatibility check.
+ *
+ * <p>Schema dependency is approximated by the use of {@link
+ * google.registry.model.transaction.JpaTransactionManagerRule}.
+ *
+ * @see SqlIntegrationMembershipTest
+ */
+// TODO(weiminyu): refactor JpaTransactionManagerRule to eliminate false positives.
+@RunWith(Suite.class)
+@SuiteClasses({
+  BloomFilterConverterTest.class,
+  ClaimsListDaoTest.class,
+  CreateAutoTimestampConverterTest.class,
+  JpaTransactionManagerImplTest.class,
+  JpaTransactionManagerRuleTest.class,
+  PremiumListDaoTest.class,
+  RegistryLockDaoTest.class,
+  UpdateAutoTimestampConverterTest.class,
+  ZonedDateTimeConverterTest.class
+})
+public class SqlIntegrationTestSuite {}
@@ -193,6 +193,7 @@ public final class AppEngineRule extends ExternalResource {
        .setPassword("foo-BAR2")
        .setPhoneNumber("+1.3334445555")
        .setPhonePasscode("12345")
+        .setRegistryLockAllowed(false)
        .build();
  }

@@ -206,6 +207,7 @@ public final class AppEngineRule extends ExternalResource {
        .setPassword("password2")
        .setPhoneNumber("+1.2223334444")
        .setPhonePasscode("22222")
+        .setRegistryLockAllowed(true)
        .build();
  }

@@ -241,6 +243,19 @@ public final class AppEngineRule extends ExternalResource {
        .build();
  }

+  public static RegistrarContact makeRegistrarContact3() {
+    return new RegistrarContact.Builder()
+        .setParent(makeRegistrar2())
+        .setName("Marla Singer")
+        .setEmailAddress("Marla.Singer@crr.com")
+        .setPhoneNumber("+1.2128675309")
+        .setTypes(ImmutableSet.of(RegistrarContact.Type.TECH))
+        .setGaeUserId(THE_REGISTRAR_GAE_USER_ID)
+        .setAllowedToSetRegistryLockPassword(true)
+        .setRegistryLockPassword("hi")
+        .build();
+  }
+
  /** Hack to make sure AppEngineRule is always wrapped in a TemporaryFolder rule. */
  @Override
  public Statement apply(Statement base, Description description) {
@@ -412,6 +427,10 @@ public final class AppEngineRule extends ExternalResource {
  public static void loadInitialData() {
    persistSimpleResources(
        ImmutableList.of(
-            makeRegistrar1(), makeRegistrarContact1(), makeRegistrar2(), makeRegistrarContact2()));
+            makeRegistrar1(),
+            makeRegistrarContact1(),
+            makeRegistrar2(),
+            makeRegistrarContact2(),
+            makeRegistrarContact3()));
  }
 }
@@ -21,6 +21,7 @@ import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.testing.DatastoreHelper.createTld;
 import static google.registry.testing.DatastoreHelper.loadRegistrar;

+import com.google.common.collect.ImmutableSortedSet;
 import google.registry.model.ImmutableObject;
 import google.registry.model.ofy.CommitLogManifest;
 import google.registry.model.ofy.CommitLogMutation;
@@ -62,13 +63,16 @@ public class ResaveEnvironmentEntitiesCommandTest
        transform(
            ofy().load().type(CommitLogMutation.class).list(),
            mutation -> ofy().load().fromEntity(mutation.getEntity()));
+    ImmutableSortedSet<RegistrarContact> theRegistrarContacts =
+        loadRegistrar("TheRegistrar").getContacts();
    assertThat(savedEntities)
        .containsExactly(
            // The Registrars and RegistrarContacts are created by AppEngineRule.
            loadRegistrar("TheRegistrar"),
            loadRegistrar("NewRegistrar"),
            Registry.get("tld"),
-            getOnlyElement(loadRegistrar("TheRegistrar").getContacts()),
+            theRegistrarContacts.first(),
+            theRegistrarContacts.last(),
            getOnlyElement(loadRegistrar("NewRegistrar").getContacts()));
  }

@@ -188,6 +188,33 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase {

  @Test
  public void testSuccess_setRegistryLockPassword() {
+    addPasswordToTechContact();
+    techContact = Iterables.getOnlyElement(loadRegistrar(CLIENT_ID).getContactsOfType(Type.TECH));
+    assertThat(techContact.verifyRegistryLockPassword("hi")).isTrue();
+    assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
+  }
+
+  @Test
+  public void testSuccess_setRegistryLockPassword_notOverriddenLater() {
+    addPasswordToTechContact();
+    techContact = Iterables.getOnlyElement(loadRegistrar(CLIENT_ID).getContactsOfType(Type.TECH));
+    assertThat(techContact.verifyRegistryLockPassword("hi")).isTrue();
+
+    techContact = Iterables.getOnlyElement(loadRegistrar(CLIENT_ID).getContactsOfType(Type.TECH));
+    Map<String, Object> techContactMap = techContact.toJsonMap();
+    techContactMap.put("name", "Some Other Name");
+    Map<String, Object> reqJson = loadRegistrar(CLIENT_ID).toJsonMap();
+    reqJson.put(
+        "contacts",
+        ImmutableList.of(AppEngineRule.makeRegistrarContact2().toJsonMap(), techContactMap));
+    Map<String, Object> response =
+        action.handleJsonRequest(ImmutableMap.of("op", "update", "id", CLIENT_ID, "args", reqJson));
+    assertThat(response).containsAtLeastEntriesIn(ImmutableMap.of("status", "SUCCESS"));
+    techContact = Iterables.getOnlyElement(loadRegistrar(CLIENT_ID).getContactsOfType(Type.TECH));
+    assertThat(techContact.verifyRegistryLockPassword("hi")).isTrue();
+  }
+
+  private void addPasswordToTechContact() {
    techContact =
        persistResource(techContact.asBuilder().setAllowedToSetRegistryLockPassword(true).build());
    Map<String, Object> contactMap = techContact.toJsonMap();
@@ -199,9 +226,6 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase {
    Map<String, Object> response =
        action.handleJsonRequest(ImmutableMap.of("op", "update", "id", CLIENT_ID, "args", reqJson));
    assertThat(response).containsAtLeastEntriesIn(ImmutableMap.of("status", "SUCCESS"));
-    techContact = Iterables.getOnlyElement(loadRegistrar(CLIENT_ID).getContactsOfType(Type.TECH));
-    assertThat(techContact.verifyRegistryLockPassword("hi")).isTrue();
-    assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
  }

  @Test
@@ -275,7 +299,7 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase {
            "results",
            ImmutableList.of(),
            "message",
-            "Registrar contact not allowed to set registry lock password");
+            "Cannot set isAllowedToSetRegistryLockPassword through UI");
    assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: FormException");
  }
 }
@@ -35,6 +35,7 @@ import org.junit.Test;
 import org.junit.rules.Timeout;
 import org.junit.runner.RunWith;
 import org.openqa.selenium.By;
+import org.openqa.selenium.WebElement;

 /** WebDriver tests for Registrar Console UI. */
@RunWith(RepeatableRunner.class)
@@ -78,6 +79,12 @@ public class RegistrarConsoleWebTest extends WebDriverTestCase {
    assertThat(driver.waitForElement(By.id(eltId)).isDisplayed()).isFalse();
  }

+  /** Checks that searching with the given By produces at least one element with the given text. */
+  void assertEltTextPresent(By by, String toFind) {
+    assertThat(driver.findElements(by).stream().map(WebElement::getText).anyMatch(toFind::equals))
+        .isTrue();
+  }
+
  @Test
  public void testEditButtonsVisibility_owner() throws Throwable {
    driver.get(server.getUrl("/registrar#whois-settings"));
@@ -190,9 +197,11 @@ public class RegistrarConsoleWebTest extends WebDriverTestCase {
    ImmutableList<RegistrarContact> contacts =
        server.runInAppEngineEnvironment(
            () -> loadRegistrar("TheRegistrar").getContacts().asList());
-    assertEltText("contacts[0].name", contacts.get(0).getName());
-    assertEltText("contacts[0].emailAddress", contacts.get(0).getEmailAddress());
-    assertEltText("contacts[0].phoneNumber", contacts.get(0).getPhoneNumber());
+    for (RegistrarContact contact : contacts) {
+      assertEltTextPresent(By.id("contacts[0].name"), contact.getName());
+      assertEltTextPresent(By.id("contacts[0].emailAddress"), contact.getEmailAddress());
+      assertEltTextPresent(By.id("contacts[0].phoneNumber"), contact.getPhoneNumber());
+    }
  }

  @Test
@@ -11,9 +11,10 @@ emailAddress: the.registrar@example.com -> thase@the.registrar
 url: http://my.fake.url -> http://my.new.url
 contacts:
    ADDED:
-        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Extra Terrestrial, emailAddress=etphonehome@example.com, phoneNumber=+1.2345678901, faxNumber=null, types=[ADMIN, BILLING, TECH, WHOIS], gaeUserId=null, visibleInWhoisAsAdmin=true, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false, registryLockPasswordHash=null, registryLockPasswordSalt=null}
+        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Extra Terrestrial, emailAddress=etphonehome@example.com, phoneNumber=+1.2345678901, faxNumber=null, types=[ADMIN, BILLING, TECH, WHOIS], gaeUserId=null, visibleInWhoisAsAdmin=true, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false}
    REMOVED:
-        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=John Doe, emailAddress=johndoe@theregistrar.com, phoneNumber=+1.1234567890, faxNumber=null, types=[ADMIN], gaeUserId=31337, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false, registryLockPasswordHash=null, registryLockPasswordSalt=null},
-        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Jian-Yang, emailAddress=jyang@bachman.accelerator, phoneNumber=+1.1234567890, faxNumber=null, types=[TECH], gaeUserId=null, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false, registryLockPasswordHash=null, registryLockPasswordSalt=null}
+        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Marla Singer, emailAddress=Marla.Singer@crr.com, phoneNumber=+1.2128675309, faxNumber=null, types=[TECH], gaeUserId=31337, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false},
+        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=John Doe, emailAddress=johndoe@theregistrar.com, phoneNumber=+1.1234567890, faxNumber=null, types=[ADMIN], gaeUserId=31337, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false},
+        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Jian-Yang, emailAddress=jyang@bachman.accelerator, phoneNumber=+1.1234567890, faxNumber=null, types=[TECH], gaeUserId=null, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false}
    FINAL CONTENTS:
-        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Extra Terrestrial, emailAddress=etphonehome@example.com, phoneNumber=+1.2345678901, faxNumber=null, types=[ADMIN, BILLING, TECH, WHOIS], gaeUserId=null, visibleInWhoisAsAdmin=true, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false, registryLockPasswordHash=null, registryLockPasswordSalt=null}
+        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Extra Terrestrial, emailAddress=etphonehome@example.com, phoneNumber=+1.2345678901, faxNumber=null, types=[ADMIN, BILLING, TECH, WHOIS], gaeUserId=null, visibleInWhoisAsAdmin=true, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false}
@@ -0,0 +1,16 @@
+-- Copyright 2019 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.
+
+create index if not exists idx_registry_lock_registrar_id ON "RegistryLock"
+   using btree (registrar_id);
@@ -22,6 +22,7 @@
    create table "ClaimsList" (
       revision_id  bigserial not null,
        creation_timestamp timestamptz not null,
+        tmdb_generation_time timestamptz not null,
        primary key (revision_id)
    );

@@ -160,6 +161,8 @@
    alter table if exists "Domain_GracePeriod" 
       add constraint UK_4ps2u4y8i5r91wu2n1x2xea28 unique (grace_periods_id);
 create index premiumlist_name_idx on "PremiumList" (name);
+create index idx_registry_lock_verification_code on "RegistryLock" (verification_code);
+create index idx_registry_lock_registrar_id on "RegistryLock" (registrar_id);

    alter table if exists "RegistryLock" 
       add constraint idx_registry_lock_repo_id_revision_id unique (repo_id, revision_id);
@@ -223,6 +223,13 @@ ALTER TABLE ONLY public."RegistryLock"
    ADD CONSTRAINT idx_registry_lock_repo_id_revision_id UNIQUE (repo_id, revision_id);


+--
+-- Name: idx_registry_lock_registrar_id; Type: INDEX; Schema: public; Owner: -
+--
+
+CREATE INDEX idx_registry_lock_registrar_id ON public."RegistryLock" USING btree (registrar_id);
+
+
 --
 -- Name: idx_registry_lock_verification_code; Type: INDEX; Schema: public; Owner: -
 --
@@ -79,6 +79,7 @@ ext {
      'com.sun.xml.bind:jaxb-xjc:2.2.11',
      'com.thoughtworks.qdox:qdox:1.12.1',
      'dnsjava:dnsjava:2.1.7',
+      'io.github.classgraph:classgraph:4.8.52',
      'io.github.java-diff-utils:java-diff-utils:4.0',
      'io.netty:netty-buffer:4.1.31.Final',
      'io.netty:netty-codec:4.1.31.Final',
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -130,6 +130,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -18,7 +18,8 @@ task buildProxyImage(dependsOn: deployJar, type: Exec) {
  commandLine 'docker', 'build', '-t', 'proxy', '.'
 }

-task deployProxy(dependsOn: [buildProxyImage, ':verifyDeployment']) {
+task deployProxy(dependsOn: buildProxyImage) {
+  configure verifyDeploymentConfig
  doLast {
    exec {
      commandLine 'docker', 'tag', 'proxy', "gcr.io/${rootProject.gcpProject}/proxy"
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -130,6 +130,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -24,6 +24,7 @@ import google.registry.proxy.HttpsRelayProtocolModule.HttpsRelayProtocol;
 import google.registry.proxy.Protocol.BackendProtocol;
 import google.registry.proxy.Protocol.FrontendProtocol;
 import google.registry.proxy.handler.EppServiceHandler;
+import google.registry.proxy.handler.FrontendMetricsHandler;
 import google.registry.proxy.handler.ProxyProtocolHandler;
 import google.registry.proxy.handler.QuotaHandler.EppQuotaHandler;
 import google.registry.proxy.handler.RelayHandler.FullHttpRequestRelayHandler;
@@ -85,6 +86,7 @@ public class EppProtocolModule {
      Provider<LengthFieldBasedFrameDecoder> lengthFieldBasedFrameDecoderProvider,
      Provider<LengthFieldPrepender> lengthFieldPrependerProvider,
      Provider<EppServiceHandler> eppServiceHandlerProvider,
+      Provider<FrontendMetricsHandler> frontendMetricsHandlerProvider,
      Provider<EppQuotaHandler> eppQuotaHandlerProvider,
      Provider<FullHttpRequestRelayHandler> relayHandlerProvider) {
    return ImmutableList.of(
@@ -94,6 +96,7 @@ public class EppProtocolModule {
        lengthFieldBasedFrameDecoderProvider,
        lengthFieldPrependerProvider,
        eppServiceHandlerProvider,
+        frontendMetricsHandlerProvider,
        eppQuotaHandlerProvider,
        relayHandlerProvider);
  }
@@ -21,6 +21,7 @@ import dagger.multibindings.IntoSet;
 import google.registry.proxy.HttpsRelayProtocolModule.HttpsRelayProtocol;
 import google.registry.proxy.Protocol.BackendProtocol;
 import google.registry.proxy.Protocol.FrontendProtocol;
+import google.registry.proxy.handler.FrontendMetricsHandler;
 import google.registry.proxy.handler.ProxyProtocolHandler;
 import google.registry.proxy.handler.QuotaHandler.WhoisQuotaHandler;
 import google.registry.proxy.handler.RelayHandler.FullHttpRequestRelayHandler;
@@ -74,6 +75,7 @@ public class WhoisProtocolModule {
      @WhoisProtocol Provider<ReadTimeoutHandler> readTimeoutHandlerProvider,
      Provider<LineBasedFrameDecoder> lineBasedFrameDecoderProvider,
      Provider<WhoisServiceHandler> whoisServiceHandlerProvider,
+      Provider<FrontendMetricsHandler> frontendMetricsHandlerProvider,
      Provider<WhoisQuotaHandler> whoisQuotaHandlerProvider,
      Provider<FullHttpRequestRelayHandler> relayHandlerProvider) {
    return ImmutableList.of(
@@ -81,6 +83,7 @@ public class WhoisProtocolModule {
        readTimeoutHandlerProvider,
        lineBasedFrameDecoderProvider,
        whoisServiceHandlerProvider,
+        frontendMetricsHandlerProvider,
        whoisQuotaHandlerProvider,
        relayHandlerProvider);
  }
@@ -36,14 +36,16 @@ import java.util.Optional;
 import java.util.Queue;
 import javax.inject.Inject;
 import org.joda.time.DateTime;
+import org.joda.time.Duration;

 /**
- * Handler that records metrics a backend channel.
+ * Handler that records metrics for a backend channel.
 *
- * <p>This handler is added right before {@link FullHttpResponseRelayHandler} in the backend
- * protocol handler provider method. {@link FullHttpRequest} outbound messages encounter this first
- * before being handed over to HTTP related handler. {@link FullHttpResponse} inbound messages are
- * first constructed (from plain bytes) by preceding handlers and then logged in this handler.
+ * <p>This handler is added before the {@link FullHttpResponseRelayHandler} in the backend protocol
+ * handler provider method. {@link FullHttpRequest} outbound messages encounter this first before
+ * being handed over to HTTP related handler. {@link FullHttpResponse} inbound messages are first
+ * constructed (from plain bytes) by preceding handlers and then related metrics are instrumented in
+ * this handler.
 */
 public class BackendMetricsHandler extends ChannelDuplexHandler {

@@ -95,7 +97,7 @@ public class BackendMetricsHandler extends ChannelDuplexHandler {
        relayedProtocolName,
        clientCertHash,
        (FullHttpResponse) msg,
-        clock.nowUtc().getMillis() - requestSentTimeQueue.remove().getMillis());
+        new Duration(requestSentTimeQueue.remove().getMillis(), clock.nowUtc().getMillis()));
    super.channelRead(ctx, msg);
  }

@@ -118,8 +120,6 @@ public class BackendMetricsHandler extends ChannelDuplexHandler {
    // called and the readable bytes would be zero by then.
    int bytes = request.content().readableBytes();

-    // Record sent time before write finishes allows us to take network latency into account.
-    DateTime sentTime = clock.nowUtc();
    ChannelFuture unusedFuture =
        ctx.write(msg, promise)
            .addListener(
@@ -127,7 +127,7 @@ public class BackendMetricsHandler extends ChannelDuplexHandler {
                  if (future.isSuccess()) {
                    // Only instrument request metrics when the request is actually sent to GAE.
                    metrics.requestSent(relayedProtocolName, clientCertHash, bytes);
-                    requestSentTimeQueue.add(sentTime);
+                    requestSentTimeQueue.add(clock.nowUtc());
                  }
                });
  }
@@ -0,0 +1,121 @@
+// 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.proxy.handler;
+
+import static com.google.common.base.Preconditions.checkState;
+import static google.registry.proxy.Protocol.PROTOCOL_KEY;
+import static google.registry.proxy.handler.EppServiceHandler.CLIENT_CERTIFICATE_HASH_KEY;
+
+import google.registry.proxy.metric.FrontendMetrics;
+import google.registry.util.Clock;
+import io.netty.channel.ChannelDuplexHandler;
+import io.netty.channel.ChannelFuture;
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelPromise;
+import java.util.ArrayDeque;
+import java.util.Optional;
+import java.util.Queue;
+import javax.inject.Inject;
+import org.joda.time.DateTime;
+import org.joda.time.Duration;
+
+/**
+ * Handler that records metrics for a fronend channel.
+ *
+ * <p>This handler is added before the {@link RelayHandler} in the frontend protocol handler
+ * provider method. Outbound messages encounter this first before being handed over to
+ * protocol-specific handlers. Inbound messages are first constructed (from plain bytes) by
+ * preceding handlers and then related metrics are instrumented in this handler.
+ */
+public class FrontendMetricsHandler extends ChannelDuplexHandler {
+
+  private final Clock clock;
+  private final FrontendMetrics metrics;
+
+  private String protocolName;
+  private String clientCertHash;
+
+  /**
+   * A queue that saves the time at which a request is received from the client.
+   *
+   * <p>This queue is used to calculate frontend request-response latency.
+   *
+   * <p>For the WHOIS protocol, the TCP connection closes after one request-response round trip and
+   * the request always comes first. The queue for WHOIS therefore only need to store one value.
+   *
+   * <p>For the EPP protocol, the specification allows for pipelining, in which a client can sent
+   * multiple requests without waiting for each responses. Therefore a queue is needed to record all
+   * the requests that are sent but have not yet received a response.
+   *
+   * <p>A server must send its response in the same order it receives requests. This invariance
+   * guarantees that the request time at the head of the queue always corresponds to the response
+   * received in {@link #channelRead}.
+   *
+   * @see <a href="https://tools.ietf.org/html/rfc3912">RFC 3912 WHOIS Protocol Specification</a>
+   * @see <a href="https://tools.ietf.org/html/rfc5734#section-3">RFC 5734 Extensible Provisioning
+   *     Protocol (EPP) Transport over TCP</a>
+   */
+  private final Queue<DateTime> requestReceivedTimeQueue = new ArrayDeque<>();
+
+  @Inject
+  FrontendMetricsHandler(Clock clock, FrontendMetrics metrics) {
+    this.clock = clock;
+    this.metrics = metrics;
+  }
+
+  @Override
+  public void channelRegistered(ChannelHandlerContext ctx) throws Exception {
+    protocolName = ctx.channel().attr(PROTOCOL_KEY).get().name();
+    super.channelRegistered(ctx);
+  }
+
+  @Override
+  public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception {
+    requestReceivedTimeQueue.add(clock.nowUtc());
+    super.channelRead(ctx, msg);
+  }
+
+  @Override
+  public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise)
+      throws Exception {
+    // Only instrument request metrics when the response is actually sent to client.
+    // It is OK to check the queue size preemptively here, not when the front element of the queue
+    // is acutally removed after the write to the client is successful,  because responses are
+    // written to the client in order. Hence there cannot be any response succsssfully sent to the
+    // client (which reduces the queue size) before this current request is sent. The queue *can*
+    // increase in size if more requests are received from the client, but that does not invalidate
+    // this check.
+    checkState(!requestReceivedTimeQueue.isEmpty(), "Response sent before request is received.");
+    // For WHOIS, client certificate hash is always set to "none".
+    // For EPP, the client hash attribute is set upon handshake completion, before the first HELLO
+    // is sent to the server. Therefore the first call to write() with HELLO payload has access to
+    // the hash in its channel attribute.
+    if (clientCertHash == null) {
+      clientCertHash =
+          Optional.ofNullable(ctx.channel().attr(CLIENT_CERTIFICATE_HASH_KEY).get()).orElse("none");
+    }
+    ChannelFuture unusedFuture =
+        ctx.write(msg, promise)
+            .addListener(
+                future -> {
+                  if (future.isSuccess()) {
+                    metrics.responseSent(
+                        protocolName,
+                        clientCertHash,
+                        new Duration(requestReceivedTimeQueue.remove(), clock.nowUtc()));
+                  }
+                });
+  }
+}
@@ -15,10 +15,7 @@
 package google.registry.proxy.metric;

 import com.google.common.collect.ImmutableSet;
-import com.google.monitoring.metrics.CustomFitter;
 import com.google.monitoring.metrics.EventMetric;
-import com.google.monitoring.metrics.ExponentialFitter;
-import com.google.monitoring.metrics.FibonacciFitter;
 import com.google.monitoring.metrics.IncrementableMetric;
 import com.google.monitoring.metrics.LabelDescriptor;
 import com.google.monitoring.metrics.MetricRegistryImpl;
@@ -26,26 +23,11 @@ import google.registry.util.NonFinalForTesting;
 import io.netty.handler.codec.http.FullHttpResponse;
 import javax.inject.Inject;
 import javax.inject.Singleton;
+import org.joda.time.Duration;

 /** Backend metrics instrumentation. */
@Singleton
-public class BackendMetrics {
-
-  // Maximum request size is defined in the config file, this is not realistic and we'd be out of
-  // memory when the size approach 1 GB.
-  private static final CustomFitter DEFAULT_SIZE_FITTER = FibonacciFitter.create(1073741824);
-
-  // Maximum 1 hour latency, this is not specified by the spec, but given we have a one hour idle
-  // timeout, it seems reasonable that maximum latency is set to 1 hour as well. If we are
-  // approaching anywhere near 1 hour latency, we'd be way out of SLO anyway.
-  private static final ExponentialFitter DEFAULT_LATENCY_FITTER =
-      ExponentialFitter.create(22, 2, 1.0);
-
-  private static final ImmutableSet<LabelDescriptor> LABELS =
-      ImmutableSet.of(
-          LabelDescriptor.create("protocol", "Name of the protocol."),
-          LabelDescriptor.create(
-              "client_cert_hash", "SHA256 hash of the client certificate, if available."));
+public class BackendMetrics extends BaseMetrics {

  static final IncrementableMetric requestsCounter =
      MetricRegistryImpl.getDefault()
@@ -96,13 +78,8 @@ public class BackendMetrics {
  @Inject
  BackendMetrics() {}

-  /**
-   * Resets all backend metrics.
-   *
-   * <p>This should only used in tests to clear out states. No production code should call this
-   * function.
-   */
-  void resetMetric() {
+  @Override
+  void resetMetrics() {
    requestBytes.reset();
    requestsCounter.reset();
    responseBytes.reset();
@@ -118,8 +95,8 @@ public class BackendMetrics {

  @NonFinalForTesting
  public void responseReceived(
-      String protocol, String certHash, FullHttpResponse response, long latency) {
-    latencyMs.record(latency, protocol, certHash);
+      String protocol, String certHash, FullHttpResponse response, Duration latency) {
+    latencyMs.record(latency.getMillis(), protocol, certHash);
    responseBytes.record(response.content().readableBytes(), protocol, certHash);
    responsesCounter.increment(protocol, certHash, response.status().toString());
  }
@@ -0,0 +1,62 @@
+// Copyright 2019 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.proxy.metric;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableSet;
+import com.google.monitoring.metrics.CustomFitter;
+import com.google.monitoring.metrics.ExponentialFitter;
+import com.google.monitoring.metrics.FibonacciFitter;
+import com.google.monitoring.metrics.LabelDescriptor;
+
+/** Base class for metrics. */
+public abstract class BaseMetrics {
+
+  /**
+   * Labels to register metrics with.
+   *
+   * <p>The client certificate hash value is only used for EPP metrics. For WHOIS metrics, it will
+   * always be {@code "none"}. In order to get the actual registrar name, one can use the {@code
+   * nomulus} tool:
+   *
+   * <pre>
+   * nomulus -e production list_registrars -f clientCertificateHash | grep $HASH
+   * </pre>
+   */
+   protected static final ImmutableSet<LabelDescriptor> LABELS =
+      ImmutableSet.of(
+          LabelDescriptor.create("protocol", "Name of the protocol."),
+          LabelDescriptor.create(
+              "client_cert_hash", "SHA256 hash of the client certificate, if available."));
+
+  // Maximum request size is defined in the config file, this is not realistic and we'd be out of
+  // memory when the size approaches 1 GB.
+  protected static final CustomFitter DEFAULT_SIZE_FITTER = FibonacciFitter.create(1073741824);
+
+  // Maximum 1 hour latency, this is not specified by the spec, but given we have a one hour idle
+  // timeout, it seems reasonable that maximum latency is set to 1 hour as well. If we are
+  // approaching anywhere near 1 hour latency, we'd be way out of SLO anyway.
+  protected static final ExponentialFitter DEFAULT_LATENCY_FITTER =
+      ExponentialFitter.create(22, 2, 1.0);
+
+  /**
+   * Resets all metrics.
+   *
+   * <p>This should only be used in tests to reset states. Production code should not call this
+   * method.
+   */
+  @VisibleForTesting
+  abstract void resetMetrics();
+}
@@ -14,12 +14,10 @@

 package google.registry.proxy.metric;

-import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
+import com.google.monitoring.metrics.EventMetric;
 import com.google.monitoring.metrics.IncrementableMetric;
-import com.google.monitoring.metrics.LabelDescriptor;
 import com.google.monitoring.metrics.Metric;
 import com.google.monitoring.metrics.MetricRegistryImpl;
 import google.registry.util.NonFinalForTesting;
@@ -32,27 +30,11 @@ import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 import javax.inject.Inject;
 import javax.inject.Singleton;
+import org.joda.time.Duration;

 /** Frontend metrics instrumentation. */
@Singleton
-public class FrontendMetrics {
-
-  /**
-   * Labels to register front metrics with.
-   *
-   * <p>The client certificate hash value is only used for EPP metrics. For WHOIS metrics, it will
-   * always be {@code "none"}. In order to get the actual registrar name, one can use the {@code
-   * nomulus} tool:
-   *
-   * <pre>
-   * nomulus -e production list_registrars -f clientCertificateHash | grep $HASH
-   * </pre>
-   */
-  private static final ImmutableSet<LabelDescriptor> LABELS =
-      ImmutableSet.of(
-          LabelDescriptor.create("protocol", "Name of the protocol."),
-          LabelDescriptor.create(
-              "client_cert_hash", "SHA256 hash of the client certificate, if available."));
+public class FrontendMetrics extends BaseMetrics {

  private static final ConcurrentMap<ImmutableList<String>, ChannelGroup> activeConnections =
      new ConcurrentHashMap<>();
@@ -65,9 +47,7 @@ public class FrontendMetrics {
              "Active Connections",
              LABELS,
              () ->
-                  activeConnections
-                      .entrySet()
-                      .stream()
+                  activeConnections.entrySet().stream()
                      .collect(
                          ImmutableMap.toImmutableMap(
                              Map.Entry::getKey, entry -> (long) entry.getValue().size())),
@@ -89,19 +69,23 @@ public class FrontendMetrics {
              "Quota Rejections",
              LABELS);

+  static final EventMetric latencyMs =
+      MetricRegistryImpl.getDefault()
+          .newEventMetric(
+              "/proxy/frontend/latency_ms",
+              "Round-trip time between a request received and its corresponding response is sent.",
+              "Latency Milliseconds",
+              LABELS,
+              DEFAULT_LATENCY_FITTER);
+
  @Inject
  public FrontendMetrics() {}

-  /**
-   * Resets all frontend metrics.
-   *
-   * <p>This should only be used in tests to reset states. Production code should not call this
-   * method.
-   */
-  @VisibleForTesting
+  @Override
  void resetMetrics() {
    totalConnectionsCounter.reset();
    activeConnections.clear();
+    latencyMs.reset();
  }

  @NonFinalForTesting
@@ -122,4 +106,9 @@ public class FrontendMetrics {
  public void registerQuotaRejection(String protocol, String certHash) {
    quotaRejectionsCounter.increment(protocol, certHash);
  }
+
+  @NonFinalForTesting
+  public void responseSent(String protocol, String certHash, Duration latency) {
+    latencyMs.record(latency.getMillis(), protocol, certHash);
+  }
 }
@@ -32,6 +32,7 @@ import google.registry.proxy.ProxyConfig.Environment;
 import google.registry.proxy.WebWhoisProtocolsModule.HttpWhoisProtocol;
 import google.registry.proxy.WhoisProtocolModule.WhoisProtocol;
 import google.registry.proxy.handler.BackendMetricsHandler;
+import google.registry.proxy.handler.FrontendMetricsHandler;
 import google.registry.proxy.handler.ProxyProtocolHandler;
 import google.registry.proxy.handler.QuotaHandler.EppQuotaHandler;
 import google.registry.proxy.handler.QuotaHandler.WhoisQuotaHandler;
@@ -74,7 +75,7 @@ import org.junit.Before;
 */
 public abstract class ProtocolModuleTest {

-  protected static final ProxyConfig PROXY_CONFIG = getProxyConfig(LOCAL);
+  protected static final ProxyConfig PROXY_CONFIG = getProxyConfig(Environment.LOCAL);

  protected TestComponent testComponent;

@@ -106,6 +107,7 @@ public abstract class ProtocolModuleTest {
          LoggingHandler.class,
          // Metrics instrumentation is tested separately.
          BackendMetricsHandler.class,
+          FrontendMetricsHandler.class,
          // Quota management is tested separately.
          WhoisQuotaHandler.class,
          EppQuotaHandler.class,
@@ -133,7 +133,8 @@ public class BackendMetricsHandlerTest {

    verify(metrics)
        .requestSent(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, request.content().readableBytes());
-    verify(metrics).responseReceived(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, response, 1);
+    verify(metrics)
+        .responseReceived(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, response, Duration.millis(1));
    verifyNoMoreInteractions(metrics);
  }

@@ -154,7 +155,8 @@ public class BackendMetricsHandlerTest {

    verify(metrics)
        .requestSent(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, request.content().readableBytes());
-    verify(metrics).responseReceived(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, response, 1);
+    verify(metrics)
+        .responseReceived(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, response, Duration.millis(1));
    verifyNoMoreInteractions(metrics);
  }

@@ -178,46 +180,46 @@ public class BackendMetricsHandlerTest {
    // First request, time = 0
    assertThat(channel.writeOutbound(request1)).isTrue();
    assertHttpRequestEquivalent(request1, channel.readOutbound());
-    DateTime sentTime1 = fakeClock.nowUtc();
+    DateTime requestTime1 = fakeClock.nowUtc();

    fakeClock.advanceBy(Duration.millis(5));

    // Second request, time = 5
    assertThat(channel.writeOutbound(request2)).isTrue();
    assertHttpRequestEquivalent(request2, channel.readOutbound());
-    DateTime sentTime2 = fakeClock.nowUtc();
+    DateTime requestTime2 = fakeClock.nowUtc();

    fakeClock.advanceBy(Duration.millis(7));

    // First response, time = 12, latency = 12 - 0 = 12
    assertThat(channel.writeInbound(response1)).isTrue();
    assertHttpResponseEquivalent(response1, channel.readInbound());
-    DateTime receivedTime1 = fakeClock.nowUtc();
+    DateTime responseTime1 = fakeClock.nowUtc();

    fakeClock.advanceBy(Duration.millis(11));

    // Third request, time = 23
    assertThat(channel.writeOutbound(request3)).isTrue();
    assertHttpRequestEquivalent(request3, channel.readOutbound());
-    DateTime sentTime3 = fakeClock.nowUtc();
+    DateTime requestTime3 = fakeClock.nowUtc();

    fakeClock.advanceBy(Duration.millis(2));

    // Second response, time = 25, latency = 25 - 5 = 20
    assertThat(channel.writeInbound(response2)).isTrue();
    assertHttpResponseEquivalent(response2, channel.readInbound());
-    DateTime receivedTime2 = fakeClock.nowUtc();
+    DateTime responseTime2 = fakeClock.nowUtc();

    fakeClock.advanceBy(Duration.millis(4));

    // Third response, time = 29, latency = 29 - 23 = 6
    assertThat(channel.writeInbound(response3)).isTrue();
    assertHttpResponseEquivalent(response3, channel.readInbound());
-    DateTime receivedTime3 = fakeClock.nowUtc();
+    DateTime responseTime3 = fakeClock.nowUtc();

-    long latency1 = new Duration(sentTime1, receivedTime1).getMillis();
-    long latency2 = new Duration(sentTime2, receivedTime2).getMillis();
-    long latency3 = new Duration(sentTime3, receivedTime3).getMillis();
+    Duration latency1 = new Duration(requestTime1, responseTime1);
+    Duration latency2 = new Duration(requestTime2, responseTime2);
+    Duration latency3 = new Duration(requestTime3, responseTime3);

    verify(metrics)
        .requestSent(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, request1.content().readableBytes());
@@ -0,0 +1,169 @@
+// 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.proxy.handler;
+
+import static com.google.common.truth.Truth.assertThat;
+import static google.registry.proxy.Protocol.PROTOCOL_KEY;
+import static google.registry.proxy.handler.EppServiceHandler.CLIENT_CERTIFICATE_HASH_KEY;
+import static google.registry.testing.JUnitBackports.assertThrows;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.mockito.Mockito.verifyZeroInteractions;
+
+import com.google.common.collect.ImmutableList;
+import google.registry.proxy.Protocol;
+import google.registry.proxy.Protocol.FrontendProtocol;
+import google.registry.proxy.metric.FrontendMetrics;
+import google.registry.testing.FakeClock;
+import io.netty.channel.ChannelInitializer;
+import io.netty.channel.embedded.EmbeddedChannel;
+import org.joda.time.DateTime;
+import org.joda.time.Duration;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Unit tests for {@link FrontendMetricsHandler}. */
+@RunWith(JUnit4.class)
+public class FrontendMetricsHandlerTest {
+
+  private static final String CLIENT_CERT_HASH = "blah12345";
+  private static final String PROTOCOL_NAME = "frontend protocol";
+
+  private final FakeClock fakeClock = new FakeClock();
+  private final FrontendMetrics metrics = mock(FrontendMetrics.class);
+  private final FrontendMetricsHandler handler = new FrontendMetricsHandler(fakeClock, metrics);
+
+  private final FrontendProtocol frontendProtocol =
+      Protocol.frontendBuilder()
+          .name(PROTOCOL_NAME)
+          .port(2)
+          .hasBackend(false)
+          .handlerProviders(ImmutableList.of())
+          .build();
+
+  private EmbeddedChannel channel;
+
+  @Before
+  public void setUp() {
+    channel =
+        new EmbeddedChannel(
+            new ChannelInitializer<EmbeddedChannel>() {
+              @Override
+              protected void initChannel(EmbeddedChannel ch) throws Exception {
+                ch.attr(PROTOCOL_KEY).set(frontendProtocol);
+                ch.attr(CLIENT_CERTIFICATE_HASH_KEY).set(CLIENT_CERT_HASH);
+                ch.pipeline().addLast(handler);
+              }
+            });
+  }
+
+  @Test
+  public void testSuccess_oneRequest() {
+    // Inbound message passed to the next handler.
+    Object request = new Object();
+    assertThat(channel.writeInbound(request)).isTrue();
+    assertThat((Object) channel.readInbound()).isEqualTo(request);
+    verifyZeroInteractions(metrics);
+  }
+
+  @Test
+  public void testSuccess_oneRequest_oneResponse() {
+    Object request = new Object();
+    Object response = new Object();
+    // Inbound message passed to the next handler.
+    assertThat(channel.writeInbound(request)).isTrue();
+    assertThat((Object) channel.readInbound()).isEqualTo(request);
+    fakeClock.advanceOneMilli();
+    // Outbound message passed to the next handler.
+    assertThat(channel.writeOutbound(response)).isTrue();
+    assertThat((Object) channel.readOutbound()).isEqualTo(response);
+    // Verify that latency is recorded.
+    verify(metrics).responseSent(PROTOCOL_NAME, CLIENT_CERT_HASH, Duration.millis(1));
+    verifyNoMoreInteractions(metrics);
+  }
+
+  @Test
+  public void testFailure_responseBeforeRequest() {
+    Object response = new Object();
+    IllegalStateException e =
+        assertThrows(IllegalStateException.class, () -> channel.writeOutbound(response));
+    assertThat(e).hasMessageThat().isEqualTo("Response sent before request is received.");
+  }
+
+  @Test
+  public void testSuccess_pipelinedResponses() {
+    Object request1 = new Object();
+    Object response1 = new Object();
+    Object request2 = new Object();
+    Object response2 = new Object();
+    Object request3 = new Object();
+    Object response3 = new Object();
+
+    // First request, time = 0
+    assertThat(channel.writeInbound(request1)).isTrue();
+    assertThat((Object) channel.readInbound()).isEqualTo(request1);
+    DateTime requestTime1 = fakeClock.nowUtc();
+
+    fakeClock.advanceBy(Duration.millis(5));
+
+    // Second request, time = 5
+    assertThat(channel.writeInbound(request2)).isTrue();
+    assertThat((Object) channel.readInbound()).isEqualTo(request2);
+    DateTime requestTime2 = fakeClock.nowUtc();
+
+    fakeClock.advanceBy(Duration.millis(7));
+
+    // First response, time = 12, latency = 12 - 0 = 12
+    assertThat(channel.writeOutbound(response1)).isTrue();
+    assertThat((Object) channel.readOutbound()).isEqualTo(response1);
+    DateTime responseTime1 = fakeClock.nowUtc();
+
+    fakeClock.advanceBy(Duration.millis(11));
+
+    // Third request, time = 23
+    assertThat(channel.writeInbound(request3)).isTrue();
+    assertThat((Object) channel.readInbound()).isEqualTo(request3);
+    DateTime requestTime3 = fakeClock.nowUtc();
+
+    fakeClock.advanceBy(Duration.millis(2));
+
+    // Second response, time = 25, latency = 25 - 5 = 20
+    assertThat(channel.writeOutbound(response2)).isTrue();
+    assertThat((Object) channel.readOutbound()).isEqualTo(response2);
+    DateTime responseTime2 = fakeClock.nowUtc();
+
+    fakeClock.advanceBy(Duration.millis(4));
+
+    // Third response, time = 29, latency = 29 - 23 = 6
+    assertThat(channel.writeOutbound(response3)).isTrue();
+    assertThat((Object) channel.readOutbound()).isEqualTo(response3);
+    DateTime responseTime3 = fakeClock.nowUtc();
+
+    Duration latency1 = new Duration(requestTime1, responseTime1);
+    Duration latency2 = new Duration(requestTime2, responseTime2);
+    Duration latency3 = new Duration(requestTime3, responseTime3);
+
+    verify(metrics)
+        .responseSent(PROTOCOL_NAME, CLIENT_CERT_HASH, latency1);
+    verify(metrics)
+        .responseSent(PROTOCOL_NAME, CLIENT_CERT_HASH, latency2);
+    verify(metrics)
+        .responseSent(PROTOCOL_NAME, CLIENT_CERT_HASH, latency3);
+    verifyNoMoreInteractions(metrics);
+  }
+}
@@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableSet;
 import io.netty.handler.codec.http.FullHttpRequest;
 import io.netty.handler.codec.http.FullHttpResponse;
 import io.netty.handler.codec.http.HttpResponseStatus;
+import org.joda.time.Duration;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -40,7 +41,7 @@ public class BackendMetricsTest {

  @Before
  public void setUp() {
-    metrics.resetMetric();
+    metrics.resetMetrics();
  }

  @Test
@@ -89,7 +90,7 @@ public class BackendMetricsTest {
  public void testSuccess_oneResponse() {
    String content = "some response";
    FullHttpResponse response = makeHttpResponse(content, HttpResponseStatus.OK);
-    metrics.responseReceived(protocol, certHash, response, 5);
+    metrics.responseReceived(protocol, certHash, response, Duration.millis(5));

    assertThat(BackendMetrics.requestsCounter).hasNoOtherValues();
    assertThat(BackendMetrics.requestBytes).hasNoOtherValues();
@@ -115,9 +116,9 @@ public class BackendMetricsTest {
    FullHttpResponse response1 = makeHttpResponse(content1, HttpResponseStatus.OK);
    FullHttpResponse response2 = makeHttpResponse(content2, HttpResponseStatus.OK);
    FullHttpResponse response3 = makeHttpResponse(content3, HttpResponseStatus.BAD_REQUEST);
-    metrics.responseReceived(protocol, certHash, response1, 5);
-    metrics.responseReceived(protocol, certHash, response2, 8);
-    metrics.responseReceived(protocol, certHash, response3, 2);
+    metrics.responseReceived(protocol, certHash, response1, Duration.millis(5));
+    metrics.responseReceived(protocol, certHash, response2, Duration.millis(8));
+    metrics.responseReceived(protocol, certHash, response3, Duration.millis(2));

    assertThat(BackendMetrics.requestsCounter).hasNoOtherValues();
    assertThat(BackendMetrics.requestBytes).hasNoOtherValues();
@@ -147,7 +148,7 @@ public class BackendMetricsTest {
    FullHttpRequest request = makeHttpPostRequest(requestContent, host, "/");
    FullHttpResponse response = makeHttpResponse(responseContent, HttpResponseStatus.OK);
    metrics.requestSent(protocol, certHash, request.content().readableBytes());
-    metrics.responseReceived(protocol, certHash, response, 10);
+    metrics.responseReceived(protocol, certHash, response, Duration.millis(10));

    assertThat(BackendMetrics.requestsCounter)
        .hasValueForLabels(1, protocol, certHash)
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -130,6 +130,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -70,32 +70,8 @@ public class Retrier implements Serializable {
   *
   * @return <V> the value returned by the {@link Callable}.
   */
-  private <V> V callWithRetry(
-      Callable<V> callable,
-      FailureReporter failureReporter,
-      Predicate<Throwable> isRetryable) {
-    int failures = 0;
-    while (true) {
-      try {
-        return callable.call();
-      } catch (Throwable e) {
-        if (++failures == attempts || !isRetryable.test(e)) {
-          throwIfUnchecked(e);
-          throw new RuntimeException(e);
-        }
-        failureReporter.beforeRetry(e, failures, attempts);
-        try {
-          // Wait 100ms on the first attempt, doubling on each subsequent attempt.
-          sleeper.sleep(Duration.millis(pow(2, failures) * 100));
-        } catch (InterruptedException e2) {
-          // Since we're not rethrowing InterruptedException, set the interrupt state on the thread
-          // so the next blocking operation will know to abort the thread.
-          Thread.currentThread().interrupt();
-          throwIfUnchecked(e);
-          throw new RuntimeException(e);
-        }
-      }
-    }
+  public <V> V callWithRetry(Callable<V> callable, Predicate<Throwable> isRetryable) {
+    return callWithRetry(callable, LOGGING_FAILURE_REPORTER, isRetryable);
  }

  /**
@@ -169,6 +145,34 @@ public class Retrier implements Serializable {
    callWithRetry(callable.asCallable(), failureReporter, retryableError, moreRetryableErrors);
  }

+  private <V> V callWithRetry(
+      Callable<V> callable,
+      FailureReporter failureReporter,
+      Predicate<Throwable> isRetryable) {
+    int failures = 0;
+    while (true) {
+      try {
+        return callable.call();
+      } catch (Throwable e) {
+        if (++failures == attempts || !isRetryable.test(e)) {
+          throwIfUnchecked(e);
+          throw new RuntimeException(e);
+        }
+        failureReporter.beforeRetry(e, failures, attempts);
+        try {
+          // Wait 100ms on the first attempt, doubling on each subsequent attempt.
+          sleeper.sleep(Duration.millis(pow(2, failures) * 100));
+        } catch (InterruptedException e2) {
+          // Since we're not rethrowing InterruptedException, set the interrupt state on the thread
+          // so the next blocking operation will know to abort the thread.
+          Thread.currentThread().interrupt();
+          throwIfUnchecked(e);
+          throw new RuntimeException(e);
+        }
+      }
+    }
+  }
+
  private static final FailureReporter LOGGING_FAILURE_REPORTER =
      (thrown, failures, maxAttempts) ->
          logger.atInfo().withCause(thrown).log(
@@ -38,7 +38,7 @@ public class RetrierTest {
    }
  }

-  /** Test object that always throws an exception with the current count. */
+  /** Test object that throws CountingExceptions up to a given limit, then succeeds. */
  static class CountingThrower implements Callable<Integer> {

    int count = 0;
@@ -125,4 +125,21 @@ public class RetrierTest {
        .isEqualTo(0);
    assertThat(reporter.numBeforeRetry).isEqualTo(0);
  }
+
+  @Test
+  public void testRetryPredicate_succeedsWhenRetries() {
+    // Throws a retryable "1" exception is retryable, and then it succeeds on "1".
+    assertThat(retrier.callWithRetry(new CountingThrower(1), e -> e.getMessage().equals("1")))
+        .isEqualTo(1);
+  }
+
+  @Test
+  public void testRetryPredicate_failsWhenDoesntRetry() {
+    // Throws a retryable "1" exception, then a non-retryable "2" exception, resulting in failure.
+    CountingException ex =
+        assertThrows(
+            CountingException.class,
+            () -> retrier.callWithRetry(new CountingThrower(2), e -> e.getMessage().equals("1")));
+    assertThat(ex).hasMessageThat().isEqualTo("2");
+  }
 }
Author	SHA1	Message	Date
Lai Jiang	3106958f18	Instrument proxy frontend latency metric (#320 ) * Instrument proxy frontend latency metric Backend latency only captures the time spent waiting for the GAE backend to respond to a request, which is not representitive of what clients experience. For instance it does not take into account the time the proxy spends on processing the requests and the time it takes to send a response to the client. This PR adds a metric for frontend latency, which is the time spent on answering a client request. This should serve as a better proxy for the latency clients observe. * Rename AbstractMetrics to BaseMetrics	2019-10-29 16:02:47 -04:00
Weimin Yu	7da94c90dc	Restrict "Public Domain" license acceptance (#329 ) "Public Domain" license must be reviewed case by case. Removed blanket acceptance and named accepted dependencies individually. Also added a README file to warn about this license and WTFPL.	2019-10-28 13:32:42 -04:00
Ben McIlwain	dbb4092680	Don't retry permanent failures when uploading ICANN monthly reports (#328 ) * Don't retry permanent failures when uploading ICANN monthly reports There are two kinds of permanent failures that this checks for that we know will never succeed, so it makes no sense to continue retrying 11 more times before moving onto the next file to upload. These errors are: 1. com.google.api.client.http.HttpResponseException: 403 Your IP address xx.xx.xx.xx is not allowed to connect 2. com.google.api.client.http.HttpResponseException: 400 <?xml version="1.0" encoding="UTF-8" standalone="yes"?><response xmlns="urn:ietf:params:xml:ns:iirdea-1.0"><result code="2002"><msg>A report for that month already exists, the cut-off date already passed.</msg><description>Date: 2019-09</description></result></response> In order to implement this new functionality, this commit also adds a new way to call Retriable that allows specifying the isRetryable Predicate (which is quite useful).	2019-10-25 13:54:47 -04:00
Weimin Yu	af3223d086	Add a test task for all schema-dependent tests (#325 ) Such tests are used in server-schema compatibility tests. Also added a test that verifies that all eligible tests are included.	2019-10-25 13:25:01 -04:00
Shicong Huang	2d7f80baaf	Fix issues related to Cloud SQL connection (#321 ) * Reenable JpaTransactionManager for Alpha and Crash * Make UploadClaimsListCommand implement CommandWithCloudSql * Fix wrong call to get password * Use Cloud SQL Socket library to provision TransactionManager * Change to use dataSource configs	2019-10-25 12:40:43 -04:00
gbrodman	d4d5d6a570	Add another test contact for Registry Lock testing (#324 ) * Add another test contact for Registry Lock testing Previously, we only had two contacts -- one per registrar. This PR adds a second, registry-lock-enabled, contact to one registrar for two reasons: 1. For registry-lock-related testing, we'd like to be able to test both positively and negatively, making sure that the permissions work the way they should 2. In general, the UI tests should include the case where we have multiple contacts in the same registrar. Previously, this was never the case in tests. * Merge remote-tracking branch 'origin/master' into addTestContact	2019-10-25 10:24:59 -04:00
gbrodman	53c0be6537	Don't destroy existing registry lock passwords in contacts (#317 ) * Don't destroy existing registry lock passwords in contacts The existing code assumes that the "contacts" segment of the form contains an exact representation of the registrar contacts. This breaks when we have a contact with an existing registry lock password because we don't want to keep passing around that password in plain text (we never store it in plain text) This PR changes the code so that instead of assuming the contact is provided in its entirety, we load the contact from storage first (matching by email address) if it exists. We then set the required fields from the JSON object, and set the password optionally if it was provided. Alternatives: - Create a separate RegistrarContactPassword object with a RegistrarContact parent. This increases complexity significantly since we'd be adding a parent-child relationship and adding more objects to Datastore during the transition to SQL. It also doesn't completely solve the problem of "When should we set the password?" because the password field still must be part of the same form. - Rearrange the UI so that the password is set as part of a completely separate form with a separate submit action. This would be possible but is sub-optimal for two reasons. First, we are trying to not re-engineer the web console as much as possible since we're likely starting it from scratch before too long anyway. Second, we want the lock-password-setting to be part of the standard contact modification workflow. * Responses to CR * Actually we need to allow "removal" of fields * Remove optional * one-statement building the contacts	2019-10-24 20:18:37 -04:00
gbrodman	63bb2dd79b	Don't include password hash + salt in visible diffs (#322 ) We don't want to override toDiffableFieldMap because (per the javadoc) that is supposed to contain sensitive information. So, we should just remove it before sending it out.	2019-10-23 10:57:46 -07:00
gbrodman	8278b5409e	Add a registrarId index to RegistryLock (#312 ) * Add a registrarId index to RegistryLock * Merge remote-tracking branch 'origin/master' into getByRegistrar * Responses to CR	2019-10-23 06:51:20 -07:00
gbrodman	f98b0f8739	Use merge instead of persist for RegistryLockDao (#310 ) * Use merge instead of persist for RegistryLockDao * CR responses	2019-10-22 12:21:32 -07:00