You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Ignasi Barrera <no...@github.com> on 2018/05/04 07:29:29 UTC
[jclouds/jclouds] JCLOUDS-1294: Attempt to retry RetryableErrors in
Azure ARM (#1203)
https://issues.apache.org/jira/browse/JCLOUDS-1294
This adds a handler for Azure `RetryableError` codes. Note that there is a second commit in the PR with some small cleanup that is not relevant to this PR.
@danielestevez Could you give this branch a try?
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/1203
-- Commit Summary --
* JCLOUDS-1294: Attempt to retry RetryableErrors in Azure ARM
* Cleanup unused variables
-- File Changes --
M core/src/main/java/org/jclouds/http/handlers/RateLimitRetryHandler.java (2)
M providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/config/AzurePredicatesModule.java (6)
A providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/domain/Error.java (58)
M providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/handlers/AzureComputeErrorHandler.java (13)
M providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/handlers/AzureRateLimitRetryHandler.java (23)
A providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/handlers/AzureRetryableErrorHandler.java (80)
M providers/azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/features/VaultApiLiveTest.java (23)
M providers/azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/features/VaultApiMockTest.java (3)
A providers/azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/handlers/AzureRetryableErrorHandlerTest.java (88)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/1203.patch
https://github.com/jclouds/jclouds/pull/1203.diff
--
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
Re: [jclouds/jclouds] JCLOUDS-1294: Attempt to retry RetryableErrors
in Azure ARM (#1203)
Posted by Ignasi Barrera <no...@github.com>.
Closed #1203.
--
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#event-1609392221
Re: [jclouds/jclouds] JCLOUDS-1294: Attempt to retry RetryableErrors
in Azure ARM (#1203)
Posted by Andrea Turli <no...@github.com>.
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
Re: [jclouds/jclouds] JCLOUDS-1294: Attempt to retry RetryableErrors
in Azure ARM (#1203)
Posted by Andrea Turli <no...@github.com>.
andreaturli approved this pull request.
+1
thanks
--
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-117528611
Re: [jclouds/jclouds] JCLOUDS-1294: Attempt to retry RetryableErrors
in Azure ARM (#1203)
Posted by Daniel Estévez <no...@github.com>.
Well, i can confirm it fixes the failing tests :clap:
I'll run the full suite of LiveTests to see if i find more problems
--
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#issuecomment-386736498
Re: [jclouds/jclouds] JCLOUDS-1294: Attempt to retry RetryableErrors
in Azure ARM (#1203)
Posted by Ignasi Barrera <no...@github.com>.
@nacx pushed 1 commit.
72b76a4 Address review comments
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1203/files/c0db82e6796fde2a2bcdd0e585b3bf9bec10c643..72b76a4e7daee5974e29fb89e1b7948ac3712acd
Re: [jclouds/jclouds] JCLOUDS-1294: Attempt to retry RetryableErrors
in Azure ARM (#1203)
Posted by Ignasi Barrera <no...@github.com>.
Merged to [master](http://git-wip-us.apache.org/repos/asf/jclouds/commit/b29f716a) and [2.1.x](http://git-wip-us.apache.org/repos/asf/jclouds/commit/b144d9f4).
--
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#issuecomment-386544321
Re: [jclouds/jclouds] JCLOUDS-1294: Attempt to retry RetryableErrors
in Azure ARM (#1203)
Posted by Ignasi Barrera <no...@github.com>.
Thanks!
--
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#issuecomment-386785419