[APIC-79] - reconfigure retry logic using tenacity #401

byndcivilization · 2020-08-26T16:39:59Z

This reimplements retry logic using tenacity. Requirements are:
[X] retry on is [429, 502, 503, and 504]
[X] retry on ['HEAD', 'TRACE', 'GET', 'PUT', 'OPTIONS', 'DELETE']
[X] Retry-After header is present, we use that value for the retry interval.
[X] Retry POSTs on 429s and 503s.
[X] default max total time 10 minutes
[X] default max tries = 10
[X] exponential backoff with full jitter

byndcivilization · 2020-08-27T23:56:42Z

Okay @skomanduri it looks like you were right on the urllib part. Bifurcation of different force lists for different verbs seems painful. This is at 60-70ish% so i wanted to check in on approach before i started on the respect_retry_after_header part, some issues with retries on the cli and tests. theres also still some tearout work and I'm not sure how you want me to handle file buffering retries. It seems like the best course here is to leave what we have in place and add some jitter to the back off calculation. I had a working backoff algo on a different branch that I can port back in.

civis/base.py

byndcivilization · 2020-08-28T22:46:44Z

Okay @skomanduri how does this look. Tests still need work but otherwise i think this works. Theres some funniness with the interaction of retry conditions in that if we raise a connection error for status the codes will not be available in as part of the exception object. This retry after header class and only the status code check gets around this and makes the response headers available for inspection.

skomanduri

This looks good. I had one question, but otherwise let me know when you are ready for final review.

civis/_utils.py

byndcivilization · 2020-09-10T17:21:19Z

Okay @skomanduri this should be ready for review

mheilman · 2020-09-10T21:29:06Z

just a minor thing: could you please add a changelog entry?

skomanduri

Couple more questions, but this seems almost ready to go!

README.rst

civis/_utils.py

skomanduri · 2020-09-10T21:39:04Z

civis/_utils.py

+        # retry_state is an instance of tenacity.RetryCallState.
+        # The .outcome property contains the result/exception
+        # that came from the underlying function.
+        result_headers = retry_state.outcome._result.headers


Do you need to account for outcome returning an exception here?

We're not throwing exceptions so I don't think so unless I'm missing something? The retry logic is using status code alone to trigger a retry. The reason for this is the default requests exception handling doesnt seem to pass back any of the response information for requests to handle. At one point I had a raise_for_status call in there hoping that i could set the retry parameter to something like lambda res: res.status_code in civis.civis.RETRY_CODES & StatusException. That would never trigger though because res.status_code didnt exist in the retry object.

Also to take a step back this is sort of unrelated to the cause of the retry. All this class is doing is overriding the wait parameter with the header value if it exists. Does that make sense?

@skomanduri does that answer your question?

Wouldn't retry_state.outcome contain an exception if requests throws an exception? The situation I'm worried about is where that happens and then trying to call .headers on it would throw its own exception and we would lose the requests exception. I don't know if this happens, but want to either rule it out or handle it.

@skomanduri I'm unclear on what type of exception you are envisioning. I thought the only exceptions that requests would implicitly return are ConnectionError, Timeout, and TooManyRedirects. I don't think any of these would come into play with the retry logic as it currently exists. Since we're purely using the status_code returned on the response object, that wait time class will only be evaluated once the retry logic has been entered afaik.

Is there another set of exceptions that I'm missing here?

That said, currently if an exception is thrown currently the retry is not triggered.

That said, currently if an exception is thrown currently the retry is not triggered.

Okay, that's the piece I was missing. I thought if there was a ConnectionError it would still evaluate here, but if not then this would be fine. Can you add a test for no retry on ConnectionError?

Okay @skomanduri test is last on the test_utils.py file.

That's what I had in mind for the test. WFM!

@skomanduri can i get the final approval so i can merge this?

civis/resources/_resources.py

byndcivilization · 2020-09-10T22:18:45Z

@mheilman yeah for sure. this is probably a good time to ask about release process. do we just bump the version here and release or is there a cadence that we follow.

