Skip to content

Add warnings for invalid setter values in ExponentialBackoffPolicy #492

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

Merged

Conversation

kssumin
Copy link
Contributor

@kssumin kssumin commented May 11, 2025

This PR addresses #352 by adding warning logs when invalid values are provided to ExponentialBackoffPolicy setters.

Changes

  • Added warning logs in setInitialInterval() when value < 1
  • Added warning logs in setMultiplier() when value <= 1.0
  • Added warning logs in setMaxInterval() when value <= 0

Motivation

The current implementation silently adjusts invalid values without informing users. This can lead to confusion when configured values differ from actual behavior. Adding warning logs helps users understand when their configuration needs adjustment.

Backward Compatibility

This change maintains backward compatibility by:

  • Keeping the existing behavior of silently adjusting invalid values
  • Only adding warning logs to inform users
  • No breaking changes to the API

Related Issue

Closes #352

@artembilan
Copy link
Member

Hi @kssumin !

We have release this Friday.
Would be great if this can make it.
If I don't hear from you by then, I'll do some clean up myself on your PR.
Merge it and then follow up commit with what I think is expected from the code.
Thanks

Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error:  Failed to execute goal io.spring.javaformat:spring-javaformat-maven-plugin:0.0.43:validate (default) on project spring-retry: Formatting violations found in the following files:
Error:   * /home/runner/work/spring-retry/spring-retry/src/main/java/org/springframework/retry/backoff/ExponentialBackOffPolicy.java
Error:   * /home/runner/work/spring-retry/spring-retry/src/test/java/org/springframework/retry/backoff/ExponentialBackOffPolicyTests.java
Error:  
Error:  Run `spring-javaformat:apply` to fix.

@kssumin kssumin force-pushed the feature/352-add-validation-warnings branch from 0e6f529 to 3d7eafe Compare May 15, 2025 11:05
@kssumin
Copy link
Contributor Author

kssumin commented May 15, 2025

@artembilan Thank you for the detailed review! I've addressed all your feedback:

  1. Removed the setLogger() method as it shouldn't be exposed to end-users
  2. Updated all test methods to use DirectFieldAccessor for reflection-based logger access
  3. Simplified all warning messages to avoid redundancy:
    • Initial interval: "Initial interval must be at least 1, but was X"
    • Multiplier: "Multiplier must be > 1.0 for effective exponential backoff, but was X"
    • Max interval: "Max interval must be positive, but was X"
  4. Applied spring-javaformat to fix formatting violations

All changes have been implemented in the latest commit. Please let me know if there's anything else that needs to be adjusted.

@kssumin kssumin force-pushed the feature/352-add-validation-warnings branch from 3d7eafe to 0ab5470 Compare May 15, 2025 11:17
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!
Please, add your name to the @author list of the effected classes.

Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error:   * /home/runner/work/spring-retry/spring-retry/src/test/java/org/springframework/retry/backoff/ExponentialBackOffPolicyTests.java
Error:  
Error:  Run `spring-javaformat:apply` to fix.

Please, ensure to run mvnw verify before pushing to the PR.
Thanks

@kssumin kssumin force-pushed the feature/352-add-validation-warnings branch from 0ab5470 to 38c071d Compare May 15, 2025 17:18
@kssumin
Copy link
Contributor Author

kssumin commented May 15, 2025

스크린샷 2025-05-16 오전 2 19 30

@artembilan
Thank you for the review! I've run spring-javaformat:apply and completed the verification. The build was successful as shown in the screenshot. This is my first time contributing to open source, so I wasn't familiar with these formatting rules. I appreciate your guidance! I'll make sure to run 'mvnw verify' before pushing to PRs in the future. 😊

Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last remark.
I see this in the commit:

Signed-off-by: kssumin <[email protected]>

That has to be your real legal name.
We cannot accept contributions with nick names.
Also, see if that is real email.

The DCO requirements must be met.

You have to fix your Git client to use your proper legal name.
Then you would need to force-push into this PR with new Signed-off-by.

Thank you for understanding!

- Add warning logs when setter values don't meet expected constraints
- Maintain backward compatibility by not changing behavior

Fixes spring-projects#352

Signed-off-by: Kim Sumin <[email protected]>
@kssumin kssumin force-pushed the feature/352-add-validation-warnings branch from 38c071d to 0149a59 Compare May 16, 2025 00:14
@kssumin
Copy link
Contributor Author

kssumin commented May 16, 2025

The email was indeed valid, but I've now changed it to a more commonly used email address.
Thank you for explaining these requirements in detail. I appreciate your guidance and have learned a lot from this process.

Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Will merge tomorrow , my morning.
Thank you again!

@artembilan artembilan merged commit a05e3a1 into spring-projects:main May 16, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent validation of exponential backoff
2 participants