spring-retry icon indicating copy to clipboard operation
spring-retry copied to clipboard

Published 20 hours ago • verified publisher icon spring-projects

Inconsistent validation of exponential backoff

Open aahlenst opened this issue 1 year ago • 3 comments

setMultiplier() in ExponentialBackoffPolicy allows a minimum multiplier of 1.0. However, the corresponding builder method in RestTemplateBuilder insists on a minimum multiplier greater than 1.0. There's even a test for it.

That's confusing. While it makes sense to require that the multiplier is greater than 1.0, aligning the setter with the builder might break existing usages.

aahlenst avatar May 26 '23 11:05 aahlenst

aligning the setter with the builder might break existing usages.

And since we agree that existing usage is wrong, it has to be fixed. However we cannot make a breaking change like that in the patch version, but what we can is to show a warning when the value in that setter is <= 1.0. This way end-user will know that something is off and respective part of his/her project has to be fixed. And it will be fixed on our side next version - 2.1 or 3.0.

Does this sound like a plan?

artembilan avatar May 26 '23 14:05 artembilan

I looked at the entire ExponentialBackoffPolicy. The pattern used by all setters is not to validate the arguments and to throw an exception if the argument is invalid but to adjust it silently. That pattern is used throughout the package. Deviating from it in a single setter feels wrong. An alternative would be to change RetryTemplateBuilder. However, the builder works differently. It validates the values and does not coerce them into the acceptable range. So maybe leaving it as it is is the more sensible approach.

aahlenst avatar May 28 '23 13:05 aahlenst

I think the pattern to "silently adjust" is wrong. Many would be confused to not see the value they have configured, even if that is there for years already. I believe it is better to say what is wrong and why. This way the library and end-user would be on the same page for values awareness. Therefore I like the RetryTemplateBuilder approach more.

So, if you are OK with warnings in those all setters for the current versions, then feel free to contribute. Otherwise the behavior change to make code flow consistent will be done in the later and probably major version.

artembilan avatar May 30 '23 13:05 artembilan