fiber
fiber copied to clipboard
✨ v3 (feature): add retry mechanism
- General logic is implemented.
- Unit tests are added.
Signed-off-by: Gökhan Özeloğlu [email protected]
Please provide enough information so that others can review your pull request: It is related to #1840.
Explain the details for making this change. What existing problem does the pull request solve?
I added a new package called retry
. Exponential backoff algorithm is used. I can refactor the code like constant/default values, tests, etc. I am going to add more tests and maybe I'll do some small changes to the code.
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord
I am ready for code review, comments, feedback, etc.
Hi @efectn. I have two questions. What do you mean by Should we integrate this into the client directly? Can you explain it? Also, what should I do for hooks? As a note, jitter is already provided with adding randomness.
Hi @efectn. I have two questions. What do you mean by Should we integrate this into the client directly? Can you explain it? Also, what should I do for hooks? As a note, jitter is already provided with adding randomness.
Perhaps we would integrate this into the client like https://github.com/go-resty/resty#retries to use with client easier.
- Btw don't forget running go mod tidy to remove testify from go.mod and go.sum
- Also you can remove app.Use lines from README due to this is not handler middleware.
- Jitter seems ok to me
Perhaps we would integrate this into the client like https://github.com/go-resty/resty#retries to use with client easier.
So, do you mean that we should use it as follow?
app := fiber.New()
retry := app.NewExponentialBackoff())
err := retry.Retry(func() error)
// handle error
(Naming can be changed like retry.Retry
might not be good.)
So, also you mentioned that
Also you can remove app.Use lines from README due to this is not handler middleware.
As I understood that I should remove it from /middleware
directory and carry it to the root directory. If yes, it might be a problem for the config part. If I am wrong, please correct me.
As I understood that I should remove it from
/middleware
directory and carry it to the root directory. If yes, it might be a problem for the config part. If I am wrong, please correct me.
We may locate it in '/utils' as a directory. however i'm not sure.
(Naming can be changed like retry.Retry might not be good.)
i wasn't talking about it. i was talking about adding some built-in retry methods for client that use this retry function. ref: https://github.com/go-resty/resty/blob/master/request.go#L763
We may locate it in '/utils' as a directory. however i'm not sure.
I think, utils
is not a good place to locate them. Because, in general, utils
is a place for utility functions used across the codebase. Also, it has a flat structure. I am not going to change it until you give another suggestion.
i wasn't talking about it. i was talking about adding some built-in retry methods for client that use this retry function. ref: https://github.com/go-resty/resty/blob/master/request.go#L763
I think I understood what you meant. Maybe, I can create a new directory called retry
in the root directory. But, it might not good for the framework.
Hooks would be also great.
Also, what can I do with hooks? I don't have any idea what should I do.
Also, what can I do with hooks? I don't have any idea what should I do.
For example, we can execute hooks in each retry if user defined them.
@ReneWerner87 what do you think about the location?
Yeah its a good idea. I like hooks, its a good solution for customization and give the possibility to extend our functionality.
Yeah its a good idea. I like hooks, its a good solution for customization and give the possibility to extend our functionality.
Btw we're not sure about middleware/
is true location for retry. What do you think?
Yeah its a good idea. I like hooks, its a good solution for customization and give the possibility to extend our functionality.
Btw we're not sure about
middleware/
is true location for retry. What do you think?
Usually retry is a core option for framework, i think we can set retry as option for framework.
Yeah its a good idea. I like hooks, its a good solution for customization and give the possibility to extend our functionality.
Can you provide an example of what should I do with hooks? It can be a code snippet, example code, or something like that. It can make more clear to understand your point.
Yeah its a good idea. I like hooks, its a good solution for customization and give the possibility to extend our functionality.
Can you provide an example of what should I do with hooks? It can be a code snippet, example code, or something like that. It can make more clear to understand your point.
We may add it when to refactor client. Looks like we dont need at the moment.
Hi @ReneWerner87. This is a polite ping. Can you review the PR?
Oh sorry, sure will have a look this week(currently much todo)
Now I need to use retry in client refactoring. I also think middleware/
is not a good location, it might be a good idea to put it in utils/
or client/retry.go
?
Now I need to use retry in client refactoring. I also think
middleware/
is not a good location, it might be a good idea to put it inutils/
orclient/retry.go
?
talked to efectn about it and we want to introduce a new folder
this should be called "plugin", "addon" or "extension"
in utils and helper should be only simple functionalities
please create an order "addon" and under it the order "retry" -> there you place your new functionality also helps go modules, so that only what is needed is checked out
please revise your readme -> the example confused me at first
Now I need to use retry in client refactoring. I also think
middleware/
is not a good location, it might be a good idea to put it inutils/
orclient/retry.go
?talked to efectn about it and we want to introduce a new folder
this should be called "plugin", "addon" or "extension"
in utils and helper should be only simple functionalities
please create an order "addon" and under it the order "retry" -> there you place your new functionality also helps go modules, so that only what is needed is checked out
please revise your readme -> the example confused me at first
Hi @gozeloglu. Can you take a look when you're free? We'll merge this after these changes.
Hi @efectn. Thanks for informing me. I'll take a look and make changes as soon as possible.
Hi again,
Thanks for renaming @efectn. It seems the PR is ready to merge. I have a question to @ReneWerner87 about the latest comment.
please revise your readme -> the example confused me at first
Which part confused you? What is the exact problem? Other than this part, rest of the code seems fine to merge.
At the time when I wrote this, there was still a completely wrong example stored there Look at it tomorrow and then I merge
Hi again,
Thanks for renaming @efectn. It seems the PR is ready to merge. I have a question to @ReneWerner87 about the latest comment.
please revise your readme -> the example confused me at first
Which part confused you? What is the exact problem? Other than this part, rest of the code seems fine to merge.
It was related app.Use
part but i fixed that.
It was related app.Use part but i fixed that
Thanks, @efectn. Looking forward to seeing the merge of the PR. 🚀
Thanks, @ReneWerner87 as well.
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord