You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Andrew Gaul <no...@github.com> on 2017/08/03 06:00:42 UTC
[jclouds/jclouds] Handle HTTP 429 in google-cloud-storage (#1126)
This addresses `rateLimitExceeded` errors encountered during integration
tests.
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/1126
-- Commit Summary --
* Handle HTTP 429 in google-cloud-storage
-- File Changes --
M providers/google-cloud-storage/src/main/java/org/jclouds/googlecloudstorage/config/GoogleCloudStorageHttpApiModule.java (8)
A providers/google-cloud-storage/src/main/java/org/jclouds/googlecloudstorage/handlers/GoogleCloudStorageClientErrorRetryHandler.java (45)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/1126.patch
https://github.com/jclouds/jclouds/pull/1126.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/1126
Re: [jclouds/jclouds] Handle HTTP 429 in google-cloud-storage (#1126)
Posted by Andrew Gaul <no...@github.com>.
Handling HTTP 429 seems like core should handle this.
--
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/1126#issuecomment-319875965
Re: [jclouds/jclouds] Handle HTTP 429 in google-cloud-storage (#1126)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> +
+import com.google.inject.Inject;
+
+@Singleton
+public final class GoogleCloudStorageClientErrorRetryHandler implements HttpRetryHandler {
+ private final BackoffLimitedRetryHandler backoffHandler;
+
+ @Inject
+ protected GoogleCloudStorageClientErrorRetryHandler(BackoffLimitedRetryHandler backoffHandler) {
+ this.backoffHandler = backoffHandler;
+ }
+
+ @Override
+ public boolean shouldRetryRequest(HttpCommand command, HttpResponse response) {
+ if (response.getStatusCode() == 429) {
+ return true;
According to [the best practices](https://cloud.google.com/storage/docs/request-rate#best_practices), we should use a backoff if there is a 429 error. However, returning true here will retry the request immediately. Shouldn't it apply the backoff for 429 errors and return false otherwise?
--
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/1126#pullrequestreview-54019857
Re: [jclouds/jclouds] Handle HTTP 429 in google-cloud-storage (#1126)
Posted by Ignasi Barrera <no...@github.com>.
> Handling HTTP 429 seems like core should handle this.
We already have the [RateLimitRetryHandler](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/handlers/RateLimitRetryHandler.java) in core. It is a base class to be extended by retry handlers that deal with the provider-specific rate limits. You have a couple example implementations for [digitalocean2](https://github.com/jclouds/jclouds/blob/master/providers/digitalocean2/src/main/java/org/jclouds/digitalocean2/handlers/DigitalOcean2RateLimitRetryHandler.java) and [azurecompute-arm](https://github.com/jclouds/jclouds-labs/blob/master/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/handlers/AzureRateLimitRetryHandler.java). Usually these rate limit handlers are configured in an [optional module](https://github.com/jclouds/jclouds/blob/master/providers/digitalocean2/src/main/java/org/jclouds/digitalocean2/config/DigitalOcean2RateLimitModule.java) to let users include it only if they want jclouds to automatically throttle the requests upon rate limit errors.
--
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/1126#issuecomment-319898015
Re: [jclouds/jclouds] Handle HTTP 429 in google-cloud-storage (#1126)
Posted by Ignasi Barrera <no...@github.com>.
Redargind the implementation, returning an absent will cause the request not to be retried. I think extending the rate limit handler does not make sense here, since we don't want to throttle the requests in a particular way. We just want to apply an exponential backoff if (and only if) the response code is 429.
I'd change the implementation class to directly extend the `BackoffLimitedRetryHandler`, and then just override the `shouldRetryRequest` method as follows:
```java
@Override
public boolean shouldRetryRequest(HttpCommand command, HttpResponse response) {
return response.getStatusCode() == 429 && super.shouldRetryRequest(command, response);
}
```
This way 429 errors will be retried using the exponential backoff, but no other 4xx error will be retried by defaut.
--
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/1126#issuecomment-320034324
Re: [jclouds/jclouds] Handle HTTP 429 in google-cloud-storage (#1126)
Posted by Andrew Gaul <no...@github.com>.
andrewgaul commented on this pull request.
> +import org.jclouds.http.HttpRetryHandler;
+
+import com.google.inject.Inject;
+
+@Singleton
+public final class GoogleCloudStorageClientErrorRetryHandler implements HttpRetryHandler {
+ private final BackoffLimitedRetryHandler backoffHandler;
+
+ @Inject
+ protected GoogleCloudStorageClientErrorRetryHandler(BackoffLimitedRetryHandler backoffHandler) {
+ this.backoffHandler = backoffHandler;
+ }
+
+ @Override
+ public boolean shouldRetryRequest(HttpCommand command, HttpResponse response) {
+ if (response.getStatusCode() == 429) {
Done.
--
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/1126#discussion_r131441897
Re: [jclouds/jclouds] Handle HTTP 429 in google-cloud-storage (#1126)
Posted by Andrew Gaul <no...@github.com>.
Switched back to `HttpRetryHandler` with corrected logic.
--
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/1126#issuecomment-320044598
Re: [jclouds/jclouds] Handle HTTP 429 in google-cloud-storage (#1126)
Posted by Andrew Gaul <no...@github.com>.
Updated the pull request to use `RateLimitRetryHandler`. Thanks for the pointer! I don't understand why these modules should be optional -- most users would want this behavior.
--
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/1126#issuecomment-320030518
Re: [jclouds/jclouds] Handle HTTP 429 in google-cloud-storage (#1126)
Posted by Andrew Gaul <no...@github.com>.
andrewgaul commented on this pull request.
> +
+import com.google.inject.Inject;
+
+@Singleton
+public final class GoogleCloudStorageClientErrorRetryHandler implements HttpRetryHandler {
+ private final BackoffLimitedRetryHandler backoffHandler;
+
+ @Inject
+ protected GoogleCloudStorageClientErrorRetryHandler(BackoffLimitedRetryHandler backoffHandler) {
+ this.backoffHandler = backoffHandler;
+ }
+
+ @Override
+ public boolean shouldRetryRequest(HttpCommand command, HttpResponse response) {
+ if (response.getStatusCode() == 429) {
+ return true;
Agree that this is backwards/wrong.
--
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/1126#discussion_r131197462
Re: [jclouds/jclouds] Handle HTTP 429 in google-cloud-storage (#1126)
Posted by Ignasi Barrera <no...@github.com>.
>I don't understand why these modules should be optional -- most users would want this behavior.
I think I kept it optional in DO to keep backward compatibility, initially. In Azure ARM, for example, it is included by default.
The point is that if the module is included in the metadata there is no trivial way to disable this functionality. Users may prefer to fail fast, instead of having a system that takes a very long time to complete a request, so I opted to, in general, provide a separate module users can include where needed.
--
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/1126#issuecomment-320032504
Re: [jclouds/jclouds] Handle HTTP 429 in google-cloud-storage (#1126)
Posted by Andrew Phillips <no...@github.com>.
demobox commented on this pull request.
Just a minor suggestion/comment
> +import org.jclouds.http.HttpRetryHandler;
+
+import com.google.inject.Inject;
+
+@Singleton
+public final class GoogleCloudStorageClientErrorRetryHandler implements HttpRetryHandler {
+ private final BackoffLimitedRetryHandler backoffHandler;
+
+ @Inject
+ protected GoogleCloudStorageClientErrorRetryHandler(BackoffLimitedRetryHandler backoffHandler) {
+ this.backoffHandler = backoffHandler;
+ }
+
+ @Override
+ public boolean shouldRetryRequest(HttpCommand command, HttpResponse response) {
+ if (response.getStatusCode() == 429) {
Would it make sense to add a link to documentation explaining what code 429 means?
--
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/1126#pullrequestreview-54403434