…city * master: [APIC-314] Refactored File Cleaning Logic (#405) [APIC-307] fix a workflows usage example in docs/source/client.rst (#409) add some missing PR numbers to the changelog (#408) Update the API spec at `civis/tests/civis_api_spec.json` so that new endpoints are included (e.g., `/exports/files/csv`) (#407) Updated list of base API resources to include `aliases`, `git_repos`, `json_values`, `services`, and `storage_hosts` so that they show up in the sphinx docs (#406) [APIC-284] remove python 3.5 support since it is reaching EOL (#404) [APIC-305] Require sql_type for every column when table_columns is provided (CHANGELOG) (#403) [APIC-313] retain specific sql types in civis_file_to_table (#402)

Co-authored-by: Saranga Komanduri <skomanduri@civisanalytics.com>

mheilman · 2020-09-10T22:24:54Z

do we just bump the version here and release or is there a cadence that we follow.

There isn't a set cadence for releases. The list of unreleased changes is getting pretty long right now, so I was thinking of making one after this and maybe one more PR, so you can just leave the version number as is.

byndcivilization · 2020-09-10T22:26:34Z

rad thanks

skomanduri

LGTM

* initial implementation of tenacity * added comment * got docs on how to stash cert * savepont * retry working * added class as trigger and propegated * everything except client working * cleanup * cleanup * cleanup * working as wrapper on call * added retry for client invoke * moved max retries var and cleanup * implemented retry_error_callback as a passthrough and some cleanup * tearout * tearout * cleanup * respect retry headers * respect retry-after header * respect retry-after header * flake fixes * add jitter to retry deco for files * working on tests * fixed some tests * all tests passing * renamed io retry tests for clarity * fixed flak8 * fixed flak8 * tests for retry * cleanup * cleanup * changlog * rebased * Update README.rst Co-authored-by: Saranga Komanduri <skomanduri@civisanalytics.com> * typo * added connectionerror test * cleanup * cleanup Co-authored-by: Saranga Komanduri <skomanduri@civisanalytics.com>

byndcivilization added 2 commits August 26, 2020 11:34

initial implementation of tenacity

a19e3ac

added comment

2d330f1

byndcivilization requested a review from skomanduri August 26, 2020 16:40

byndcivilization added 8 commits August 26, 2020 12:35

got docs on how to stash cert

375b871

savepont

a5d7460

retry working

6a7e5e4

added class as trigger and propegated

9eae3e3

everything except client working

c66b0d6

cleanup

79bfd5c

cleanup

333a91f

cleanup

9ff71b5

skomanduri reviewed Aug 28, 2020

View reviewed changes

civis/base.py Outdated Show resolved Hide resolved

byndcivilization added 11 commits August 28, 2020 15:45

working as wrapper on call

b58cf40

added retry for client invoke

acb941e

moved max retries var and cleanup

d0a63a9

implemented retry_error_callback as a passthrough and some cleanup

ecc4a61

tearout

13d1c44

tearout

8810229

cleanup

e1b124e

respect retry headers

741fec0

respect retry-after header

0947b14

respect retry-after header

545ccb5

flake fixes

492a174

add jitter to retry deco for files

3cba154

skomanduri reviewed Aug 31, 2020

View reviewed changes

civis/_utils.py Show resolved Hide resolved

byndcivilization added 3 commits September 8, 2020 11:14

working on tests

fd6528c

fixed some tests

450f565

all tests passing

e16b10f

byndcivilization added 6 commits September 9, 2020 17:05

renamed io retry tests for clarity

1c5fb2e

fixed flak8

d6ae8e3

fixed flak8

565da01

tests for retry

e4a72f6

cleanup

c519a58

cleanup

e0245a5

skomanduri reviewed Sep 10, 2020

View reviewed changes

changlog

758f82a

byndcivilization and others added 3 commits September 10, 2020 17:19

rebased

913a3fc

Update README.rst

3b2c4ba

Co-authored-by: Saranga Komanduri <skomanduri@civisanalytics.com>

byndcivilization added 4 commits September 10, 2020 17:36

typo

98ad203

added connectionerror test

f3e6c5b

cleanup

2b05edd

cleanup

024df6e

skomanduri approved these changes Sep 24, 2020

View reviewed changes

byndcivilization merged commit 151a017 into master Sep 24, 2020

byndcivilization deleted the APIC-79-reconfigure-retry-logic-using-tenacity branch September 24, 2020 21:11

jacksonlee-civis mentioned this pull request Nov 10, 2021

[CIVIS-1642] FIX deprecate the inactive retry_total parameter of civis.APIClient #431

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APIC-79] - reconfigure retry logic using tenacity #401

[APIC-79] - reconfigure retry logic using tenacity #401

byndcivilization commented Aug 26, 2020 •

edited

byndcivilization commented Aug 27, 2020 •

edited

byndcivilization commented Aug 28, 2020

skomanduri left a comment

byndcivilization commented Sep 10, 2020

mheilman commented Sep 10, 2020

skomanduri left a comment

skomanduri Sep 10, 2020

byndcivilization Sep 10, 2020

byndcivilization Sep 14, 2020 •

edited

skomanduri Sep 14, 2020

byndcivilization Sep 15, 2020 •

edited

skomanduri Sep 16, 2020

byndcivilization Sep 23, 2020 •

edited

skomanduri Sep 23, 2020

byndcivilization Sep 24, 2020

byndcivilization commented Sep 10, 2020

mheilman commented Sep 10, 2020

byndcivilization commented Sep 10, 2020

skomanduri left a comment

[APIC-79] - reconfigure retry logic using tenacity #401

[APIC-79] - reconfigure retry logic using tenacity #401

Conversation

byndcivilization commented Aug 26, 2020 • edited

byndcivilization commented Aug 27, 2020 • edited

byndcivilization commented Aug 28, 2020

skomanduri left a comment

Choose a reason for hiding this comment

byndcivilization commented Sep 10, 2020

mheilman commented Sep 10, 2020

skomanduri left a comment

Choose a reason for hiding this comment

skomanduri Sep 10, 2020

Choose a reason for hiding this comment

byndcivilization Sep 10, 2020

Choose a reason for hiding this comment

byndcivilization Sep 14, 2020 • edited

Choose a reason for hiding this comment

skomanduri Sep 14, 2020

Choose a reason for hiding this comment

byndcivilization Sep 15, 2020 • edited

Choose a reason for hiding this comment

skomanduri Sep 16, 2020

Choose a reason for hiding this comment

byndcivilization Sep 23, 2020 • edited

Choose a reason for hiding this comment

skomanduri Sep 23, 2020

Choose a reason for hiding this comment

byndcivilization Sep 24, 2020

Choose a reason for hiding this comment

byndcivilization commented Sep 10, 2020

mheilman commented Sep 10, 2020

byndcivilization commented Sep 10, 2020

skomanduri left a comment

Choose a reason for hiding this comment

byndcivilization commented Aug 26, 2020 •

edited

byndcivilization commented Aug 27, 2020 •

edited

byndcivilization Sep 14, 2020 •

edited

byndcivilization Sep 15, 2020 •

edited

byndcivilization Sep 23, 2020 •

edited