Skip to content

Conversation

@yxtay
Copy link
Contributor

@yxtay yxtay commented Dec 18, 2023

Summary

Improves wait_exponential_jitter to inherit wait_exponential
- Reuses wait_exponential.__call__ method instead of of duplicating code
- Follows argument names of wait_exponential
- Adds min argument
- Supports supplying max, jitter, min arguments as timedelta

Tests

pytest tests/test_tenacity.py::TestWaitConditions::test_wait_exponential_jitter

jd
jd previously approved these changes Dec 18, 2023
@mergify mergify bot dismissed jd’s stale review December 18, 2023 08:40

Pull request has been modified.

@yxtay yxtay requested a review from jd December 18, 2023 08:44
@yxtay
Copy link
Contributor Author

yxtay commented Nov 14, 2024

Is there anything blocking this from being merged? Deprecation concerns? It's been almost 1 year.

@LotfiRafik
Copy link

LotfiRafik commented Feb 20, 2025

Hi @yxtay ,
in the case min > initial * exp , the result will be wrong (not equivalent to the original formula).
example :
min == 100; max == 10^9; initial * exp == 8; jitter == 10

  • Original formula (+adding the min value)

max(max(0, min), min(result, self.max)) == max(max(0, min), min(initial * exp + jitter, max)) == max(max(0, 100), min(18, 10^9)) == max(100, 18) == 100

  • Formula in this PR
 max(max(0, 100), min(max(max(0, min), min(initial * exp, self.max)) + jitter, max) == 
 max(max(0, 100), min(max(max(0, 100), min(8, 10^9)) + 10, 10^9) == 
 max(100, min(100 + 10, 10^9) == max(100, 110) == 110

The correct thing to do is to leave the implementation of wait_exponential_jitter separate from the wait_exponential (because its adding jitter before comparing with max, min values..).

result = self.max
return max(0, min(result, self.max))
result = super().__call__(retry_state) + random.uniform(0, self.jitter)
return max(self.min, min(result, self.max))

Choose a reason for hiding this comment

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

the min part should be equal to max(0, self.min) (in case self.min is negative + follow same formula as wait_exponential)

@krispenney
Copy link

👋 I was just looking at implementing the min and the typing improvements that you added here @yxtay, I feel like they'd still be nice api improvements.

Would it make sense to split those commits off to be merged while @LotfiRafik 's comments are addressed?

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.

4 participants