Skip to content

[Doc] clarify retry loop #130

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
merged 13 commits into from
Mar 2, 2022

Conversation

kaisecheng
Copy link
Contributor

@kaisecheng kaisecheng commented Mar 1, 2022

The retry policy confuses users mainly due to the unclear boundary of retry_failed and automatic_retries.

This PR explains retry_failed set to false does not affect the number of retries in lib level


This output has two levels of retry, library and plugin.

The library only retry IO related failures. Non retriable IOException are InterruptedIOException, UnknownHostException, ConnectException and SSLException.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

From a user perspective maybe is better not to report the Java exceptions names, but the concept they carries.
So it could be something like:

Non retriable errors could regard SSL problems or host not resolved but also connection problem or OS/JVM level interruptions happening during a request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

awesome suggestion!

@kaisecheng kaisecheng changed the title [Doc] clarify retry loop [WIP][Doc] clarify retry loop Mar 1, 2022
@kaisecheng kaisecheng changed the title [WIP][Doc] clarify retry loop [Doc] clarify retry loop Mar 1, 2022
Copy link

@andsel andsel left a comment

Choose a reason for hiding this comment

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

The splitting seems good, I've left a note on how to improve the description of what not retriable errors are, without listing any specific Java exception


This output has two levels of retry, library and plugin.

The library only retry IO related failures. Non retriable IOException are InterruptedIOException, UnknownHostException, ConnectException and SSLException.
Copy link

Choose a reason for hiding this comment

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

From a user perspective maybe is better not to report the Java exceptions names, but the concept they carries.
So it could be something like:

Non retriable errors could regard SSL problems or host not resolved but also connection problem or OS/JVM level interruptions happening during a request.

@kaisecheng kaisecheng requested a review from karenzone March 1, 2022 18:03
Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

Fantastic explanations, @kaise. Thank you for taking this on.

I left some comments about wording and formatting for your consideration. I can't test the documentation until changes are applied, but I'll be happy to retest.

@karenzone
Copy link
Contributor

I left some comments wrt formatting.

Now

Here's how it renders now:
Screen Shot 2022-03-01 at 7 15 04 PM

Proposed

We can make it easier for users to scan and parse with some formatting tweaks (as suggested inline):
Screen Shot 2022-03-01 at 7 11 25 PM

Let me know what you think and if you would like to discuss. Thanks for your work on this.

@karenzone
Copy link
Contributor

Also, what do you think about noting in option descriptions which retry type they apply to?

@kaisecheng
Copy link
Contributor Author

Also, what do you think about noting in option descriptions which retry type they apply to?

If we have some markers to make it clear which options belong to which types, it would be very good. But now I only see verbose messages, so my new commits give a pointer (see Retry Policy...) to all four options. I hope these hints are clear enough.

@@ -343,10 +343,11 @@ Timeout (in seconds) for the entire request
* Value type is <<boolean,boolean>>
* Default value is `true`

Note that this option controls retries for plugin-level requests only.
Note that this option controls plugin-level retries only.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My change was misleading. We don't have plugin-level requests, instead, we have plugin-level retries

@kaisecheng
Copy link
Contributor Author

kaisecheng commented Mar 2, 2022

@karenzone Wow! The new format is so amazing! You raise it to the next level 🚀

Committed all your suggestion.
I copy some important information from "Retry Policy" session to those four options, make a pointer from the options to the "Retry Policy", and update some wording. Please have a look to the last commit. Thank you.

Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

Left some suggestions for wording tweaks for your consideration. Otherwise, LGTM!
Thank you for clarifying how retries work.

This only affects connectivity related errors (see related `automatic_retries` setting).
When set to `false` and `automatic_retries` is enabled, GET, HEAD, PUT, DELETE, OPTIONS, and TRACE requests will be retried.

When set to `true` and `automatic_retries` is enabled, this will cause non-idempotent HTTP verbs (such as POST) to be retried.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When set to `true` and `automatic_retries` is enabled, this will cause non-idempotent HTTP verbs (such as POST) to be retried.
When this option is set to `true` and `automatic_retries` is enabled, non-idempotent HTTP verbs (such as POST) to be retried.

kaisecheng and others added 3 commits March 2, 2022 17:11
Co-authored-by: Karen Metts <[email protected]>
Co-authored-by: Karen Metts <[email protected]>
Co-authored-by: Karen Metts <[email protected]>
@kaisecheng kaisecheng merged commit 6bc70f6 into logstash-plugins:main Mar 2, 2022
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.

4 participants