Make the verify OT&E action more performant

As previously written, it loaded up all history entries into memory and then
processed them. This was OOMing for some registrars on sandbox who had performed
a large number of testing actions, most of them long OT&E was passed.

This commit changes the verify OT&E action to stream the history entries in
batches, ordered by when they were made, and then terminates once all tests have
passed. This prevents OOMing because only a single batch of history entries need
reside in memory at once.

This does necessitate the creation of a new composite Datastore index on
HistoryEntry, so we'll need to run the ResaveAllHistoryEntriesAction in sandbox
after this change is deployed before the new verify OT&E code will work.

Note that the "history viewer" is long dead, but that the pre-existing index
on HistoryEntries is still used for many other purposes.

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

java/google/registry/env/common/default/WEB-INF/datastore-indexes.xml

@@ -69,10 +69,14 @@
     <property name="clientId" direction="asc"/>
     <property name="eventTime" direction="asc"/>
   </datastore-index>
-  <!-- For the history viewer. -->
+  <!-- For querying HistoryEntries. -->
   <datastore-index kind="HistoryEntry" ancestor="true" source="manual">
     <property name="modificationTime" direction="asc"/>
   </datastore-index>
+  <datastore-index kind="HistoryEntry" ancestor="false" source="manual">
+    <property name="clientId" direction="asc"/>
+    <property name="modificationTime" direction="asc"/>
+  </datastore-index>
   <!-- For RDAP. -->
   <datastore-index kind="DomainBase" ancestor="false" source="manual">
     <property name="^i" direction="asc"/>

java/google/registry/tools/VerifyOteCommand.java

@@ -36,7 +36,12 @@ import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Objects;
 
-/** Command to verify that a registrar has passed OT&E. */
+/**
+ * Command to verify that a registrar has passed OT&E.
+ *
+ * <p>Outputted stats may be truncated at the point where all tests passed to avoid unnecessarily
+ * loading lots of data.
+ */
 @Parameters(
     separators = " =",
     commandDescription = "Verify passage of OT&E for specified (or all) registrars")

java/google/registry/tools/server/VerifyOteAction.java

@@ -28,6 +28,8 @@ import com.google.common.base.Predicates;
 import com.google.common.collect.HashMultiset;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Multiset;
+import com.googlecode.objectify.Key;
+import com.googlecode.objectify.cmd.Query;
 import google.registry.flows.EppException;
 import google.registry.model.domain.DomainCommand;
 import google.registry.model.domain.fee.FeeCreateCommandExtension;
@@ -44,12 +46,14 @@ import google.registry.request.JsonActionRunner;
 import google.registry.request.JsonActionRunner.JsonAction;
 import google.registry.request.auth.Auth;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.EnumSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
+import java.util.stream.IntStream;
 import java.util.stream.Stream;
 import javax.inject.Inject;
 
