handle 429 responses with exponential backoff retries

apache / openwhisk-client-js

JavaScript client library for the Apache OpenWhisk platform

https://openwhisk.apache.org/

Apache License 2.0

83 stars 53 forks source link

handle 429 responses with exponential backoff retries #148

Open alexkli opened 5 years ago

alexkli commented 5 years ago

When using openwhisk.action.invoke() or other operations one can encounter HTTP 429 TOO MANY REQUESTS status code responses from Openwhisk's rate limiting. It would be nice if the openwhisk js client can provide a retry mechanism out of the box.

The suggested approach is a exponential backoff. This was implemented e.g. here: https://github.com/apache/incubator-openwhisk-package-cloudant/pull/90

alexkli commented 5 years ago

Some thoughts on a possible implementation:

AFAICS, a retry would have to be added in Resources.operation() where it would apply to most operations (actions, triggers, ...).
Maybe it's necessary to limit it to certain operations only (GETs, invoke + fire) or maybe not, since it's a generic HTTP mechanism.
The API should allow to configure the timeouts, e.g. as part of the openwhisk constructor options.

I could provide a PR if that helps.

starpit commented 5 years ago

FYI prior discussion: https://github.com/apache/incubator-openwhisk-client-js/issues/119

alexkli commented 5 years ago

@starpit I saw it, but it was mentioning network level failures. I am quite explicitly interested in handling 429 rate limit errors, since we can get them consistently with too much traffic 😄

jthomas commented 5 years ago

As discussed in #119, I didn't agree with introducing generic retry behaviour for all lower-level networking issues. However, I can see the case for a retry mechanism for explicit cases like the HTTP 429 returned by the platform API.

This should be turned off by default ("no surprises for the user and no breaking changes") and configurable through constructor options as @alexkli suggested. Exponential back-off is the correct approach.

@alexkli If you want to provide a PR - we can discuss the approach?

I was going to start the process of a release this library to push out the recent change (https://github.com/apache/incubator-openwhisk-client-js/pull/147) but will wait until this is in?

Maybe it would be better to implement this in the client module rather than the resource class? Resources could pass a new parameter to indicate whether 429 errors with retryable. https://github.com/apache/incubator-openwhisk-client-js/blob/master/lib/client.js

alexkli commented 5 years ago

I could take a look at it next week, but I wouldn’t want to block any release for it :-)

alexkli commented 5 years ago

Right, in the client.js. I missed that this is where the http request logic is actually implemented.

jthomas commented 5 years ago

Hello @alexkli - do you know if you'll be able to submit a PR this week? No rush - but as some other PRs have come in I'm now thinking about getting that release out.

alexkli commented 5 years ago

@jthomas Unfortunately no, some other priorities pushed this down on my list. With the right concurrency settings in openwhisk the 429 problem went away for us (at least for now). Still planning to take a look maybe in a few weeks.