✨ v3 (feature): add retry mechanism #1972

gozeloglu · 2022-07-09T21:50:27Z

General logic is implemented.
Unit tests are added.

Signed-off-by: Gökhan Özeloğlu gokhan.ozeloglu@deliveryhero.com

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.

* General logic is implemented. * Unit tests are added. Signed-off-by: Gökhan Özeloğlu <gokhan.ozeloglu@deliveryhero.com>

welcome · 2022-07-09T21:50:29Z

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

middleware/retry/exponential_backoff_test.go

* Replaced testify/assert with fiber's assert. Signed-off-by: Gökhan Özeloğlu <gokhan.ozeloglu@deliveryhero.com>

* currentInterval bug is fixed in Retry. * If condition is fixed in next. * struct definition refactored and if condtion is removed in TestExponentialBackoff_Retry. Signed-off-by: Gökhan Özeloğlu <gokhan.ozeloglu@deliveryhero.com>

gozeloglu · 2022-07-11T20:21:28Z

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

efectn

I think we should be able to change defaults by a config.
Jitter backoff would be great.
Hooks would be also great.
A simple README for the middleware.
Should we integrate this into the client directly?

* Constant variables are removed. * Helper function is added for default. * Helper function is used in New function. Signed-off-by: Gökhan Özeloğlu <gokhan.ozeloglu@deliveryhero.com>

* Random number generation package has been replaced with more secure one, crypto/rand. Signed-off-by: Gökhan Özeloğlu <gokhan.ozeloglu@deliveryhero.com>

* Explanation and examples are added. Signed-off-by: Gökhan Özeloğlu <gokhan.ozeloglu@deliveryhero.com>

gozeloglu · 2022-07-13T10:56:28Z

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.

efectn · 2022-07-13T11:41:22Z

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

* Comment lines are added for ExponentialBackoff variables. Signed-off-by: Gökhan Özeloğlu <gokhan.ozeloglu@deliveryhero.com>

* Unused package(s) removed. Signed-off-by: Gökhan Özeloğlu <gokhan.ozeloglu@deliveryhero.com>

gozeloglu · 2022-07-13T21:12:22Z

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.

efectn · 2022-07-13T21:54:51Z

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

Ja7ad · 2022-07-16T04:57:12Z

middleware/retry/config.go

+	if len(config) == 0 {
+		return DefaultConfig
+	}
+	cfg := config[0]


Why do we need to get the config slice if we have set element 0 of the config slice?

I checked configs in other parts and I saw it as a general convention or something like that.

It's because of the make config parameter optional.

It's because of the make config parameter optional.

Usually get a slice of the configuration with a type options pattern.
For normal configuration, getting slice is not necessary.

gozeloglu · 2022-07-16T10:05:51Z

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.

efectn · 2022-07-16T10:18:45Z

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?

ReneWerner87 · 2022-07-16T11:05:21Z

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

efectn · 2022-07-16T16:44:42Z

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?

Ja7ad · 2022-07-17T04:06:33Z

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.

gozeloglu · 2022-07-17T20:49:06Z

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.

efectn · 2022-07-30T08:33:53Z

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.

gozeloglu · 2022-08-02T10:21:14Z

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

ReneWerner87 · 2022-08-02T10:27:45Z

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

wangjq4214 · 2022-08-06T08:53:13Z

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?

ReneWerner87 · 2022-08-08T08:11:05Z

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

efectn · 2022-08-16T08:35:41Z

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.

gozeloglu · 2022-08-16T09:50:02Z

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

gozeloglu · 2022-08-18T20:39:36Z

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.

ReneWerner87 · 2022-08-18T20:52:06Z

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

efectn · 2022-08-18T21:15:36Z

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.

gozeloglu · 2022-08-18T21:17:19Z

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.

welcome · 2022-08-19T06:20:15Z

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

v3-retry-mechanism: Add retry mechanism

2c66b22

* General logic is implemented. * Unit tests are added. Signed-off-by: Gökhan Özeloğlu <gokhan.ozeloglu@deliveryhero.com>

efectn reviewed Jul 10, 2022

View reviewed changes

middleware/retry/exponential_backoff_test.go Outdated Show resolved Hide resolved

efectn changed the title ~~✨ v3 (feature):: Add retry mechanism~~ ✨ v3 (feature): add retry mechanism Jul 10, 2022

efectn added ✏️ Feature v3 labels Jul 10, 2022

efectn linked an issue Jul 10, 2022 that may be closed by this pull request

🚀 v3 Request: Retry Mechanism #1840

Closed

efectn added this to the v3 milestone Jul 10, 2022

Gökhan Özeloğlu added 2 commits July 10, 2022 15:34

Refactor test assertion

cd5d0cd

* Replaced testify/assert with fiber's assert. Signed-off-by: Gökhan Özeloğlu <gokhan.ozeloglu@deliveryhero.com>

Add test for next method

ec934b1

* currentInterval bug is fixed in Retry. * If condition is fixed in next. * struct definition refactored and if condtion is removed in TestExponentialBackoff_Retry. Signed-off-by: Gökhan Özeloğlu <gokhan.ozeloglu@deliveryhero.com>

