Skip to content

Automatically retry when rate limit is reached #23

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 13, 2017

Conversation

budirec
Copy link
Contributor

@budirec budirec commented Aug 30, 2017

No description provided.

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Aug 30, 2017
@SendGridDX
Copy link

SendGridDX commented Aug 30, 2017

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@thinkingserious thinkingserious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, we would like this feature to be implemented with the same features here. I think it's OK to start simple, but please design in a way that would allow upgrades. For example, to enable, we should pass in an object that allows us to configure the settings.

Thanks for starting this off!

lib/Client.php Outdated
*
* @return Response object
*/
public function makeRequest($method, $url, $body = null, $headers = null)
public function makeRequest($method, $url, $body = null, $headers = null, $retryOnLimit = true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, this should be set to null to so we don't have a breaking change.

@budirec
Copy link
Contributor Author

budirec commented Aug 31, 2017

I don't believe handling other error is a good idea. it should be handled by the user.
Next improvement maybe to add flag to throw error instead of returning the error

@thinkingserious
Copy link
Contributor

Hi @budirec,

Could you please update the tests? https://travis-ci.org/sendgrid/php-http-client/jobs/270245199#L292

Thanks!


if ($statusCode == 429 && $retryOnLimit) {
$headers = $response->headers(true);
$sleepDurations = $headers['X-Ratelimit-Reset'] - time();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain the idea behind this algorithm.

I want to make sure we don't overload our servers when something has gone wrong. I was hoping for a backoff strategy that eventually fails after a certain period of time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every request we made to sendgrid will have X-Ratelimit-Reset in the header, where it tell us when the rate limit is reset in sendgrid.
So when ever we have error 429(rate limit reached) we use X-Ratelimit-Reset to determine how long the script need to wait before make another call to sendgrid.
This way we don't use a random time or predefined time.

@thinkingserious thinkingserious merged commit 8091fc5 into sendgrid:master Sep 13, 2017
@thinkingserious
Copy link
Contributor

Hello @budirec,

Thanks again for the PR!

We want to show our appreciation by sending you some swag. Could you please fill out this form so we can send it to you? Thanks!

Team SendGrid DX

Falseclock pushed a commit to Falseclock/php-http-client that referenced this pull request Jan 30, 2019
Automatically retry when rate limit is reached
Falseclock pushed a commit to Falseclock/php-http-client that referenced this pull request Jan 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: code review request requesting a community code review or review from Twilio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants