-
Notifications
You must be signed in to change notification settings - Fork 519
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
Fix backOff API in UniformRandomBackOffPolicy #465
Conversation
When maxBackOffPeriod is less than minBackOffPeriod, delta is taken taken as zero in UniformRandomBackOffPolicy backOff method.
@@ -134,7 +134,7 @@ public long getMaxBackOffPeriod() { | |||
protected void doBackOff() throws BackOffInterruptedException { | |||
try { | |||
Long min = this.minBackOffPeriod.get(); | |||
long delta = this.maxBackOffPeriod.get() == this.minBackOffPeriod.get() ? 0 | |||
long delta = this.maxBackOffPeriod.get() <= this.minBackOffPeriod.get() ? 0 | |||
: this.random.nextInt((int) (this.maxBackOffPeriod.get() - min)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to extract both Supplier.get()
into local variables and use them afterwards.
I think the logical way is possibility to return different values for every get()
call.
So, it is OK to have them different for each backOff()
call, but not within a single one.
Otherwise it may lead to unexpected behavior.
I mean we have that Long min = this.minBackOffPeriod.get();
variable.
So, let's use that in the following code!
Plus let's extract Long max = this.maxBackOffPeriod.get();
I have identified one more issue in Issue: With UniformRandomBackOffPolicy there are no chances of getting Reason: long delta = max <= min ? 0 : this.random.nextInt((int) (max - min)); Random nextInt upper bound value is always less than the provided argument. So, backoffValue computated as Possible Fix (need verify one more time on this): |
According to its JavaDocs:
So, your observation is correct. |
When maxBackOffPeriod is less than minBackOffPeriod, delta is taken taken as zero in UniformRandomBackOffPolicy backOff method.