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

Auto Retry (Idempotency requests) #462

Closed
wants to merge 2 commits into from
Closed

Auto Retry (Idempotency requests) #462

wants to merge 2 commits into from

Conversation

gildub
Copy link
Collaborator

@gildub gildub commented Nov 2, 2018

Auto Retry option for all services

Auto Retry is off by default but can be turned on with :idempotent option.
Default values are :retry_limit => 2 and :retry_interval => 5 (seconds)

Only GET and DELETE REST verbs are used for auto retry. More verbs can be added if that make sense.

Addresses #453

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 2, 2018

Build succeeded.

@gildub gildub changed the title [WIP] Idempotency Idempotency Mar 20, 2019
@gildub gildub requested review from aufi, Ladas and timuralp March 20, 2019 08:24
@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 20, 2019

Build succeeded.

@gildub gildub changed the title Idempotency Auto Retry Mar 20, 2019
@gildub gildub changed the title Auto Retry Auto Retry (Idempotency requests) Mar 20, 2019
@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 20, 2019

Build succeeded.

@idempotent ||= options[:idempotent].nil? ? false : !!options[:idempotent]
@retry_interval ||= options[:retry_interval] ? options[:retry_interval] : 5 # Seconds
@retry_limit ||= options[:retry_limit] ? options[:retry_limit] : 2

Copy link
Member

@aufi aufi Mar 21, 2019

Choose a reason for hiding this comment

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

Just a nit, but if I'm not mistaken, 3 lines above could be written in shorter way (also matching to pattern few lines bellow)

@idempotent     ||= !!options[:idempotent]
@retry_interval ||= options[:retry_interval] || 5 # Seconds
@retry_limit    ||= options[:retry_limit] || 2

Some test would be nice too..

Generally nice PR, overall looks good to me!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's neat.

Absolutely, I'lll looking into adding some tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@aufi, sorry didn't get the bandwidth to do it.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 21, 2019

Build succeeded.

@aufi
Copy link
Member

aufi commented Feb 12, 2020

@gildub Hi Gilles, I know it is late, but I'm happy merging this PR when the https://github.com/fog/fog-openstack/pull/462/conflicts gets resolved.

@gildub
Copy link
Collaborator Author

gildub commented Apr 8, 2020

@aufi, Hi Marek, sorry for slow response. I've addressed conflict. Thanks!

@theopenlab-ci
Copy link

theopenlab-ci bot commented Apr 8, 2020

Build succeeded.

@PatrickFranken
Copy link

PatrickFranken commented May 12, 2021

@aufi any chance to merge this into the master? The Swift service used by our company is pretty unreliable and this should solve that issue.

@ares
Copy link
Collaborator

ares commented Feb 4, 2022

@ShamoX this seems as a good and safe addition and ready to get in. Of course tests are missing, what is your preference here?

@ShamoX
Copy link
Contributor

ShamoX commented Feb 4, 2022

Yes, tests are missing. I don't really see how we can tests this except by implementing a complexe request/response scenario, which seems not easy to implement with VCRs (maybe with a sort of interaction with doubles - but seems really complicated).

Anyway, I can review it more precisely when I have more time and see if I can catch any statically analysable bad behaviours. But I doubt it.

We should at least test new configuration variables to see if they are used.

@gildub gildub closed this by deleting the head repository Sep 13, 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.

5 participants