Skip to content
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

Inconsistent validation of exponential backoff #352

Open
aahlenst opened this issue May 26, 2023 · 3 comments
Open

Inconsistent validation of exponential backoff #352

aahlenst opened this issue May 26, 2023 · 3 comments
Labels
Milestone

Comments

@aahlenst
Copy link
Contributor

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.

@artembilan
Copy link
Member

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 artembilan added the bug label May 26, 2023
@artembilan artembilan added this to the 2.0.2 milestone May 26, 2023
@aahlenst
Copy link
Contributor Author

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.

@artembilan
Copy link
Member

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 artembilan modified the milestones: 2.0.2, Backlog Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants