From 3e7e6fc1c5605388d8ea97f4746d9ee6b4bbb0a3 Mon Sep 17 00:00:00 2001
From: Edwin Kempin <ekempin@google.com>
Date: Thu, 7 Jun 2018 15:45:28 +0200
Subject: [PATCH] Emails: Retry failed index queries when querying accounts by
 preferred email

Accessing the secondary index may sometimes fail due to a temporary
issue with the index.

Temporary issues with accessing the index are not common with Lucene but
at Google where we have a custom index implementation we have such
short-term failures from time to time.

Change I59aa974ced implemented retrying request validation during
auto-close because looking up accounts by preferred email could fail.
Since the retry is now done on the lower level in the Emails class,
ReceiveCommits no longer needs to retry the request validation.

Change-Id: I58147ac8d9fa837053504c1de6ff695cc8461ef3
Signed-off-by: Edwin Kempin <ekempin@google.com>
---
 .../google/gerrit/server/account/Emails.java  | 30 ++++++++++++++-----
 .../server/git/receive/ReceiveCommits.java    | 16 +---------
 2 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/java/com/google/gerrit/server/account/Emails.java b/java/com/google/gerrit/server/account/Emails.java
index 31e845b2a7..8a481677fe 100644
--- a/java/com/google/gerrit/server/account/Emails.java
+++ b/java/com/google/gerrit/server/account/Emails.java
@@ -16,6 +16,7 @@ package com.google.gerrit.server.account;
 
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 
+import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.collect.Streams;
@@ -23,6 +24,9 @@ import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.server.account.externalids.ExternalId;
 import com.google.gerrit.server.account.externalids.ExternalIds;
 import com.google.gerrit.server.query.account.InternalAccountQuery;
+import com.google.gerrit.server.update.RetryHelper;
+import com.google.gerrit.server.update.RetryHelper.Action;
+import com.google.gerrit.server.update.RetryHelper.ActionType;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -34,11 +38,16 @@ import java.io.IOException;
 public class Emails {
   private final ExternalIds externalIds;
   private final Provider<InternalAccountQuery> queryProvider;
+  private final RetryHelper retryHelper;
 
   @Inject
-  public Emails(ExternalIds externalIds, Provider<InternalAccountQuery> queryProvider) {
+  public Emails(
+      ExternalIds externalIds,
+      Provider<InternalAccountQuery> queryProvider,
+      RetryHelper retryHelper) {
     this.externalIds = externalIds;
     this.queryProvider = queryProvider;
+    this.retryHelper = retryHelper;
   }
 
   /**
@@ -63,7 +72,8 @@ public class Emails {
   public ImmutableSet<Account.Id> getAccountFor(String email) throws IOException, OrmException {
     return Streams.concat(
             externalIds.byEmail(email).stream().map(ExternalId::accountId),
-            queryProvider.get().byPreferredEmail(email).stream().map(a -> a.getAccount().getId()))
+            executeIndexQuery(() -> queryProvider.get().byPreferredEmail(email).stream())
+                .map(a -> a.getAccount().getId()))
         .collect(toImmutableSet());
   }
 
@@ -80,12 +90,18 @@ public class Emails {
         .entries()
         .stream()
         .forEach(e -> builder.put(e.getKey(), e.getValue().accountId()));
-    queryProvider
-        .get()
-        .byPreferredEmail(emails)
-        .entries()
-        .stream()
+    executeIndexQuery(() -> queryProvider.get().byPreferredEmail(emails).entries().stream())
         .forEach(e -> builder.put(e.getKey(), e.getValue().getAccount().getId()));
     return builder.build();
   }
+
+  private <T> T executeIndexQuery(Action<T> action) throws OrmException {
+    try {
+      return retryHelper.execute(ActionType.INDEX_QUERY, action, OrmException.class::isInstance);
+    } catch (Exception e) {
+      Throwables.throwIfUnchecked(e);
+      Throwables.throwIfInstanceOf(e, OrmException.class);
+      throw new OrmException(e);
+    }
+  }
 }
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 568b966cfd..1c3747c78c 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -2973,7 +2973,7 @@ class ReceiveCommits {
 
               for (ReplaceRequest req : replaceAndClose) {
                 Change.Id id = req.notes.getChangeId();
-                if (!executeRequestValidation(() -> req.validate(true))) {
+                if (!req.validate(true)) {
                   logDebug("Not closing %s because validation failed", id);
                   continue;
                 }
@@ -3017,20 +3017,6 @@ class ReceiveCommits {
     }
   }
 
-  private <T> T executeRequestValidation(Action<T> action)
-      throws IOException, PermissionBackendException, OrmException {
-    try {
-      // The request validation needs to do an account query to lookup accounts by preferred email,
-      // if that index query fails the request validation should be retried.
-      return retryHelper.execute(ActionType.INDEX_QUERY, action, OrmException.class::isInstance);
-    } catch (Exception t) {
-      Throwables.throwIfInstanceOf(t, IOException.class);
-      Throwables.throwIfInstanceOf(t, PermissionBackendException.class);
-      Throwables.throwIfInstanceOf(t, OrmException.class);
-      throw new OrmException(t);
-    }
-  }
-
   private void updateAccountInfo() {
     if (setFullNameTo == null) {
       return;