gozeloglu marked this pull request as ready for review July 11, 2022 20:20

gozeloglu requested a review from efectn July 11, 2022 20:21

efectn reviewed Jul 11, 2022

View reviewed changes

Gökhan Özeloğlu added 3 commits July 12, 2022 19:51

Add config for retry.

806dfae

* Constant variables are removed. * Helper function is added for default. * Helper function is used in New function. Signed-off-by: Gökhan Özeloğlu <gokhan.ozeloglu@deliveryhero.com>

Replace math/rand with crypto/rand

5fe0adb

* Random number generation package has been replaced with more secure one, crypto/rand. Signed-off-by: Gökhan Özeloğlu <gokhan.ozeloglu@deliveryhero.com>

Add a README for retry middleware

abd05a0

* Explanation and examples are added. Signed-off-by: Gökhan Özeloğlu <gokhan.ozeloglu@deliveryhero.com>

Gökhan Özeloğlu added 2 commits July 13, 2022 14:44

Add comment line for documentation

47a50c8

* Comment lines are added for ExponentialBackoff variables. Signed-off-by: Gökhan Özeloğlu <gokhan.ozeloglu@deliveryhero.com>

Run go mod tidy

f7949c1

* Unused package(s) removed. Signed-off-by: Gökhan Özeloğlu <gokhan.ozeloglu@deliveryhero.com>

Ja7ad reviewed Jul 16, 2022

View reviewed changes

efectn approved these changes Jul 30, 2022

View reviewed changes

efectn requested a review from ReneWerner87 July 30, 2022 08:44

efectn mentioned this pull request Aug 4, 2022

✨ v3 (feature): client refactor #1986

Merged

27 tasks

move middleware -> addon

2bab177

ReneWerner87 approved these changes Aug 19, 2022

View reviewed changes

ReneWerner87 merged commit 4adda50 into gofiber:v3-beta Aug 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ v3 (feature): add retry mechanism #1972

✨ v3 (feature): add retry mechanism #1972

gozeloglu commented Jul 9, 2022 •

edited

Loading

welcome bot commented Jul 9, 2022

gozeloglu commented Jul 11, 2022

efectn left a comment

gozeloglu commented Jul 13, 2022

efectn commented Jul 13, 2022

gozeloglu commented Jul 13, 2022

efectn commented Jul 13, 2022

Ja7ad Jul 16, 2022

gozeloglu Jul 16, 2022

efectn Jul 16, 2022

Ja7ad Jul 17, 2022

gozeloglu commented Jul 16, 2022

efectn commented Jul 16, 2022

ReneWerner87 commented Jul 16, 2022

efectn commented Jul 16, 2022

Ja7ad commented Jul 17, 2022

gozeloglu commented Jul 17, 2022

efectn commented Jul 30, 2022

gozeloglu commented Aug 2, 2022

ReneWerner87 commented Aug 2, 2022

wangjq4214 commented Aug 6, 2022

ReneWerner87 commented Aug 8, 2022

efectn commented Aug 16, 2022

gozeloglu commented Aug 16, 2022

gozeloglu commented Aug 18, 2022

ReneWerner87 commented Aug 18, 2022

efectn commented Aug 18, 2022

gozeloglu commented Aug 18, 2022

welcome bot commented Aug 19, 2022

✨ v3 (feature): add retry mechanism #1972

✨ v3 (feature): add retry mechanism #1972

Conversation

gozeloglu commented Jul 9, 2022 • edited Loading

welcome bot commented Jul 9, 2022

gozeloglu commented Jul 11, 2022

efectn left a comment

Choose a reason for hiding this comment

gozeloglu commented Jul 13, 2022

efectn commented Jul 13, 2022

gozeloglu commented Jul 13, 2022

efectn commented Jul 13, 2022

Ja7ad Jul 16, 2022

Choose a reason for hiding this comment

gozeloglu Jul 16, 2022

Choose a reason for hiding this comment

efectn Jul 16, 2022

Choose a reason for hiding this comment

Ja7ad Jul 17, 2022

Choose a reason for hiding this comment

gozeloglu commented Jul 16, 2022

efectn commented Jul 16, 2022

ReneWerner87 commented Jul 16, 2022

efectn commented Jul 16, 2022

Ja7ad commented Jul 17, 2022

gozeloglu commented Jul 17, 2022

efectn commented Jul 30, 2022

gozeloglu commented Aug 2, 2022

ReneWerner87 commented Aug 2, 2022

wangjq4214 commented Aug 6, 2022

ReneWerner87 commented Aug 8, 2022

efectn commented Aug 16, 2022

gozeloglu commented Aug 16, 2022

gozeloglu commented Aug 18, 2022

ReneWerner87 commented Aug 18, 2022

efectn commented Aug 18, 2022

gozeloglu commented Aug 18, 2022

welcome bot commented Aug 19, 2022

gozeloglu commented Jul 9, 2022 •

edited

Loading