Randomize jitter #38545
Merged
Randomize jitter #38545
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aditya-vector
commented
Feb 21, 2020
aditya-vector
commented
Feb 21, 2020
aditya-vector
commented
Feb 21, 2020
aditya-vector
commented
Feb 21, 2020
|
I see the build failing but I can't zero down to the source where my set of changes are causing exceptions. Any help would be appreciated! |
|
The extra log entries for base changes and |
aditya-vector
force-pushed
the
activejob-randomize-jitter
branch
2 times, most recently
from
0a5ca02
to
c7c7f6f
Compare
|
It'd be great if someone could take a look at this one! Sorry if I'm being too pushy! :) |
rafaelfranca
requested changes
Mar 4, 2020
jeremy
reviewed
Mar 5, 2020
aditya-vector
force-pushed
the
activejob-randomize-jitter
branch
from
c7c7f6f
to
07e31aa
Compare
author Aditya Narsapurkar <adityanarsapurkar@yahoo.com> 1582316102 +0530 committer Aditya Narsapurkar <adityanarsapurkar@yahoo.com> 1583159505 +0530 parent 6d0895a author Aditya Narsapurkar <adityanarsapurkar@yahoo.com> 1582316102 +0530 committer Aditya Narsapurkar <adityanarsapurkar@yahoo.com> 1583159327 +0530 Randomize jitter - This PR attempts to fix a problem with ActiveJob jitter where the `determine_jitter_for_delay` value may not always be randomized. Especially when the jitter delay multplier is between 1 and 2 it always returns 0. - With this change, we pass a range to `Kernel.rand` beginning with 0 to the `jitter_multiplier`. With positive float values, the return value will be a random float number from the range. - Includes test cases to verify random wait time when the jitter_multiplier is between 1 and 2. - Updated relevant test cases stubbing the `Kernel.rand` method, refactored some by removing unwanted stubs for `Kernel.rand` method where jitter is falsey. Fixed rubocop issue - used assert_not_equal instead of refute_equal in test case Fixed rubocop issue - used assert_not_equal instead of refute_equal in test case Fixed rubocop issue - used assert_not_equal instead of refute_equal in test case Review updates - separated test cases for random wait time with default and exponentially retrying jobs - Another test case added to make sure negative wait time does not affect the randomization Review updates - Instead of using Kernel.rand with range, used simple multiplication with Kernel.rand for calculating delay for jitter - Updates to the tests according to changes
aditya-vector
force-pushed
the
activejob-randomize-jitter
branch
from
07e31aa
to
77435e2
Compare
aditya-vector
commented
Mar 5, 2020
aditya-vector
commented
Mar 5, 2020
aditya-vector
commented
Mar 5, 2020
aditya-vector
commented
Mar 5, 2020
|
@rafaelfranca @jeremy Thanks again for your reviews! I've made the suggested changes. Please take a look! 🤞 |
jeremy
approved these changes
Mar 5, 2020
There was a problem hiding this comment.
Great work @aditya-vector. Grateful for your contribution. Thank you!
|
Thanks, @jeremy! For being patient and supportive. Happy to have my first commit merged! 😃 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
The
jitteroption toActiveJob#retry_onwas added in this PR #31872 and relevant updates made in #38003 and #37923. Thanks to these changes, the thundering herd issue is taken care of.Currently, an issue with the
jitteroption is that even when non zero jitter values are provided, the resultant wait time may not be random. This is a case for many combinations of wait types and values (some of them with default argument value combinations).For some set of arguments provided, the jitter values fetched from
Kernel.randare not randomized. For instance, in thedetermine_jitter_for_delay(delay, jitter)method, whendelay * jitterresults in a value between 1 and 2,Kernel.randwill always be0. It may provide false positive to the user that a random jitter will be added to thewaittime. And a situation may occur where there may be a possibility of thundering herd problem.Some examples for reference,
With wait time,
->
retry_on(*exceptions, jitter: 0.6)will always result indelay_jitteras 0 andwaitas 3 seconds for the first attempt after failure.With exponential wait time,
->
retry_on(*exceptions, wait: :exponentially_longer, jitter: 1.2)will always result indelay_jitteras 0 andwaitas 3 seconds for the first attempt after failure.Also, with a positive numeric argument,
Kernel.randreturns an integer which reduces the possible set of returned values and thus, resulting in a higher probability that a value will be repeated. Eg:Kernel.rand(2.4)will only have 2 possible values, 0 and 1.Proposed solution:
This PR attempts to pass a range to
Kernel.randbeginning with 0 to thejitter_multiplier. With positive float values, the return value will be a random float number from the range.Kernel.randmethod, refactored some by removing unwanted stubs forKernel.randmethod where jitter is falsey.PS: This happens to be my first PR on Rails repository. Apologies if I've missed something obvious.