Skip to content
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

Retry on API failure when connecting from builders #4902

Merged
merged 2 commits into from Nov 19, 2018

Conversation

@humitos
Copy link
Member

@humitos humitos commented Nov 14, 2018

I used 3 retries for now at 0.5, 1 and 2 seconds.

We can test with these values and we can increase later if we consider necessary.

Closes #2258

@humitos humitos force-pushed the humitos/api/retry-on-fail branch from f80fb1e to d9a9af1 Nov 14, 2018
connect=3,
status=3,
backoff_factor=0.5, # 0.5, 1, 2 seconds
method_whitelist=('GET', 'PUT', 'PATCH', 'POST'),
Copy link
Member Author

@humitos humitos Nov 14, 2018

POST could be a little dangerous here, but considering that we are using it to create a new BuildCommand I think it's necessary.

https://github.com/rtfd/readthedocs.org/blob/d9a9af14260fcf9ffcbb63cd0b986d7f1a83079a/readthedocs/doc_builder/environments.py#L227

Copy link
Member

@stsewd stsewd Nov 14, 2018

From the docs

By default, we only retry on methods which are considered to be idempotent (multiple requests with the same parameters end with the same state). See Retry.DEFAULT_METHOD_WHITELIST.

https://urllib3.readthedocs.io/en/latest/reference/urllib3.util.html#urllib3.util.retry.Retry.DEFAULT_METHOD_WHITELIST

Copy link
Contributor

@agjohnson agjohnson Nov 15, 2018

I'd be a little concerned about the POST retry as well, but these are the calls that we're currently failing on, so I'm not sure if there is anything we want to do differently here.

@humitos humitos requested a review from Nov 14, 2018
@humitos humitos force-pushed the humitos/api/retry-on-fail branch from d9a9af1 to eee2c36 Nov 14, 2018
@humitos
Copy link
Member Author

@humitos humitos commented Nov 14, 2018

Lint is failing with

    pylint: import-error / Unable to import 'requests.packages.urllib3.util.retry'

Copy link
Contributor

@agjohnson agjohnson left a comment

I think the fix looks good! I don't have a strong opinion on POST support, it seems to be a requirement that we retry POST requests.

@humitos
Copy link
Member Author

@humitos humitos commented Nov 19, 2018

In the worst case, retrying on POST will give us duplicated BuildCommand; which is not good at all but at least it won't break anything. We could merge and investigate after deploy or maybe build something to de-duplicate these entries :/

@codecov
Copy link

@codecov codecov bot commented Nov 19, 2018

Codecov Report

Merging #4902 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4902      +/-   ##
==========================================
+ Coverage   76.65%   76.65%   +<.01%     
==========================================
  Files         158      158              
  Lines       10057    10059       +2     
  Branches     1269     1269              
==========================================
+ Hits         7709     7711       +2     
  Misses       2007     2007              
  Partials      341      341
Impacted Files Coverage Δ
readthedocs/restapi/client.py 87.87% <100%> (+0.78%) ⬆️

@humitos humitos merged commit 4b7478c into master Nov 19, 2018
3 checks passed
@humitos humitos deleted the humitos/api/retry-on-fail branch Nov 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants