Added requests retry mechanism for all requests call in _pulp_client #53

midnightercz · 2019-04-16T13:29:41Z

To make sure tool process isn't interrupted due network issues,
retry mechanism based on urllib3 Retry was implemented.
New configuration environment variables to customize this
were added:

UBIPOP_HTTP_TOTAL_RETRIES
UBIPOP_HTTP_RETRY_BACKOFF

fixes #32

To make sure tool process isn't interrupted due network issues, retry mechanism based on urllib3 Retry was implemented. New configuration environment variables to customize this were added: - UBIPOP_HTTP_TOTAL_RETRIES - UBIPOP_HTTP_RETRY_BACKOFF

alexandrevicenzi

Does this retry on Exceptions as well? Like ConnectionTimeout

ubipop/_pulp_client.py

tests/test_pulp.py

alexandrevicenzi · 2019-04-16T13:54:38Z

ubipop/_pulp_client.py

@@ -9,11 +13,27 @@

 _LOG = logging.getLogger("ubipop")

+HTTP_TOTAL_RETRIES = os.environ.get("UBIPOP_HTTP_TOTAL_RETRIES", 10)
+UBIPOP_HTTP_RETRY_BACKOFF = os.environ.get("UBIPOP_HTTP_RETRY_BACKOFF", 1)


with backoff=1, it means that will retry after 0, 2, 4, 8 (secs) and so on, 10 times can take ages, maybe 0.1 or 0.2 would be a better number no?

I took the value from existing project that we have for helping with requests to pulp. But for sure I'm willing to change to anything we agree on.

maybe 5 times' enough

alexandrevicenzi · 2019-04-16T14:00:04Z

ubipop/_pulp_client.py

+    def __init__(self, *args, **kwargs):
+        kwargs['max_retries'] = Retry(
+            total=kwargs.get('total_retries', HTTP_TOTAL_RETRIES),
+            status_forcelist=[500, 502, 503, 504],


some 4xx also make sense here, for example: 429 Too Many Requests

Again, these are from existing projects. But sure we can add more status codes there

Maybe makes sense to make that configurable.

- Added test params descriptions - Removed unneeded code - fixed linter errors

ubipop/_pulp_client.py

- fixed constant names inconsistency - fixed typecast for env variables

midnightercz · 2019-04-17T07:29:45Z

Does this retry on Exceptions as well? Like ConnectionTimeout

Sorry that I overlooked your comment, urllib3 also retries for errors like timeout, socket error, protocol error and similar.

tests/test_pulp.py

midnightercz · 2019-04-24T14:10:58Z

I had to rebased this, so please approve this again

New commit pushed

danrodrig · 2019-04-25T09:11:23Z

tests/test_pulp.py

+                 env_retries, retry_call, retry_args, ok_response, expected_retries):
+    global HTTP_TOTAL_RETRIES
+    if env_retries:
+        HTTP_TOTAL_RETRIES = env_retries


Would be nice to save a restore original value.

danrodrig · 2019-04-25T09:25:05Z

tests/test_pulp.py

+        HTTP_TOTAL_RETRIES = env_retries
+
+    retries = [make_mock_response(err_status_code, 'Fake Http error')
+               for _ in range(HTTP_TOTAL_RETRIES)[:-1]] + [make_mock_response(200, ok_response)]


range(HTTP_TOTAL_RETRIES - 1)

And the 'ok_response' could be added in the next line, would make the code more readable.

ubipop/_pulp_client.py

- backoff factor env variable type casted as float

…lation-tool into retry-pulp

danrodrig

UBIPOP_HTTP_RETRY_BACKOFF should allow floats.

alexandrevicenzi · 2019-04-26T09:28:42Z

tests/test_pulp.py

@@ -1,8 +1,30 @@
-from ubipop._pulp_client import Repo, Package, ModuleDefaults, Pulp
+try:
+    from importlib import reload


does this import make sense? not used, if fails nothing happens

not sure what you mean. Python3 doesn't have reload globally available

But where is used?

Looks like reload is not used.

And if we need the import for an specific version better use sys.version_info.

I see, I removed that code and forgot to remove that import

alexandrevicenzi · 2019-04-26T09:30:52Z

ubipop/_pulp_client.py

@@ -1,4 +1,7 @@
 import requests
+from urllib3.util.retry import Retry
+import os


imports could be sorted better

alexandrevicenzi · 2019-04-26T09:34:41Z

tests/test_pulp.py

+)
+def test_retries(set_backoff_to_zero_fixture, mocked_getresponse, mock_pulp, should_retry, err_status_code,
+                 env_retries, retry_call, retry_args, ok_response, expected_retries):
+    global HTTP_TOTAL_RETRIES


Just an idea, I don't like global keyword, the best way would be to use pulp_client.HTTP_TOTAL_RETRIES = value.

I think global is fine here, the variable is used in this scope.

alexandrevicenzi · 2019-04-26T11:15:32Z

tests/test_pulp.py

+            HTTP_TOTAL_RETRIES = env_retries
+
+        retries = [make_mock_response(err_status_code, 'Fake Http error')
+                   for _ in range(HTTP_TOTAL_RETRIES)[:-1]]


range(HTTP_TOTAL_RETRIES - 1)

danrodrig · 2019-04-29T12:27:06Z

ubipop/_pulp_client.py

@@ -9,11 +12,25 @@

 _LOG = logging.getLogger("ubipop")

+HTTP_TOTAL_RETRIES = int(os.environ.get("UBIPOP_HTTP_TOTAL_RETRIES", 10))
+HTTP_RETRY_BACKOFF = float(os.environ.get("UBIPOP_HTTP_RETRY_BACKOFF", 1))


Personally, I think that the defined empty should be handled. But If nobody else thinks it's an issue I'm not requesting the change.

danrodrig

Some minor issues, non-blocking. LGTM

- easier iteration over HTTP_TOTAL_RETRIES

midnightercz requested a review from a team April 16, 2019 13:30

alexandrevicenzi reviewed Apr 16, 2019

View reviewed changes

- Fixed python versions incompatiblities

a66c02a

- Added test params descriptions - Removed unneeded code - fixed linter errors

midnightercz force-pushed the retry-pulp branch from 4dbac80 to a66c02a Compare April 16, 2019 14:41

fixed python3 incompatible import

23d2fc8

rohanpm requested changes Apr 16, 2019

View reviewed changes

ubipop/_pulp_client.py Show resolved Hide resolved

rohanpm mentioned this pull request Apr 17, 2019

requests.Session should not be shared between threads #57

Closed

- added tests for env variables

c7e0434

- fixed constant names inconsistency - fixed typecast for env variables

rohanpm reviewed Apr 17, 2019

View reviewed changes

tests/test_pulp.py Outdated Show resolved Hide resolved

Removed unneeded tests

d6e18b0

rohanpm previously approved these changes Apr 24, 2019

View reviewed changes

Merge branch 'master' into retry-pulp

6f69e12

midnightercz requested review from rohanpm and a team April 24, 2019 14:09

rbikar previously approved these changes Apr 24, 2019

View reviewed changes

midnightercz requested a review from danrodrig April 25, 2019 08:24

danrodrig reviewed Apr 25, 2019

View reviewed changes

ubipop/_pulp_client.py Outdated Show resolved Hide resolved

midnightercz added 2 commits April 25, 2019 12:59

- restore original http retries in tests

f3fa369

- backoff factor env variable type casted as float

Merge branch 'retry-pulp' of https://github.com/midnightercz/ubi-popu…

4111fe8

…lation-tool into retry-pulp

midnightercz dismissed rbikar’s stale review via 4111fe8 April 25, 2019 11:02

danrodrig suggested changes Apr 25, 2019

View reviewed changes

midnightercz requested a review from danrodrig April 25, 2019 12:14

rbikar previously approved these changes Apr 26, 2019

View reviewed changes

alexandrevicenzi reviewed Apr 26, 2019

View reviewed changes

danrodrig reviewed Apr 29, 2019

View reviewed changes

danrodrig previously approved these changes Apr 29, 2019

View reviewed changes

- removed unused import

6730d82

- easier iteration over HTTP_TOTAL_RETRIES

midnightercz dismissed stale reviews from danrodrig and rbikar via 6730d82 April 29, 2019 15:36

midnightercz requested review from danrodrig and rbikar April 29, 2019 15:40

rbikar approved these changes Apr 29, 2019

View reviewed changes

danrodrig approved these changes Apr 29, 2019

View reviewed changes

midnightercz merged commit 58a74b1 into release-engineering:master Apr 29, 2019

alexandrevicenzi mentioned this pull request May 3, 2019

Failing tests on Python 3.7 #82

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added requests retry mechanism for all requests call in _pulp_client #53

Added requests retry mechanism for all requests call in _pulp_client #53

midnightercz commented Apr 16, 2019

alexandrevicenzi left a comment

alexandrevicenzi Apr 16, 2019

midnightercz Apr 16, 2019

JayZ12138 Apr 23, 2019

alexandrevicenzi Apr 16, 2019

midnightercz Apr 16, 2019

danrodrig Apr 25, 2019

midnightercz commented Apr 17, 2019

midnightercz commented Apr 24, 2019

danrodrig Apr 25, 2019

danrodrig Apr 25, 2019

danrodrig left a comment

alexandrevicenzi Apr 26, 2019

midnightercz Apr 26, 2019 •

edited

Loading

alexandrevicenzi Apr 29, 2019

danrodrig Apr 29, 2019

midnightercz Apr 29, 2019

alexandrevicenzi Apr 26, 2019

alexandrevicenzi Apr 26, 2019

danrodrig Apr 29, 2019

alexandrevicenzi Apr 26, 2019

danrodrig Apr 29, 2019

danrodrig left a comment

Added requests retry mechanism for all requests call in _pulp_client #53

Added requests retry mechanism for all requests call in _pulp_client #53

Conversation

midnightercz commented Apr 16, 2019

alexandrevicenzi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

midnightercz commented Apr 17, 2019

midnightercz commented Apr 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danrodrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

midnightercz Apr 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danrodrig left a comment

Choose a reason for hiding this comment

midnightercz Apr 26, 2019 •

edited

Loading