Skip to content

Conversation

@jsiembida
Copy link

The original commit doesn't explain why retry=3 was introduced in the first place.
Before, it was default, zero. Now it seems to conflict with retry decorator used
in some call in managers code resulting in 16 retries in total when combined.

The original commit doesn't explain why retry=3 was introduced in the first place.
Before, it was default, zero. Now it seems to conflict with retry decorator used
in some call in managers code resulting in 16 retries in total when combined.
@coveralls
Copy link

coveralls commented Apr 17, 2019

Coverage Status

Coverage decreased (-0.002%) to 91.773% when pulling aa67c28 on jsiembida:reduntant-retry-logic-removal into cfab07e on softlayer:master.

@allmightyspiff
Copy link
Member

The retry decorator exists to retry application errors, and the retry adapter exists to retry connection errors.

Having them retry the same error is certainly not intended for sure, but removing the retry adapter isn't the solution.

I think removing the exceptions.TransportError from the RETRIABLE in https://github.com/softlayer/softlayer-python/blob/master/SoftLayer/decoration.py will fix the 16 retries issue. If you want to test that out and change your pull request I'll merge it.

I'm also ok with tuneing down the retries to 2 each, but the retry adapter needs to be kept in.

@allmightyspiff allmightyspiff self-requested a review April 17, 2019 23:45
Copy link
Member

@allmightyspiff allmightyspiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To fix the Retry adapter and the Retry decorator both activating, remove the TransportError exception from the retry decorator instead.

@allmightyspiff
Copy link
Member

I've fixed this in #1150 by removing TransportError from one of the retryable errors.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants