Skip to content

Conversation

kurtzur
Copy link
Contributor

@kurtzur kurtzur commented Mar 16, 2018

Related issue: #558

Description

This PR introduces the flags cut-over-exponential-backoff and cut-over-exponential-backoff-max-interval. cut-over-exponential-backoff is a boolean flag which, if set to true, spaces out cutover attempts using an exponential backoff algorithm. This flag defaults to false.cut-over-exponential-backoff-max-interval is a numeric flag which dictates the upper bound that the exponentially increasing attempt spacing should obey. This flag defaults to 64 seconds, or 8 total attempts. I enforced a minimum of 2 seconds for this value, which would correspond to 3 total attempts. I added this validation because enabling exponential backoff with fewer than 3 attempts seems both pointless and likely accidental.

The exponential backoff is implemented as a separate function retryOperationWithExponentialBackoff. Like retryOperation, this function would be reusable in other parts of the project if desired.

  • contributed code is using same conventions as original code
  • script/cibuild returns with no formatting errors, build errors or unit test errors.

Copy link
Contributor

@shlomi-noach shlomi-noach left a comment

Choose a reason for hiding this comment

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

Thank you! Looking very good.

We have a flag called default-retries. I'm thinking we should respect is. Thus, the loop should not end on interval < maxInterval but rather on numAttempts < defaultRetries. We can cap interval to maxInterval and keep looping with maxInterval.

Also, if this logic is to be used by other retry operations (as you suggest), I'd advise renaming the flags so that they do not contain the text cut-over.

@shlomi-noach shlomi-noach self-assigned this Mar 19, 2018
kurtzur added 2 commits March 19, 2018 12:26
…max interval flag more generically (#2)

* rename flags, obey defaultNumRetries

* capitalization fixes

* fix flag description typo

* fix sleep algorithm
@kurtzur
Copy link
Contributor Author

kurtzur commented Mar 19, 2018

@shlomi-noach I made the updates you suggested, with one exception: I don't think we should rename the flag cut-over-exponential-backoff to exponential-backoff. That's because my changeset only respects this boolean flag for the cut-over operation specifically. It would be misleading to give it a generic name. It might be nice to have a globally obeyed exponential-backoff flag, but I'd have to find and update each place where it should be obeyed.

@shlomi-noach shlomi-noach changed the base branch from master to exponential-backoff March 20, 2018 06:14
@shlomi-noach
Copy link
Contributor

Sent to testing.

@shlomi-noach shlomi-noach merged commit 731dfab into github:exponential-backoff Mar 25, 2018
kurtzur pushed a commit to Gusto/gh-ost that referenced this pull request Apr 12, 2018
Support exponential backoff for cutover attempts
@kurtzur
Copy link
Contributor Author

kurtzur commented Apr 12, 2018

@shlomi-noach Why was this merged into the branch github:exponential-backoff and not master? We're currently running a fork of this repository, so I was hoping to pull in both my change and any other new changes at the same time. Is the change still undergoing testing?

@shlomi-noach
Copy link
Contributor

@kurtzur ah, my mistake! Will merge to master. Thank you for pointing this out.

@shlomi-noach
Copy link
Contributor

Merged into master

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.

2 participants