failsafe-lib / failsafe

Fault tolerance and resilience patterns for the JVM
https://failsafe.dev
Apache License 2.0
4.16k stars 295 forks source link

Also add note about default maxRetries of 3 to maxDuration section of the documentation #364

Closed stefanrybacki closed 1 year ago

stefanrybacki commented 1 year ago

The default maxRetries setting of 3 is mentioned in the retries section, however the duration section https://failsafe.dev/retry/#duration reads as if maxRetries is then determined by the max duration.

Tembrel commented 1 year ago

So you want to emphasize that maxDuration doesn't supersede maxRetries/Attempts? Would this addition make it clear?

You can add a max duration for an execution, after which retries will stop (if not stopped by other constraints, like maxRetries/Attempts).

stefanrybacki commented 1 year ago

Yes exactly I ran into this today wondering why only 3 attempts were taking place, only rereading the duration section I was confused. However, after reading the javadoc of withMaxDuration it dawned on me.

Regarding your proposed wording I still find it missleading as I never specified maxRetries so I would not assume other constraints as I already have specified a stop constraint via duration. So maybe also add a note to your proposal, something along the lines:

You can add a max duration for an execution, after which retries will stop (if not stopped by other constraints, like maxRetries/Attempts, Note: maxRetries is set to 3 if not overridden).

Tembrel commented 1 year ago

@jhalterman - Would you want a wording change here, or an earlier emphasis on how maxRetries/Attempts is always considered, or both?

jhalterman commented 1 year ago

Maybe a wording change that does both...

It might be good to move the "Duration" section of the Retry docs to be right below the "Attempts" section, since they both serve the same purpose of constraining executions, as you point out. For proposed wording on "Duration", which would directly follow "Attempts", maybe:

In addition to max attempts, you can also add a max duration for an execution, after which retries will stop if the max attempts haven't already been reached.

Would those two changes clarify things?

stefanrybacki commented 1 year ago

@jhalterman sounds good, I could also image that an example with both maxDuration and maxRetries set to -1 would also make it even clearer

jhalterman commented 1 year ago

I just pushed this change and also added a note about disabling retries: https://github.com/failsafe-lib/failsafe.dev/commit/4ae3ec32d0ad5a0d10e2a195770b22ec00ff0a07

stefanrybacki commented 1 year ago

@jhalterman much better now, thank you for the quick fix