-
Notifications
You must be signed in to change notification settings - Fork 16.4k
handle overflow in TaskInstance next_retry_datetime fixes 47971 #48557
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
handle overflow in TaskInstance next_retry_datetime fixes 47971 #48557
Conversation
|
If you turn off whitespace changes, only 6 lines are added in the new version and none are removed. :) |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
|
Is it possible to localise the try-except block a bit more? Currently the block is a bit too large IMO. |
|
The overflow error is likely two places: on line 1120: And on line 1145: https://docs.python.org/3/library/datetime.html#timedelta-objects Keeping the try block as it currently is avoids having two try blocks, but possibly two try blocks would be more readable. I'll fiddle with it and try to improve. Thanks for the feedback. |
|
Looking a little closer, we cap MAX_RETRY_DELAY at 24 * 60 * 60 so probably it can't throw at line 1145.... I'll need to be sure that isn't configurable at run time, but as long as it is always less than 999999999 days then I can shorten up the section in the try block. I'll try to get to it tonight. |
Done. Thanks for the feedback, it is much cleaner now. |
|
FWIW, I think we should drop the log when overflow is caught since we don't log other reasons for capping the delay. |
|
@uranusjr could you take a look at this PR again when you have some time? |
…8557) This handles overflow when calculating the next execution time for a task instance by falling back to the configured maximum delay. The solution uses the same strategy that tenacity uses: https://github.com/jd/tenacity/blob/main/tenacity/wait.py#L167 An alternate solution would be the determine the maximum tries that wouldn't exceed the maximum delay and then not calculate the timeout for values larger than that. Something like ``` max_delay = self.task.max_retry_delay if self.task.max_retry_delay is not null else MAX_RETRY_DELAY tries_before_max_delay = math.floor(math.log2(max_delay)) if self.try_number <= tries_before_max_delay: # existing logic else: delay = max_delay ``` (cherry picked from commit 33658f0) Co-authored-by: perry2of5 <perry2of5@yahoo.com> closes: #47971
…ache#48557) This handles overflow when calculating the next execution time for a task instance by falling back to the configured maximum delay. The solution uses the same strategy that tenacity uses: https://github.com/jd/tenacity/blob/main/tenacity/wait.py#L167 An alternate solution would be the determine the maximum tries that wouldn't exceed the maximum delay and then not calculate the timeout for values larger than that. Something like ``` max_delay = self.task.max_retry_delay if self.task.max_retry_delay is not null else MAX_RETRY_DELAY tries_before_max_delay = math.floor(math.log2(max_delay)) if self.try_number <= tries_before_max_delay: # existing logic else: delay = max_delay ``` (cherry picked from commit 33658f0) Co-authored-by: perry2of5 <perry2of5@yahoo.com> closes: apache#47971
…8557) (#54460) This handles overflow when calculating the next execution time for a task instance by falling back to the configured maximum delay. The solution uses the same strategy that tenacity uses: https://github.com/jd/tenacity/blob/main/tenacity/wait.py#L167 An alternate solution would be the determine the maximum tries that wouldn't exceed the maximum delay and then not calculate the timeout for values larger than that. Something like ``` max_delay = self.task.max_retry_delay if self.task.max_retry_delay is not null else MAX_RETRY_DELAY tries_before_max_delay = math.floor(math.log2(max_delay)) if self.try_number <= tries_before_max_delay: # existing logic else: delay = max_delay ``` (cherry picked from commit 33658f0) closes: #47971 Co-authored-by: perry2of5 <perry2of5@yahoo.com>
This handles overflow when calculating the next execution time for a task instance by falling back to the configured maximum delay. The solution uses the same strategy that tenacity uses:
https://github.com/jd/tenacity/blob/main/tenacity/wait.py#L167
An alternate solution would be the determine the maximum tries that wouldn't exceed the maximum delay and then not calculate the timeout for values larger than that.
Something like
closes: #47971