Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Fix missing "Retry-After" header and fix integration tests#678

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
max-wittig merged 2 commits intopython-gitlab:masterfromappian:backoff-requests
Jan 13, 2019

Conversation

srikanthchelluri
Copy link
Contributor

@srikanthchellurisrikanthchelluri commentedJan 9, 2019•
edited
Loading

This PR does two things: a) implements an exponential backoff in the case that we are throttled and "Retry-After" isn't in the response header (this currently causes aKeyError), b) fix an integration test because we were decoding something that was already a string.

Closes#632.

valdisrigdon and max-wittig reacted with thumbs up emoji
When requests are throttled (HTTP response code 429), python-gitlabassumed that 'Retry-After' existed in the response headers. This isnot always the case and so the request fails due to a KeyError. Thechange in this commit adds a rudimentary exponential backoff to the'http_request' method, which defaults to 10 retries but can be setto -1 to retry without bound.
The integration tests failed because a test called 'decode()' on astring-type variable - the GitLabException class handles byte-to-stringconversion already in its __init__. This commit removes the call to'decode()' in the test.```Traceback (most recent call last):  File "./tools/python_test_v4.py", line 801, in <module>    assert 'Retry later' in error_message.decode()AttributeError: 'str' object has no attribute 'decode'```
@max-wittig
Copy link
Member

max-wittig commentedJan 9, 2019•
edited
Loading

Thanks for this contribution! Looks pretty good. Any way to easily test this? (e.g. against a public GitLab instance withoutRetry-After header?)

@srikanthchelluri
Copy link
ContributorAuthor

srikanthchelluri commentedJan 10, 2019•
edited
Loading

@max-wittig Hm, this is a bit difficult to test. But here's how I validated the code path - let me know what you think.

  1. Created a virtual environment and built the library with the following code:
whileTrue:# result = self.session.send(prepped, timeout=timeout, **settings)result=requests.get('https://httpstat.us/429')self._check_redirects(result)if200<=result.status_code<300:returnresultif429==result.status_codeandobey_rate_limit:ifmax_retries==-1orcur_retries<max_retries:wait_time=2**cur_retries*0.1if"Retry-AfterXYZ"inresult.headers:wait_time=int(result.headers["Retry-After"])cur_retries+=1print('DEBUG: backing off',str(wait_time))time.sleep(wait_time)continue

Note thathttps://httpstat.us returns a response with theRetry-After header set to3600. I first validated that we backed off 3600 seconds; then I changed the header we were checking for (Retry-AfterXYZ as shown above) and validated that we were backing off exponentially (see below).

  1. Wrote this small script:
fromgetpassimportgetpassimportgitlabprivate_token=getpass('GitLab API Token: ')gl=gitlab.Gitlab('https://gitlab.com/',private_token=private_token)project=gl.projects.get(<some-project-id-we-own>)project.members.list(all=True)
  1. Validated the exponential backoff:
(env) ➜  python-gitlab python throttle.py GitLab API Token: DEBUG: backing off 0.1DEBUG: backing off 0.2DEBUG: backing off 0.4DEBUG: backing off 0.8DEBUG: backing off 1.6DEBUG: backing off 3.2DEBUG: backing off 6.4DEBUG: backing off 12.8DEBUG: backing off 25.6DEBUG: backing off 51.2Traceback (most recent call last):  File "/Users/srikanth.chelluri/repo/python-gitlab/gitlab/exceptions.py", line 251, in wrapped_f    return f(*args, **kwargs)  File "/Users/srikanth.chelluri/repo/python-gitlab/gitlab/mixins.py", line 49, in get    server_data = self.gitlab.http_get(path, **kwargs)  File "/Users/srikanth.chelluri/repo/python-gitlab/gitlab/__init__.py", line 542, in http_get    streamed=streamed, **kwargs)  File "/Users/srikanth.chelluri/repo/python-gitlab/gitlab/__init__.py", line 520, in http_request    response_body=result.content)gitlab.exceptions.GitlabHttpError: 429: 429 Too Many RequestsDuring handling of the above exception, another exception occurred:Traceback (most recent call last):  File "throttle.py", line 10, in <module>    project = gl.projects.get(8244461)  File "/Users/srikanth.chelluri/repo/python-gitlab/gitlab/exceptions.py", line 253, in wrapped_f    raise error(e.error_message, e.response_code, e.response_body)gitlab.exceptions.GitlabGetError: 429: 429 Too Many Requests

Thoughts/feedback on this approach? Didn't try spinning up and configuring a GitLab instance without the header since we don't really need it to test the code path.

@max-wittigmax-wittig self-assigned thisJan 12, 2019
@max-wittig
Copy link
Member

@srikanthchelluri Works as expected. Many thanks for the contribution 👍

Just a hint for future contributions: It's best practice to use a separate branch in your fork. Makes it easier for us to checkout the changes and easier for you to maintain your next contribution. Otherwise your master in your fork is polluted by another commit etc.

@max-wittigmax-wittig merged commit89679ce intopython-gitlab:masterJan 13, 2019
@srikanthchelluri
Copy link
ContributorAuthor

@max-wittig This was a separate branch, no? I forked the repo into my org (appian), then checked out a new branch calledbackoff-requests. Let me know if there's a better practice.

Thanks for the review and merge 👍

@max-wittig
Copy link
Member

max-wittig commentedJan 13, 2019•
edited
Loading

ah really. Okay weird. Nevermind. Maybe I miss-typed something and didn't see it. Not sure 😄

srikanthchelluri reacted with laugh emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@max-wittigmax-wittigmax-wittig approved these changes

Assignees

@max-wittigmax-wittig

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Retry-After header may be missing: need default timeout
2 participants
@srikanthchelluri@max-wittig
[8]ページ先頭
©2009-2025 Movatter.jp