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 coroutines #37

Merged
merged 1 commit into from Dec 13, 2016
Merged

Retry coroutines #37

merged 1 commit into from Dec 13, 2016

Conversation

bersace
Copy link
Contributor

@bersace bersace commented Nov 23, 2016

Hi,

This PR ports rholder/retrying#64 to tenacity. Thanks for the fork. The codebase is really clean.

It's a bit tricky to implement retry on coroutines. I did this by extracting the calling (aka attempt) and sleep logic from the retry control loop. This is done by using a generator. The control loop generate instructions (attempt, sleep or return), it's up to the wrapping code to do the actual IO, asynchronously or not.

I'm actually not very proud of this. Supporting old python versions and sync/async is a bit hackish. I'd be glad to hear your opinion on this contribution. Until it's mergeable, i wish :)

Regards,

@jd
Copy link
Owner

jd commented Nov 23, 2016

@Haypo I'd love to have your opinion on this patch!

@bersace
Copy link
Contributor Author

bersace commented Nov 24, 2016

cc @jpic

@bersace
Copy link
Contributor Author

bersace commented Nov 24, 2016

Grün :)

@@ -55,6 +58,11 @@

from tenacity import _utils

if sys.version_info > (3, 4):
Copy link

@vstinner vstinner Nov 24, 2016

Choose a reason for hiding this comment

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

asyncio is part of Python stdlib since Python 3.4, so it should be >=, no?

It is possible to install it as a third party module on Python 3.3, but I'm not sure that it's worth it to support Python 3.3 (it's no more used, and it wasn't never very popular).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

>>> (3, 4, 0) > (3, 4)
True

As you wish.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pyt3.3 is not tested on travis for this project :)



@asyncio.coroutine
def call(self, fn, *args, **kwargs):
Copy link

@vstinner vstinner Nov 24, 2016

Choose a reason for hiding this comment

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

"self" is a bad parameter name if it's not a method but a function. Call it maybe retrying?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, retrying is good.

Copy link

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

I'm not sure that the code works :-) asyncio is very complex and I'm confused by the yield used to retry in a function.

@bersace
Copy link
Contributor Author

bersace commented Nov 24, 2016

@Haypo do you mean the test case is not effective ?

@bersace
Copy link
Contributor Author

bersace commented Dec 1, 2016

@harlowja @jd do you want an implementation without iterator ? That's a nice piece of refacto. I might do this in two distinct PR.

@jd
Copy link
Owner

jd commented Dec 1, 2016

@bersace if you squash your fixup commits that will make that easier to review first.

If the code without iterator is simpler, give it a try in another PR, that might be interesting or help us review both PR :)

@bersace
Copy link
Contributor Author

bersace commented Dec 1, 2016

@jd let's try this.

@bersace bersace closed this Dec 13, 2016
@bersace bersace reopened this Dec 13, 2016
@bersace
Copy link
Contributor Author

bersace commented Dec 13, 2016

@jd @Haypo here you are :)

@bersace
Copy link
Contributor Author

bersace commented Dec 13, 2016

@jd rebased. One-commit PR to add retry on coroutines :)

Copy link
Owner

@jd jd left a comment

Choose a reason for hiding this comment

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

LGTM

elif isinstance(do, DoSleep):
result = NO_RESULT
exc_info = None
yield from asyncio.sleep(do)
Copy link
Owner

Choose a reason for hiding this comment

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

So you're really ignoring self.sleep?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

self.sleep is not in BaseRetrying but (Sync)Retrying. If you want, i can add a sleep=asyncio.sleep __init__() argument.

Copy link
Owner

Choose a reason for hiding this comment

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

You're right, I forgot that. I don't think it makes sense to have a different sleep here since it's tied to asyncio, so that's good. :)

@bersace
Copy link
Contributor Author

bersace commented Dec 13, 2016

Green \o/

@jd jd merged commit c3b02ab into jd:master Dec 13, 2016
@bersace bersace deleted the async branch December 13, 2016 14:58
@bersace
Copy link
Contributor Author

bersace commented Dec 13, 2016

@jd thanks. Do you plan a release soon ? :)

@jd
Copy link
Owner

jd commented Dec 13, 2016

@bersace just pushed 3.6.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants