fiber icon indicating copy to clipboard operation
fiber copied to clipboard

✨ v3 (feature): add retry mechanism

Open gozeloglu opened this issue 2 years ago • 17 comments

  • 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.

gozeloglu avatar Jul 09 '22 21:07 gozeloglu

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

welcome[bot] avatar Jul 09 '22 21:07 welcome[bot]

I am ready for code review, comments, feedback, etc.

gozeloglu avatar Jul 11 '22 20:07 gozeloglu

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.

gozeloglu avatar Jul 13 '22 10:07 gozeloglu

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

efectn avatar Jul 13 '22 11:07 efectn

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.

gozeloglu avatar Jul 13 '22 21:07 gozeloglu

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

efectn avatar Jul 13 '22 21:07 efectn

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.

gozeloglu avatar Jul 16 '22 10:07 gozeloglu

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?

efectn avatar Jul 16 '22 10:07 efectn

Yeah its a good idea. I like hooks, its a good solution for customization and give the possibility to extend our functionality.

ReneWerner87 avatar Jul 16 '22 11:07 ReneWerner87

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?

efectn avatar Jul 16 '22 16:07 efectn

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.

Ja7ad avatar Jul 17 '22 04:07 Ja7ad

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.

gozeloglu avatar Jul 17 '22 20:07 gozeloglu

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.

efectn avatar Jul 30 '22 08:07 efectn

Hi @ReneWerner87. This is a polite ping. Can you review the PR?

gozeloglu avatar Aug 02 '22 10:08 gozeloglu

Oh sorry, sure will have a look this week(currently much todo)

ReneWerner87 avatar Aug 02 '22 10:08 ReneWerner87

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?

wangjq4214 avatar Aug 06 '22 08:08 wangjq4214

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?

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

ReneWerner87 avatar Aug 08 '22 08:08 ReneWerner87

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?

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.

efectn avatar Aug 16 '22 08:08 efectn

Hi @efectn. Thanks for informing me. I'll take a look and make changes as soon as possible.

gozeloglu avatar Aug 16 '22 09:08 gozeloglu

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.

gozeloglu avatar Aug 18 '22 20:08 gozeloglu

At the time when I wrote this, there was still a completely wrong example stored there Look at it tomorrow and then I merge

ReneWerner87 avatar Aug 18 '22 20:08 ReneWerner87

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.

efectn avatar Aug 18 '22 21:08 efectn

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.

gozeloglu avatar Aug 18 '22 21:08 gozeloglu

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

welcome[bot] avatar Aug 19 '22 06:08 welcome[bot]