feat(exceptions): Add SpinnakerRetrofitErrorHandler and replace RetrofitError catch blocks

kayenta-atlas/kayenta-atlas.gradle

@@ -8,4 +8,6 @@ dependencies {
   api "com.squareup.okhttp:okhttp-apache"
 
   api "org.apache.commons:commons-io:1.3.2"
+
+  api "io.spinnaker.kork:kork-retrofit"
 }

kayenta-atlas/src/main/java/com/netflix/kayenta/atlas/backends/AtlasStorageUpdater.java

@@ -20,14 +20,15 @@
 import com.netflix.kayenta.atlas.service.AtlasStorageRemoteService;
 import com.netflix.kayenta.retrofit.config.RemoteService;
 import com.netflix.kayenta.retrofit.config.RetrofitClientFactory;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
 import com.netflix.spinnaker.security.AuthenticatedRequest;
 import com.squareup.okhttp.OkHttpClient;
 import java.util.Map;
 import javax.validation.constraints.NotNull;
 import lombok.Builder;
 import lombok.Getter;
 import lombok.extern.slf4j.Slf4j;
-import retrofit.RetrofitError;
 import retrofit.converter.JacksonConverter;
 
 @Slf4j
@@ -58,11 +59,13 @@ boolean run(
       Map<String, Map<String, AtlasStorage>> atlasStorageMap =
           AuthenticatedRequest.allowAnonymous(atlasStorageRemoteService::fetch);
       atlasStorageDatabase.update(atlasStorageMap);
-    } catch (RetrofitError e) {
-      log.warn("While fetching atlas backends from " + uri, e);
-      return succeededAtLeastOnce;
+      succeededAtLeastOnce = true;
+    } catch (SpinnakerHttpException e) {
+      log.warn(e.getResponseCode() + " error while fetching atlas backends from " + uri, e);
+    } catch (SpinnakerServerException e) {
+      log.warn("Error while fetching atlas backends from " + uri, e);
     }
-    succeededAtLeastOnce = true;
-    return true;
+
+    return succeededAtLeastOnce;
   }
 }

kayenta-atlas/src/main/java/com/netflix/kayenta/atlas/backends/BackendUpdater.java

@@ -20,14 +20,15 @@
 import com.netflix.kayenta.atlas.service.BackendsRemoteService;
 import com.netflix.kayenta.retrofit.config.RemoteService;
 import com.netflix.kayenta.retrofit.config.RetrofitClientFactory;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
 import com.netflix.spinnaker.security.AuthenticatedRequest;
 import com.squareup.okhttp.OkHttpClient;
 import java.util.List;
 import javax.validation.constraints.NotNull;
 import lombok.Builder;
 import lombok.Getter;
 import lombok.extern.slf4j.Slf4j;
-import retrofit.RetrofitError;
 import retrofit.converter.JacksonConverter;
 
 @Slf4j
@@ -57,11 +58,13 @@ boolean run(
     try {
       List<Backend> backends = AuthenticatedRequest.allowAnonymous(backendsRemoteService::fetch);
       backendDatabase.update(backends);
-    } catch (RetrofitError e) {
-      log.warn("While fetching atlas backends from " + uri, e);
-      return succeededAtLeastOnce;
+      succeededAtLeastOnce = true;
+    } catch (SpinnakerHttpException e) {
+      log.warn(e.getResponseCode() + " error while fetching atlas backends from " + uri, e);
+    } catch (SpinnakerServerException e) {
+      log.warn("Error while fetching atlas backends from " + uri, e);
     }
-    succeededAtLeastOnce = true;
-    return true;
+
+    return succeededAtLeastOnce;
   }
 }

kayenta-core/kayenta-core.gradle

