Retry on specific exception type 84 #135

ewjoachim · 2020-01-31T13:51:00Z

Fixes #84

Pair programmed with @emmanuelleq

Successful PR Checklist:

Tests
Documentation (optionally: run spell checking)
Had a good time contributing? (if not, feel free to give some feedback)

codecov-io · 2020-01-31T13:53:55Z

Codecov Report

Merging #135 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #135   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          20     20           
  Lines         791    796    +5     
  Branches       78     79    +1     
=====================================
+ Hits          791    796    +5

Impacted Files	Coverage Δ
procrastinate/retry.py	`100% <100%> (ø)`	⬆️
procrastinate/worker.py	`100% <100%> (ø)`	⬆️
procrastinate/tasks.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b18f978...3256d4c. Read the comment docs.

pmourlanne

Couple of comments here and there (you know me 🙃 ) but code looks really good 👍

docs/howto/retry.rst

procrastinate/retry.py

ewjoachim · 2020-01-31T15:37:01Z

@pmourlanne both problems fixed

ewjoachim · 2020-01-31T17:11:13Z

We could merge but @elemoine said he wanted to make a review, so let's wait until Monday/Tuesday.

tests/acceptance/test_nominal.py

CorBott

To improve consistency, I'd stick to one way of expressing retry:
if you don't, jobs will be retried indefinitely until they pass
if you don't, jobs will be retried immediately
if you don't, jobs will be retried on any type of exceptions

elemoine · 2020-02-03T12:33:35Z

procrastinate/retry.py

@@ -17,15 +17,19 @@ class BaseRetryStrategy:
    Children classes only need to implement `get_schedule_in`.


nit: s/Children/Child (while you're at it)

Isn't children the proper form as classes is plural? 🤔

Ah maybe not, as child is an adjective here :) .

elemoine · 2020-02-03T12:41:12Z

docs/howto/retry.rst

+    @app.task(retry=procrastinate.RetryStrategy(
+        max_attempts=10,
+        wait=5,
+        retry_exceptions=[ConnectionError, IOError]


I'd use a tuple here instead of a list. To advertise better practices to our users.

In what way is this a better practice to use a tuple here ? This is an array of homogeneous elements with arbitrary length, which fits better as a list IMHO, contrary to a tuple which works best as a fixed-length iterable where each element's type and signification is described through their position, no ?

To be more precise, my rule of thumb:

If you have very specific performance constraints, you may explore the collections zoo (deque, ...)

If it makes no sense to have duplicates and order doesn't matter, it's a set

Else if any 2 elements could be swapped and it would still make sense, it's a list

Else if it can be used as basis for a namedtuple, it's a tuple (and you should consider using a namedtuple)

Else... I'm interested to know what iterable you've found that fail all of the above tests.

I did not go that far :-)

I just think a fixed-length, immutable, data structure makes more sense in such cases. In the same way isinstance receives a tuple as the second argument. Or when you write elem in ("foo", "bar").

But I don't mean to be too pedantic. Using a list is fine too.

Ha sorry for being pedantic too :D

I think the fact ìsinstance() takes a tuple and not an iterable is an error from Python's youth.

Also, I think you were totally right questioning my choice of using a list: from my own rationale just above, a set would actually be much more appropriate (order doesn't matter and duplicates make no sense) 😆

And finally, regarding elem in ("foo", "bar"), I was curious if the most "appropriate" structure (here, a set) was also the most performant and it kinda looks like it is (with huge question marks regarding the validity of my benchmark):

In [1]: %timeit "baz" in {"foo", "bar"} 29.6 ns ± 0.79 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each) In [2]: %timeit "baz" in ("foo", "bar") 57 ns ± 0.656 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each) In [3]: %timeit "baz" in ["foo", "bar"] 58 ns ± 1.9 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

(this holds true if we're looking for the 2nd element but the 3 are as long if we're looking for the 1st element)
(Just to be clear, I'm not advocating for using one or another way just to earn a few nanoseconds as long as it's not on the critical path)

elemoine

Two minor comments, otherwise looks very good to me!

Co-Authored-By: Emmanuelle Quilliet <[email protected]>

elemoine · 2020-02-10T10:01:36Z

@ewjoachim feel free to merge this whenever you think this is good to go.

ewjoachim · 2020-02-10T12:24:22Z

Yes, just waiting for @CorBott to approve :)

CorBott

Fine, thank you!

ewjoachim requested review from CorBott, mgu, pmourlanne and sophie-ulti as code owners January 31, 2020 13:51

ewjoachim requested a review from emmanuelleq January 31, 2020 13:51

pmourlanne approved these changes Jan 31, 2020

View reviewed changes

docs/howto/retry.rst Outdated Show resolved Hide resolved

docs/howto/retry.rst Outdated Show resolved Hide resolved

procrastinate/retry.py Outdated Show resolved Hide resolved

ewjoachim force-pushed the retry-exception-84 branch from c7c004c to bc16c81 Compare January 31, 2020 14:51

k4nar reviewed Jan 31, 2020

View reviewed changes

tests/acceptance/test_nominal.py Show resolved Hide resolved

CorBott suggested changes Feb 3, 2020

View reviewed changes

elemoine reviewed Feb 3, 2020

View reviewed changes

ewjoachim changed the title ~~Retry exception 84~~ Retry on specific exception type 84 Feb 8, 2020

ewjoachim requested a review from DainDwarf as a code owner February 8, 2020 12:52

Joachim Jablon and others added 9 commits February 8, 2020 13:52

Fixes #84: Add a retry_exceptions parameter to RetryStrategy

12ada29

Co-Authored-By: Emmanuelle Quilliet <[email protected]>

Add and fix tests

979ce56

Co-Authored-By: Emmanuelle Quilliet <[email protected]>

Adjust documentation

16ac742

Co-Authored-By: Emmanuelle Quilliet <[email protected]>

Add precision on retry documentation

9b78f49

Make the RetryStrategy API more future proof

8b54b62

Improve display of expections & logging in case of failure

8a921c9

Test stderr too

b7d02a4

Doc wording

cf3f449

isinstance wants a tuple, but it's not the best iterable

3256d4c

ewjoachim force-pushed the retry-exception-84 branch from 768ff81 to 3256d4c Compare February 8, 2020 12:53

elemoine approved these changes Feb 10, 2020

View reviewed changes

k4nar approved these changes Feb 10, 2020

View reviewed changes

CorBott approved these changes Feb 10, 2020

View reviewed changes

ewjoachim merged commit 61396c7 into master Feb 10, 2020

ewjoachim deleted the retry-exception-84 branch February 10, 2020 12:36

ewjoachim mentioned this pull request Feb 14, 2020

Release 0.4.0 #154

Merged

ewjoachim mentioned this pull request Jun 24, 2020

RetryStrategy.retry_exceptions not documented #269

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retry on specific exception type 84 #135

Retry on specific exception type 84 #135

ewjoachim commented Jan 31, 2020 •

edited

Loading

codecov-io commented Jan 31, 2020 •

edited

Loading

pmourlanne left a comment

ewjoachim commented Jan 31, 2020

ewjoachim commented Jan 31, 2020

CorBott left a comment

elemoine Feb 3, 2020

k4nar Feb 10, 2020

k4nar Feb 10, 2020

elemoine Feb 3, 2020

ewjoachim Feb 3, 2020

ewjoachim Feb 3, 2020 •

edited

Loading

elemoine Feb 3, 2020 •

edited

Loading

ewjoachim Feb 3, 2020

elemoine left a comment

elemoine commented Feb 10, 2020

ewjoachim commented Feb 10, 2020

CorBott left a comment

		@@ -17,15 +17,19 @@ class BaseRetryStrategy:
		Children classes only need to implement `get_schedule_in`.

Retry on specific exception type 84 #135

Retry on specific exception type 84 #135

Conversation

ewjoachim commented Jan 31, 2020 • edited Loading

Successful PR Checklist:

codecov-io commented Jan 31, 2020 • edited Loading

Codecov Report

pmourlanne left a comment

Choose a reason for hiding this comment

ewjoachim commented Jan 31, 2020

ewjoachim commented Jan 31, 2020

CorBott left a comment

Choose a reason for hiding this comment

elemoine Feb 3, 2020

Choose a reason for hiding this comment

k4nar Feb 10, 2020

Choose a reason for hiding this comment

k4nar Feb 10, 2020

Choose a reason for hiding this comment

elemoine Feb 3, 2020

Choose a reason for hiding this comment

ewjoachim Feb 3, 2020

Choose a reason for hiding this comment

ewjoachim Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

elemoine Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

ewjoachim Feb 3, 2020

Choose a reason for hiding this comment

elemoine left a comment

Choose a reason for hiding this comment

elemoine commented Feb 10, 2020

ewjoachim commented Feb 10, 2020

CorBott left a comment

Choose a reason for hiding this comment

ewjoachim commented Jan 31, 2020 •

edited

Loading

codecov-io commented Jan 31, 2020 •

edited

Loading

ewjoachim Feb 3, 2020 •

edited

Loading

elemoine Feb 3, 2020 •

edited

Loading