@@ -242,18 +246,30 @@ public class VerifyOteAction implements Runnable, JsonAction {
     /**
      * Records data in the passed historyEntryStats object on what actions have been performed by
      * the four numbered OT&E variants of the registrar name.
+     *
+     * <p>Stops when it notices that all tests have passed.
      */
     HistoryEntryStats recordRegistrarHistory(String registrarName) {
-      ImmutableList.Builder<String> clientIds = new ImmutableList.Builder<>();
+      ImmutableList<String> clientIds =
-      for (int i = 1; i <= 4; i++) {
+          IntStream.rangeClosed(1, 4)
-        clientIds.add(String.format("%s-%d", registrarName, i));
+              .mapToObj(i -> String.format("%s-%d", registrarName, i))
-      }
+              .collect(toImmutableList());
-      for (HistoryEntry historyEntry :
+
-          ofy().load().type(HistoryEntry.class).filter("clientId in", clientIds.build()).list()) {
+      Query<HistoryEntry> query =
+          ofy()
+              .load()
+              .type(HistoryEntry.class)
+              .filter("clientId in", clientIds)
+              .order("modificationTime");
+      for (HistoryEntry historyEntry : query) {
         try {
           record(historyEntry);
         } catch (EppException e) {
-          throw new RuntimeException(e);
+          throw new RuntimeException("Couldn't parse history entry " + Key.create(historyEntry), e);
+        }
+        // Break out early if all tests were passed.
+        if (wereAllTestsPassed()) {
+          break;
         }
       }
       return this;
@@ -264,18 +280,19 @@ public class VerifyOteAction implements Runnable, JsonAction {
       byte[] xmlBytes = historyEntry.getXmlBytes();
       // xmlBytes can be null on contact create and update for safe-harbor compliance.
       final Optional<EppInput> eppInput =
-          (xmlBytes == null)
+          (xmlBytes == null) ? Optional.empty() : Optional.of(unmarshal(EppInput.class, xmlBytes));
-              ? Optional.empty()
-              : Optional.of(unmarshal(EppInput.class, xmlBytes));
       if (!statCounts.addAll(
-          EnumSet.allOf(StatType.class)
+          EnumSet.allOf(StatType.class).stream()
-              .stream()
               .filter(statType -> statType.matches(historyEntry.getType(), eppInput))
               .collect(toImmutableList()))) {
         statCounts.add(StatType.UNCLASSIFIED_FLOWS);
       }
     }
 
+    boolean wereAllTestsPassed() {
+      return Arrays.stream(StatType.values()).allMatch(s -> statCounts.count(s) >= s.requirement);
+    }
+
     /**
      * Returns a list of failure messages describing any cases where the passed stats fail to meet
      * the required thresholds, or the empty list if all requirements are met.

javatests/google/registry/tools/server/VerifyOteActionTest.java

@@ -17,9 +17,11 @@ package google.registry.tools.server;
 import static com.google.common.truth.Truth.assertThat;
 import static google.registry.testing.DatastoreHelper.deleteResource;
 import static google.registry.testing.DatastoreHelper.persistResource;
+import static google.registry.util.DateTimeUtils.END_OF_TIME;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import google.registry.model.eppcommon.Trid;
 import google.registry.model.reporting.HistoryEntry;
 import google.registry.model.reporting.HistoryEntry.Type;
 import google.registry.testing.AppEngineRule;
@@ -40,9 +42,9 @@ public class VerifyOteActionTest {
 
   private final VerifyOteAction action = new VerifyOteAction();
 
-  HistoryEntry hostDeleteHistoryEntry;
+  private HistoryEntry hostDeleteHistoryEntry;
-  HistoryEntry domainCreateHistoryEntry;
+  private HistoryEntry domainCreateHistoryEntry;
-  HistoryEntry domainRestoreHistoryEntry;
+  private HistoryEntry domainRestoreHistoryEntry;
 
   @Before
   public void init() throws Exception {
@@ -137,13 +139,20 @@ public class VerifyOteActionTest {
                 .setType(Type.HOST_DELETE)
                 .setXmlBytes(ToolsTestData.loadBytes("host_delete.xml").read())
                 .build());
+    // Persist 10 host updates for a total of 25 history entries. Since these also sort last by
+    // modification time, when these cause all tests to pass, only the first will be recorded and
+    // the rest will be skipped.
+    for (int i = 0; i < 10; i++) {
       persistResource(
           new HistoryEntry.Builder()
               .setClientId("blobio-1")
               .setType(Type.HOST_UPDATE)
               .setXmlBytes(ToolsTestData.loadBytes("host_update.xml").read())
+              .setTrid(Trid.create(null, String.format("blahtrid-%d", i)))
+              .setModificationTime(END_OF_TIME)
               .build());
     }
+  }
 
   @Test
   public void testSuccess_summarize_allPass() {
@@ -165,7 +174,7 @@ public class VerifyOteActionTest {
             ImmutableMap.of("summarize", "true", "registrars", ImmutableList.of("blobio")));
     assertThat(response)
         .containsExactly(
-            "blobio", "# actions:   26 - Reqs: [-.-----.------.-] 13/16 - Overall: FAIL");
+            "blobio", "# actions:   35 - Reqs: [-.-----.------.-] 13/16 - Overall: FAIL");
   }
 
   @Test
@@ -235,7 +244,7 @@ public class VerifyOteActionTest {
               + ".*"
               + "host creates subordinate: 1\n"
               + "host deletes: 0\n"
-              + "host updates: 1\n"
+              + "host updates: 10\n"
               + ".*"
               + "Requirements passed: 15/16\n"
               + "Overall OT&E status: FAIL\n";