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

retryWith with Retry behavior have to be defaulted to Long.MAX_VALUE #149

Closed
apischan opened this issue Feb 22, 2018 · 3 comments · Fixed by #154
Closed

retryWith with Retry behavior have to be defaulted to Long.MAX_VALUE #149

apischan opened this issue Feb 22, 2018 · 3 comments · Fixed by #154
Labels
good first issue Ideal for a new contributor, we'll help warn/api-change Breaking change with compilation errors
Milestone

Comments

@apischan
Copy link

  1. As I understand default retry(Predicate) function have default amount of retries Long.MAX_VALUE. But if to change default retry(Predicate) to retryWith(Retry) without specifying retryMax(int) - the number of retries will be 1 (which is differs from default retry).
  2. Also there is default retry that's parameter is long but in Retry from addons amount of retries is int that is wrong as for me. The type should be the same.
  3. and in addition to the 1st point. What is the difference between this 2 pieces of code:
    someFlux.retryWith(Retry.any(RuntimeException.class))
    and
    someFlux.retryWith(Retry.any(RuntimeException.class).retryOnce())
    and why this retryOnce() exists at all? And why there is no something like retryInfinite()?

P.S. The most important point is 1st.

@simonbasle simonbasle added the good first issue Ideal for a new contributor, we'll help label Feb 22, 2018
@simonbasle
Copy link
Member

  1. indeed it would make sense to align the default retry of core and the equivalent builder configuration.
  2. Keep in mind this is a builder for the retryWhen operator, which is different from the retry one. At the time the builder was introduced, the easiest way to keep track of retry attempt was zipping with a Flux.range... which is Integer based. There might be a solution to that with the newly introduced index() operator that tags the elements in the sequence with a Long index, so that misalignment is fixable (also that would be a breaking change)
  3. I think it points back to issue one and proves that the original thought process was probably started around the idea of "illimited retries" per default. retryOnce make a ton more sense in that context, as a retryMax(1) alias that reads more nicely in the fluent API style.

@simonbasle simonbasle added this to the 3.2.0.RELEASE milestone Feb 27, 2018
@simonbasle simonbasle added warn/api-change Breaking change with compilation errors and removed type/bug A general bug labels Feb 27, 2018
simonbasle added a commit that referenced this issue Feb 27, 2018
See #150 and #149, pending the behavioral changes the javadoc is
clarified to reflect the fact that factory methods retry once and that
timeout() resets the maxAttempts to "unlimited" (Integer.MAX_VALUE).
@simonbasle
Copy link
Member

Note: this is also true of Repeat.

@simonbasle
Copy link
Member

Some occurrences of Integer.MAX_VALUE still to be fixed, and the previous PR didn't change the default number of retries from 1 to unlimited.

simonbasle added a commit that referenced this issue Apr 11, 2018
+ missing Integer.MAX_VALUE to Long.MAX_VALUE conversions
@simonbasle simonbasle modified the milestones: 3.2.0.RELEASE, 3.2.0.M1 Jun 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Ideal for a new contributor, we'll help warn/api-change Breaking change with compilation errors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants