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

Support for retry event hook #50

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Support for retry event hook #50

wants to merge 6 commits into from

Conversation

bodenr
Copy link

@bodenr bodenr commented May 27, 2016

It's very common for consumers to perform some action on each
attempt iteration; for example logging a message that the operation
failed and a retry will be performed after some time. While this can
be done today using a custom wait_func, it's inconvenient to use
a partial to wrap an existing retrying sleep function just to get this
behavior.

This patch adds support for a new kwarg called wait_event_func
that when passed should reference a function to be called
before sleeping for another attempt iteration. This function
looks similar to retrying wait_funcs except its first arg is
the time to sleep as returned by the current 'wait' function.

A handful of unit tests are also included.

bodenr and others added 3 commits May 27, 2016 09:22
It's very common for consumers to perform some action on each
attempt iteration; for example logging a message that the operation
failed and a retry will be performed after some time. While this can
be done today using a custom wait_func, it's inconvenient to use
a partial to wrap an existing retrying sleep function just to get this
behavior.

This patch adds support for a new kwarg called wait_event_func
that when passed should reference a function to be called
before sleeping for another attempt iteration. This function
looks similar to retrying wait_funcs except its first arg is
the time to sleep as returned by the current 'wait' function.

A handful of unit tests are also included.
It's very common for consumers to perform some action on each
attempt iteration; for example logging a message that the operation
failed and a retry will be performed after some time. While this can
be done today using a custom wait_func, it's inconvenient to use
a partial to wrap an existing retrying sleep function just to get this
behavior.

This patch adds support for a new kwarg called wait_event_func
that when passed should reference a function to be called
before sleeping for another attempt iteration. This function
looks similar to retrying wait_funcs except its first arg is
the time to sleep as returned by the current 'wait' function.

A handful of unit tests are also included.
No assertIsNotNone() in py26 so change to a py26 compatible assert statement.
@bodenr
Copy link
Author

bodenr commented Jun 7, 2016

@jd @harlowja @rholder
I realize this PR has conflicts now, but I'd like to understand your initial impressions of this code to determine if it's worth my time continuing here.
Thanks

@jd
Copy link
Contributor

jd commented Jun 9, 2016

LGTM, feel free to rebase!

…nto retry_log_hook

Conflicts:
	test_retrying.py
@bodenr
Copy link
Author

bodenr commented Jun 9, 2016

travis didn't rebuild; let me try closing + reopening this

@bodenr bodenr closed this Jun 9, 2016
@bodenr bodenr reopened this Jun 9, 2016
Conflicts:
	AUTHORS.rst
	retrying.py
	test_retrying.py
@bodenr
Copy link
Author

bodenr commented Jun 9, 2016

@jd looks like this one is clean now.
My apologies for all the noise here; I took a bumpy path on a git adventure.

@harlowja
Copy link
Contributor

Seems pretty useful to me 👍

@bodenr
Copy link
Author

bodenr commented Jun 16, 2016

@jd @rholder what do you guys think; is this merge-worthy?

@jd
Copy link
Contributor

jd commented Jun 16, 2016

Looks redundant with wait_func, no?

@bodenr
Copy link
Author

bodenr commented Jun 16, 2016

@jd perhaps.

As indicated in the "commit message" its very common to want a "callback" per retry (for something like logging a message). While this could be done with a custom wait_func it inconvenient to decorate and provide a custom wait_func just for this purpose. That said the intent here is to simplify that consumption pattern.

@jd
Copy link
Contributor

jd commented Jun 16, 2016

I think we don't want to clutter more the init func. What about providing that functionality as part of a custom wait_func that would be provided by retrying itself?

Possibly a composable class.

@harlowja
Copy link
Contributor

I agree with jd (also why I did #55 to help move toward a model that hopefully can be more composeable and user-friendly); so perhaps we should get #55 in first?

@bodenr
Copy link
Author

bodenr commented Jun 16, 2016

@jd @harlowja
I just pushed a commit here with some changes I was playing with that introduce function chaining using the CallChain class I whipped up (no UTs; more of a talking point).

Using this approach it's easy to just pass my "hook function" as the wait_func to achieve what I need. It also address the "chaining" TODO in the code if I understand properly.

I haven't see #55 yet, so perhaps we can weight pros/cons of it vs. the CallChain I just committed to this PR.

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.

3 participants