andreaturli approved this pull request. looks good to me and very useful! Thanks!
`AzureRetryableErrorHandlerTest` suite is ok too Please update https://issues.apache.org/jira/browse/JCLOUDS-1294 accordingly. > + if (response.getStatusCode() != 429 || isRateLimitError(response)) { + return false; + } + + try { + // Note that this will consume the response body. At this point, + // subsequent retry handlers or error handlers will not be able to read + // again the payload, but that should only be attempted when the + // command is not retryable and an exception should be thrown. + Error error = parseError.apply(response); + logger.debug("processing error: %s", error); + + boolean isRetryable = RETRYABLE_ERROR_CODE.equals(error.details().code()); + return isRetryable ? super.shouldRetryRequest(command, response) : false; + } catch (Exception ex) { + // If we can't parse the error, just assume it is not a retryable error would it be worth to add a `logger.warn` that suggests what's happening? > @@ -30,10 +31,32 @@ @Singleton public class AzureRateLimitRetryHandler extends RateLimitRetryHandler { + private final AzureRetryableErrorHandler retryableErrorHandler; + + @Inject + AzureRateLimitRetryHandler(AzureRetryableErrorHandler retryableErrorHandler) { + this.retryableErrorHandler = retryableErrorHandler; + } + + @Override + protected boolean delayRequestUntilAllowed(HttpCommand command, HttpResponse response) { + if (!isRateLimitError(response)) { + // If it is not a rate-limit error but the response is a 429, delegate [minor] I'd rather move this to the javadoc, as it looks really important > @@ -330,7 +330,6 @@ public Provisionable get() { @Override public boolean apply(final String name) { checkNotNull(name, "name cannot be null"); - boolean present = false; thanks for cleaning those `present` booleans up! > + + assertFalse(handler.shouldRetryRequest(command, response)); + } + + @Test + public void testDoesNotRetryWhenErrorNotRetryable() { + String nonRetryable = "{\"error\":{\"code\":\"ReferencedResourceNotProvisioned\",\"message\":\"Not provisioned\"}}"; + HttpCommand command = new HttpCommand(HttpRequest.builder().method("GET").endpoint("http://localhost";).build()); + HttpResponse response = HttpResponse.builder().statusCode(429).payload(nonRetryable).build(); + + assertFalse(handler.shouldRetryRequest(command, response)); + } + + @Test + public void testRetriesWhenRetryableError() { + String nonRetryable = "{\"error\":{\"code\":\"RetryableError\",\"message\":\"Resource busy\"}}"; shouldn't this be `retryable` ? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1203#pullrequestreview-117521108
