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

Add hooks for @retry decorator #162

Open
prkumar opened this issue May 26, 2019 · 5 comments
Open

Add hooks for @retry decorator #162

prkumar opened this issue May 26, 2019 · 5 comments

Comments

@prkumar
Copy link
Owner

prkumar commented May 26, 2019

Is your feature request related to a problem? Please describe.
This FR is from @liiight on gitter:

So i was trying to add metrics to get a good sense of our requests/rate limit issue and while i can add a metric to the response via a response_handler, I can not do the same per each retry. Maybe you should have a retry_handler that will be used for those requests.

Describe the solution you'd like

One way we could approach this is with a on_retry argument for the @retry decorator. This would be a function that is called when the retry condition is met, and it would get the most recent exception or failed response on each invocation:

@retry(..., on_retry=count_retries)
@get('users')
def get_users(self):
    pass

Alternatively, we could add the hook using a @retry.handler decorator:

@retry.handler(count_retries)
@retry(...)
@get('users')
def get_users(self):
    pass
@prkumar prkumar added this to the v0.10.0 milestone May 26, 2019
@liiight
Copy link

liiight commented Jun 19, 2019

@prkumar this issue is really problematic for me. Is there any way I can help with this? You think this would make a good issue to contribute to?

@prkumar prkumar pinned this issue Jun 20, 2019
@prkumar
Copy link
Owner Author

prkumar commented Jun 20, 2019

@liiight I've added the Critical label to this issue and pinned it, so I'll prioritize the work for this one! If you'd like to tackle a solution, I'd suggest looking at the uplink/retry/retry.py module. Specifically, these lines:

def after_response(self, request, response):
if self._condition.should_retry_after_response(response):
return self._next_delay()
else:
self._reset()
def after_exception(self, request, exc_type, exc_val, exc_tb):
if self._condition.should_retry_after_exception(
exc_type, exc_val, exc_tb
):
return self._next_delay()
else:
self._reset()

For both method, the if case is where you want to invoke the new retry handler.

Possible Workaround

Here's a workaround that you can use right now: define your own uplink.retry.when.RetryPredicate.

For example, here's a very simple implementation for a retry counter:

from uplink.retry.when import RetryPredicate

class RetryPredicateWithCounter(RetryPredicate):
    def __init__(self, predicate):
        self._predicate = predicate  # This should be a `retry.when.*` predicate
        self._retry_count = 0

    def should_retry_after_response(self, response):
        should_retry = self._predicate.should_retry_after_response(response)
        self_retry_count = 0 if not should_retry else self_retry_count + 1
        return result

    def should_retry_after_exception(self, exc_type, exc_val, exc_tb):
        should_retry = self._predicate.should_retry_after_exception(exc_type, exc_val, exc_tb)
        self_retry_count = 0 if not should_retry else self_retry_count + 1
        return result

Then, you would pass a RetryPredicateWithCounter instance into the when argument for the uplink.retry decorator:

@retry(when=RetryPredicateWithCounter(retry.when.raises(MyException)))
@get('users')
def get_users(self):
    pass

@liiight
Copy link

liiight commented Jun 21, 2019

Thanks for the workaround the pointers!

I'd like to run another design option other than those you suggested, more consistent with response handler (which i really like)

@uplink.retry_handler
def a_retry_handler(retry_exception, response):
	if response.status_code == 404:
		do_something()
	return response # maybe raise the retry exception?

@a_retry_handler
@retry(...)
@get('users')
def get_users(self):
    pass

WDYT?

@prkumar
Copy link
Owner Author

prkumar commented Jun 27, 2019

@liiight - The @retry_handler design looks good to me!

Have you had a chance to try out the workaround I mentioned in my previous comment? It should work with the latest version of the library.

@cognifloyd
Copy link
Contributor

cognifloyd commented Jul 9, 2019

@retry_handler looks great to me.
I tried creating a @response_handler to raise an exception to be caught by an @retry, but that failed because @retry runs before @response_handler. It took a few weeks for me to figure out that my retry was actually having no effect.
So, making it easier to debug @retry, or making a simple interface for making your own retry logic (like @retry_handler) sounds great.

I ended up writing my own RetryPredicate:

class FutureIsNotCompleted(retry.when.RetryPredicate):
    # noinspection PyMethodMayBeStatic
    def should_retry_after_response(self, response):
        try:
            res_json = response.json()
        except ValueError:
            res_json = [{}]
        # status: Completed, Inprogress
        return not all(outer.get("status") == "Completed" for outer in res_json)
class FuturesMixin:  # meant to be inherited by a class that also inherits from uplink.Consumer
    @retry(when=FutureIsNotCompleted(), stop=retry.stop.after_delay(240))
    @returns_results_or_future
    @get(args=(Url,))
    def _check_async_results(self, location):
        pass

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

No branches or pull requests

3 participants