@@ -2,6 +2,7 @@ dependencies {
   api "redis.clients:jedis"
   api "io.spinnaker.kork:kork-core"
   api "io.spinnaker.kork:kork-web"
+  api "io.spinnaker.kork:kork-retrofit"
   api "com.netflix.spectator:spectator-api"
 
   // TODO update korkSwagger?

kayenta-core/src/main/java/com/netflix/kayenta/metrics/SynchronousQueryProcessor.java

@@ -24,6 +24,8 @@
 import com.netflix.kayenta.storage.StorageServiceRepository;
 import com.netflix.spectator.api.Id;
 import com.netflix.spectator.api.Registry;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException;
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult;
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus;
 import java.io.IOException;
@@ -34,9 +36,7 @@
 import java.util.UUID;
 import lombok.extern.slf4j.Slf4j;
 import org.springframework.beans.factory.annotation.Autowired;
-import org.springframework.http.HttpStatus;
 import org.springframework.stereotype.Component;
-import retrofit.RetrofitError;
 
 @Component
 @Slf4j
@@ -88,10 +88,8 @@ public String executeQuery(
             metricsService.queryMetrics(
                 metricsAccountName, canaryConfig, canaryMetricConfig, canaryScope);
         success = true;
-      } catch (RetrofitError e) {
-
-        boolean retryable = isRetryable(e);
-        if (retryable) {
+      } catch (SpinnakerHttpException e) {
+        if (e.getRetryable()) {
           retries++;
           if (retries >= retryConfiguration.getAttempts()) {
             throw e;
@@ -101,18 +99,38 @@ public String executeQuery(
             Thread.sleep(backoffPeriod);
           } catch (InterruptedException ignored) {
           }
-          Object error = e.getResponse() != null ? e.getResponse().getStatus() : e.getCause();
           log.warn(
               "Got {} result when querying for metrics. Retrying request (current attempt: "
                   + "{}, max attempts: {}, last backoff period: {}ms)",
-              error,
+              e.getResponseCode(),
               retries,
               retryConfiguration.getAttempts(),
               backoffPeriod);
         } else {
           throw e;
         }
-      } catch (IOException | UncheckedIOException | RetryableQueryException e) {
+      } catch (SpinnakerNetworkException e) {
+        if (e.getRetryable()) {
+          retries++;
+          if (retries >= retryConfiguration.getAttempts()) {
+            throw e;
+          }
+          long backoffPeriod = getBackoffPeriodMs(retries);
+          try {
+            Thread.sleep(backoffPeriod);
+          } catch (InterruptedException ignored) {
+          }
+          log.warn(
+                  "Got network error when querying for metrics. Retrying request (current attempt: "
+                          + "{}, max attempts: {}, last backoff period: {}ms)",
+                  retries,
+                  retryConfiguration.getAttempts(),
+                  backoffPeriod);
+        } else {
+          throw e;
+        }
+      }
+      catch (IOException | UncheckedIOException | RetryableQueryException e) {
         retries++;
         if (retries >= retryConfiguration.getAttempts()) {
           throw e;
@@ -146,28 +164,6 @@ private long getBackoffPeriodMs(int retryAttemptNumber) {
         * retryConfiguration.getBackoffPeriodMultiplierMs();
   }
 
-  private boolean isRetryable(RetrofitError e) {
-    if (isNetworkError(e)) {
-      // retry in case of network errors
-      return true;
-    }
-    if (e.getResponse() == null) {
-      // We don't have a network error, but the response is null. It's better to not retry these.
-      return false;
-    }
-    HttpStatus responseStatus = HttpStatus.resolve(e.getResponse().getStatus());
-    if (responseStatus == null) {
-      return false;
-    }
-    return retryConfiguration.getStatuses().contains(responseStatus)
-        || retryConfiguration.getSeries().contains(responseStatus.series());
-  }
-
-  private boolean isNetworkError(RetrofitError e) {
-    return e.getKind() == RetrofitError.Kind.NETWORK
-        || (e.getResponse() == null && e.getCause() instanceof IOException);
-  }
-
   public Map<String, ?> processQueryAndReturnMap(
       String metricsAccountName,
       String storageAccountName,

kayenta-core/src/main/java/com/netflix/kayenta/retrofit/config/RetrofitClientFactory.java

@@ -20,6 +20,7 @@
 
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.netflix.spinnaker.kork.annotations.VisibleForTesting;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler;
 import com.netflix.spinnaker.retrofit.Slf4jRetrofitLogger;
 import com.squareup.okhttp.Authenticator;
 import com.squareup.okhttp.Credentials;
@@ -119,6 +120,7 @@ public <T> T createClient(
         .setEndpoint(endpoint)
         .setClient(new OkClient(okHttpClient))
         .setConverter(converter)
+        .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
         .setLogLevel(RestAdapter.LogLevel.valueOf(retrofitLogLevel))
         .setLog(logger)
         .build()

kayenta-core/src/test/groovy/com/netflix/kayenta/metrics/SynchronousQueryProcessorTest.java

@@ -45,6 +45,8 @@
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
+
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;

kayenta-objectstore-configbin/src/main/java/com/netflix/kayenta/configbin/storage/ConfigBinStorageService.java

@@ -30,6 +30,9 @@
 import com.netflix.kayenta.storage.ObjectType;
 import com.netflix.kayenta.storage.StorageService;
 import com.netflix.kayenta.util.Retry;
+import com.netflix.spinnaker.kork.exceptions.SpinnakerException;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
 import com.netflix.spinnaker.kork.web.exceptions.NotFoundException;
 import com.netflix.spinnaker.security.AuthenticatedRequest;
 import com.squareup.okhttp.MediaType;
@@ -45,7 +48,6 @@
 import lombok.extern.slf4j.Slf4j;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.util.StringUtils;
-import retrofit.RetrofitError;
 
 @Builder
 @Slf4j
@@ -87,7 +89,7 @@ public <T> T loadObject(String accountName, ObjectType objectType, String object
                       () -> remoteService.get(ownerApp, configType, objectKey),
                       MAX_RETRIES,
                       RETRY_BACKOFF));
-    } catch (RetrofitError e) {
+    } catch (SpinnakerServerException e) {
       throw new NotFoundException("No such object named " + objectKey);
     }
 
@@ -337,7 +339,7 @@ private Map<String, Object> metadataFor(ConfigBinNamedAccountCredentials credent
                       () -> remoteService.get(ownerApp, configType, id),
                       MAX_RETRIES,
                       RETRY_BACKOFF));
-    } catch (RetrofitError e) {
+    } catch (SpinnakerServerException e) {
       throw new IllegalArgumentException("No such object named " + id);
     }
 

kayenta-prometheus/src/test/java/com/netflix/kayenta/prometheus/service/PrometheusRemoteServiceTest.java

@@ -27,6 +27,7 @@
 import com.netflix.kayenta.prometheus.model.PrometheusResults;
 import com.netflix.kayenta.retrofit.config.RemoteService;
 import com.netflix.kayenta.retrofit.config.RetrofitClientFactory;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
 import com.squareup.okhttp.OkHttpClient;
 import java.io.File;
 import java.nio.charset.StandardCharsets;
@@ -42,7 +43,6 @@
 import org.mockserver.client.MockServerClient;
 import org.mockserver.junit.MockServerRule;
 import org.mockserver.model.MediaType;
-import retrofit.RetrofitError;
 
 public class PrometheusRemoteServiceTest {
   @Rule public MockServerRule mockServerRule = new MockServerRule(this, true);
@@ -78,7 +78,7 @@ public void isHealthyReturnsInternalServerError() {
         .respond(response().withStatusCode(500));
 
     assertThatThrownBy(() -> prometheusRemoteService.isHealthy())
-        .isInstanceOf(RetrofitError.class)
+        .isInstanceOf(SpinnakerServerException.class)
         .hasMessageContaining("500 Internal Server Error");
   }
 

kayenta-signalfx/src/main/java/com/netflix/kayenta/signalfx/metrics/SignalFxMetricsService.java

@@ -35,6 +35,7 @@
 import com.netflix.kayenta.signalfx.service.SignalFlowExecutionResult;
 import com.netflix.kayenta.signalfx.service.SignalFxRequestError;
 import com.netflix.kayenta.signalfx.service.SignalFxSignalFlowRemoteService;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
 import com.signalfx.signalflow.ChannelMessage;
 import java.time.Duration;
 import java.time.Instant;
@@ -50,7 +51,6 @@
 import lombok.Singular;
 import lombok.extern.slf4j.Slf4j;
 import org.springframework.beans.factory.annotation.Autowired;
-import retrofit.RetrofitError;
 
 @Builder
 @Slf4j
@@ -144,8 +144,9 @@ public List<MetricSet> queryMetrics(
       signalFlowExecutionResult =
           signalFlowService.executeSignalFlowProgram(
               accessToken, startEpochMilli, endEpochMilli, stepMilli, maxDelay, immediate, program);
-    } catch (RetrofitError e) {
-      ErrorResponse errorResponse = (ErrorResponse) e.getBodyAs(ErrorResponse.class);
+    } catch (SpinnakerServerException e) {
+      ErrorResponse errorResponse = new ErrorResponse();
+      errorResponse.setMessage(e.getMessage());
       throw new SignalFxRequestError(
           errorResponse, program, startEpochMilli, endEpochMilli, stepMilli, metricsAccountName);
     }