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

[FeatureRequest] Make number of retries configurable for /nodes and potentially other relevant http calls #1756

Closed
dwco-z opened this issue Apr 22, 2024 · 7 comments · Fixed by #1758

Comments

@dwco-z
Copy link

dwco-z commented Apr 22, 2024

Is your feature request related to a problem? Please describe.
Currently rqlite uses defaultMaxRetries(8) during GetNodeAPIAddr on the cluster client. Considering that unreachable nodes like shutdown rqlite instances will keep failing during the first + 8 retry attempts, having the option to configure this number of retries and possibly decrease it might be useful to avoid having too many retries being ineffectively executed.

Describe the solution you'd like
A possible solution could be making the defaultMaxRetries configurable

@otoolep
Copy link
Member

otoolep commented Apr 22, 2024

I may just start by setting the default retry value to 0. This is the second issue that's been filed about the retry behavior, so I think it's time to start removing default retries. It's always been an implementation detail, so I wouldn't consider it a breaking change.

@otoolep
Copy link
Member

otoolep commented Apr 22, 2024

I do plan to allow is to be set on a request-by-request basis, but may just change the default to 0 to see if tests still pass.

@dwco-z
Copy link
Author

dwco-z commented Apr 22, 2024

I do plan to allow is to be set on a request-by-request basis, but may just change the default to 0 to see if tests still pass.

I may just start by setting the default retry value to 0. This is the second issue that's been filed about the retry behavior, so I think it's time to start removing default retries. It's always been an implementation detail, so I wouldn't consider it a breaking change.

Right, take makes sense and it would help with my issue

@otoolep otoolep linked a pull request Apr 23, 2024 that will close this issue
@otoolep
Copy link
Member

otoolep commented Apr 23, 2024

@dwco-z -- this is now fixed on master. You will get zero retries without doing anything, but can add retries=N to your request (as a query parameter) where N is an integer.

@otoolep
Copy link
Member

otoolep commented Apr 23, 2024

I can make a release, but perhaps you just want to build master to check it out.

@dwco-z
Copy link
Author

dwco-z commented Apr 23, 2024

Thanks @otoolep. Sure, I'll build master to test it

@dwco-z
Copy link
Author

dwco-z commented Apr 23, 2024

Everything looks good, now I don't have /nodes retries happening on the rqlite's side per default

otoolep added a commit that referenced this issue Apr 26, 2024
@otoolep otoolep linked a pull request Apr 26, 2024 that will close this issue
@otoolep otoolep removed a link to a pull request Apr 26, 2024
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 a pull request may close this issue.

2 participants