-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add support for rate limiting #21
Conversation
9cd740e
to
e0651ae
Compare
235ac82
to
346c540
Compare
README.md
Outdated
* `max_retries=` sets the maximum number of retries for failed requests, default: 20 | ||
* `backoff_factor=` sets the exponential backoff factor, default: 2 | ||
|
||
Set it to 0 to disable it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need more detail in this sentence, should max_retries
or backoff_factor
be set to zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
chartmogul/retry_request.py
Outdated
def requests_retry_session(retries=20, backoff_factor=2, session=None,): | ||
session = session or requests.Session() | ||
adapter = _retry_adapter(retries, backoff_factor) | ||
session.mount('http://', adapter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this allow requests to be made over HTTP? If so, do you think we should remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
test/api/test_retry_request.py
Outdated
import unittest | ||
import httpretty | ||
import chartmogul | ||
from unittest.mock import patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import doesn't seem used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was an obsolete test. Removed.
# backoff_factor set as 0 to avoid waiting while testing | ||
config = Config("token", "secret", None, 4, 0) | ||
try: | ||
DataSource.create(config, data={ "test_date": date(2015, 1, 1) }).get() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a mock too right? Sorry I couldn't see where we mock it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is mocked by httpretty here
31 httpretty.register_uri(
32 httpretty.POST,
33 "https://api.chartmogul.com/v1/data_sources",
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
README.md
Outdated
@@ -65,6 +65,17 @@ This throws error or returns `<Ping{data='pong!'}>` | |||
You can also pass to the Config initializer: | |||
* `request_timeout=` sets timeout for requests (seconds), default: none (see [requests docs](http://docs.python-requests.org/en/master/user/quickstart/#timeouts) for details) | |||
|
|||
### Rate Limits & Exponential Backoff | |||
The library will keep retrying if the request exceeds the rate limit or if there's any network related error. | |||
By default, the request will be retried for 20 times (approximated 15 minutes) before finally giving up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'approximated' should be 'approximately'.
Ah the build failed, dismissing my review.
210652d
to
d6b5795
Compare
@bilbof Do you agree with the version changed to 1.2.0 instead of 1.1.9? This looks like a backwards compatible feature more than a bug fix, right? |
@spyrbri I think it's worth it. If a user has added their own retry logic for e.g. a 500 error, this may create bugs in their code - or at least, lots of additional unnecessary requests. Even if it doesn't, I don't see a negative from this small version bump. I guess we'll need to create a release + release doc for this. |
Adding support for rate limiting.
Allow configuration of max_retries and backoff_factor.