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

Fix: splitting a long doc sentence to 2 #3079

Closed
wants to merge 1 commit into from
Closed

Conversation

lynch19
Copy link

@lynch19 lynch19 commented Jun 17, 2022

No description provided.

@lynch19 lynch19 requested a review from a team as a code owner June 17, 2022 08:33
@pivotal-cla
Copy link

@lynch19 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

1 similar comment
@pivotal-cla
Copy link

@lynch19 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@lynch19 Thank you for signing the Contributor License Agreement!

Copy link
Member

@simonbasle simonbasle left a comment

Choose a reason for hiding this comment

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

I understand the spirit and that sentence is indeed a bit long, but the split is not great. what comes after the : is a single sentence. the idea was to convey that a different index is used for computing retry delays and deciding whether to retry or not. that index is reset to 0 in case of onNext.

@lynch19
Copy link
Author

lynch19 commented Jun 17, 2022

For new users, it's really hard to understand what's going on in this paragraph. Notice that this is the first time in the docs that the word "index" is referenced (raises the question: "What index?").

  • Notice RetrySignal's use isn't clear for new users. There is no code example for its implementation usages nor explanations for its implementations at all.
  • The word "index" doesn't appear in this interface nor the interface's Javadoc. ImmutableRetrySignal#totalRetriesInARow() and RetryWhenMainSubscriber#totalRetriesInARow() do return a this.failureSubsequentIndex, but this seems pretty irrelevant if there's no further explanation for the both of them.

As long as my suggestion will be correct (I'm pretty new to the subject of retrying. Trying to fix the parts I've didn't understand from the docs), I suggest rephrasing the unclear:

Transient error handling in the Retry specs makes use of RetrySignal#totalRetriesInARow(): to check whether to retry or not and to compute the retry delays, the index used is an alternative one that is reset to 0 each time an onNext is emitted.

to:
"Transient error handling in the Retry specs (like RetryWhenMainSubscriber, ImmutableRetrySignal) makes use of RetrySignal#totalRetriesInARow(): to check whether to retry or not and to compute the retry delays. They use an index that is reset to 0 each time an onNext is emitted (this is the number of errors -1 since the latest onNext)."

In general, I think that there's should be a new scope for ==== Transient Errors, and that the RetrySignal section should contain another concrete code example.

And also I have a little question that I think needs to be answered inside the docs - In the initial retrying example (the one with the interval ticks) there was shown how to retry the whole flux. What if I'd like to retry only the specific error-causing item? e.g. I think that many (I'm included) would like to know how to produce the output of:

259,tick 0
249,tick 1
251,tick 2
java.lang.RuntimeException: boom
java.lang.RuntimeException: boom

instead of:

259,tick 0
249,tick 1
251,tick 2
506,tick 0 
248,tick 1
253,tick 2
java.lang.RuntimeException: boom

This can be shown in a more appropriate example (that can demonstrate a temporary network-caused IO exception).

@simonbasle
Copy link
Member

ok, it sounds like the concept of transient error needs more explaining and a more detailed section (eg. TIP) in the reference guide. I'll open a separate PR. This split is too naive and prone to introducing confusion. Keep in mind this mode is opt-in, and not the norm for a RetrySpec.

@simonbasle simonbasle added the status/declined We feel we shouldn't currently apply this change/suggestion label Jun 29, 2022
@simonbasle simonbasle closed this Jun 29, 2022
simonbasle added a commit that referenced this pull request Jul 6, 2022
This commit reworks the reference guide section on Retry with Transient
Errors, improving the explanation and adding more details in a separate
section.

The code snippet has also been changed to reflect a more practical use
case, with the original artificial `Flux<Integer>` split out in a
secondary snippet.

Supersedes #3079.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/declined We feel we shouldn't currently apply this change/suggestion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants