Change compute_backoff implementation to respect min_backoff

[LincolnPuzey]

Closes #651

Currently, because the 'jittering' makes the backoff smaller, for the first attempt, the returned value will be smaller than factor - which is min_backoff. This is the bug reported.

My change here is to make the 'jittering' increase the backoff, so it is never smaller than factor (min_backoff).

This implementation does mean there will be no jittering once max_backoff is reached.
Is that a problem? @Bogdanp Can I get your opinion?

[Bogdanp]

This implementation does mean there will be no jittering once max_backoff is reached. Is that a problem? @Bogdanp Can I get your opinion?

I suspect it doesn't matter in practice, but it's hard to say. This change will cause most tasks to be delayed more, so that's something to consider. An in-between change could be to just change the return to return attempts + 1, max(factor, backoff) in the original code (and keep the new docstring comment). That would preserve the jitter at the top end, and keep factor as the min backoff.

[LincolnPuzey]

@Bogdanp

I suspect it doesn't matter in practice, but it's hard to say.

I would say that assuming the first several backoffs are jittered, once max_backoff is reached it doesn't matter, since tasks that started their first attempt at the same time, are no longer in sync.

This change will cause most tasks to be delayed more

I reckon, probably doesn't matter in practice.
Or we could make this change and halve the DEFAULT_MIN_BACKOFF, so anyone using all the defaults, keeps the same behavior.

An in-between change could be to just change the return to return attempts + 1, max(factor, backoff)

That would mean the first backoff is exactly factor with no jitter

[Bogdanp]

Or we could make this change and halve the DEFAULT_MIN_BACKOFF, so anyone using all the defaults, keeps the same behavior.

This would still end up affecting people who've explicitly set a min backoff.

That would mean the first backoff is exactly factor with no jitter

Yes, and IIRC that's why I went with the current approach, but I don't think it would be that bad to not have jitter on the first attempt.

All that said, I am fine with any of these approaches. Ideally, we would come up with a couple workload examples (as in, code in the examples/ dir) to see if the change is going to have any impact (eg. to throughput and to worker memory consumption), but I understand that would be quite a bit of work. Also, consider that the Redis broker and the results backend use compute_backoff, too, not just Retries, so we should think about how those would be affected as well.

[LincolnPuzey]

I have pushed an update to keep the jitter happening at max_backoff, so there is jitter at both ends.

This would still end up affecting people

Ultimately, fixing this bug means there will be some effect on people. But we aren't changing any API, and I can explain in the changelog that tasks retries will be delayed longer on average.

[LincolnPuzey]

To gauge the effect, these are some example number:

To simulate task reties:

for i in range(20):
    print(dramatiq.common.compute_backoff(i, factor=15000, max_backoff=(86400000 * 7)))

results:

Old               New
(1,       7991)   (1,      29091)
(2,      28551)   (2,      54575)
(3,      54695)   (3,      91485)
(4,     110217)   (4,     213304)
(5,     192363)   (5,     417787)
(6,     355626)   (6,     724451)
(7,     801379)   (7,    1409588)
(8,    1721092)   (8,    3666171)
(9,    3771939)   (9,    4821197)
(10,   7478468)   (10,  12933777)
(11,  15234240)   (11,  15529418)
(12,  23771112)   (12,  56590998)
(13,  43314201)   (13,  76367250)
(14,  88593935)   (14, 224890672)
(15, 178710296)   (15, 477088020)
(16, 380335431)   (16, 511192657)
(17, 572914695)   (17, 527386475)
(18, 519998459)   (18, 497887595)
(19, 576937344)   (19, 564550618)
(20, 548646440)   (20, 326744272)

New ones are about 2x, as expected. Both hit the max_backoff for the last ~5.

[LincolnPuzey]

Also, consider that the Redis broker and the results backend use compute_backoff

I think there it will be a similar thing, the backoffs will be 2x on average, although the max_backoff it a lot lower, and is reached sooner.

@Bogdanp I could halve the factor in those places so that there should be no change in behavior?

[Bogdanp]

@Bogdanp I could halve the factor in those places so that there should be no change in behavior?

Let's keep them as is for now. The factors in those other cases are small enough that this probably won't matter.