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

Add builder for Backoff #5488

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

kth496
Copy link
Contributor

@kth496 kth496 commented Mar 6, 2024

Motivation:

Modifications:

  • Add Builder class for various backoff implementations.

Result:

return new Builder();
}

public static class Builder {
Copy link
Contributor

Choose a reason for hiding this comment

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

Code style) We prefer top-level builder classes instead of nested classes.
Could you move this class to ExpontionalBackoffBuilder?

Comment on lines 99 to 101
public static Builder builder() {
return new Builder();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding a builder method to each implementation, should we add a builder method Backoff interface?

interface Backoff {
   static ExpontionalBackoffBuilder builderForExpontional() {
      ...
   }
}

Note that ExponentialBackoff is a package-private class that users can't access.

return new ExponentialBackoff(initialDelayMillis, maxDelayMillis, multiplier);
}

public Builder initialDelayMillis(long initialDelayMillis) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make jitter values configured with this builder?

private long maxDelayMillis;

FibonacciBackoff build() {
return new FibonacciBackoff(initialDelayMillis, maxDelayMillis);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we migrate all factory methods where constructors are used into builder methods? e.g.

return new ExponentialBackoff(initialDelayMillis, maxDelayMillis, multiplier);

@kth496
Copy link
Contributor Author

kth496 commented Mar 26, 2024

@ikhoon Thank you for your review. I will fix it within a week.

@kth496 kth496 marked this pull request as draft March 27, 2024 15:10
@github-actions github-actions bot added the Stale label Apr 27, 2024
@trustin
Copy link
Member

trustin commented May 8, 2024

@kth496 Looking forward to seeing more commits in this PR 💟

@github-actions github-actions bot removed the Stale label May 9, 2024
@trustin
Copy link
Member

trustin commented Jun 6, 2024

@kth496 Any updates?

@github-actions github-actions bot added the Stale label Jul 8, 2024
@github-actions github-actions bot removed the Stale label Aug 9, 2024
@github-actions github-actions bot added the Stale label Sep 10, 2024
@kth496
Copy link
Contributor Author

kth496 commented Oct 3, 2024

@ikhoon @trustin
Thanks for patience. I have improved the code based on your feedback:

  • Moved the nested classes into top-level classes
  • Moved the builder methods to the BackOff interface

Question:
Is there a good way to make the jitter value configurable in the builder? Since jitter can be commonly set across multiple BackOff implementations, I'm considering whether to define a separate higher-level builder.

@kth496 kth496 marked this pull request as ready for review October 3, 2024 07:28
Copy link

codecov bot commented Oct 3, 2024

Codecov Report

Attention: Patch coverage is 83.87097% with 5 lines in your changes missing coverage. Please review.

Project coverage is 74.68%. Comparing base (9a29b39) to head (7575359).
Report is 35 commits behind head on main.

Files with missing lines Patch % Lines
...corp/armeria/client/retry/FixedBackoffBuilder.java 0.00% 4 Missing ⚠️
...ava/com/linecorp/armeria/client/retry/Backoff.java 75.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5488      +/-   ##
============================================
+ Coverage     74.66%   74.68%   +0.01%     
- Complexity    21681    21725      +44     
============================================
  Files          1896     1901       +5     
  Lines         80320    80414      +94     
  Branches      10548    10553       +5     
============================================
+ Hits          59974    60059      +85     
- Misses        15318    15324       +6     
- Partials       5028     5031       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add builder for Backoff
3 participants