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

Feature spec: Add spec point for incremental backoff and jitter #1445

Merged
merged 4 commits into from
Jun 9, 2022

Conversation

owenpearson
Copy link
Member

@owenpearson owenpearson commented Jun 1, 2022

Resolves #832

RTB1a

The backoff coefficient described in RTB1a can be calculated in javascript like so:

function getBackoffCoefficient(n) {
  return Math.min((n + 2) / 3, 2);
}

The idea of this is modelled off of the '15 -> 20 -> 25 -> 30' sequence suggested by paddy in #832. For the default retry client options this will give that exact sequence, and if users configure their client to use a different retry time this will give an equivalent sequence of longer retry times proportionate to their configured retry time.

RTB1b

The jitter coefficient described in RTB1b can be calculated in javascript like so:

function getJitterCoefficient() {
  return 1 - Math.random() * 0.2;
}

The reason I chose not to allow for the jitter coefficient to be a value higher than 1 is that this could result in a retry length longer than 30s for the 4th retry and beyond. A side effect of this is that all initial retry attempts will be slightly shorter than they would have been prior to this change. It could also work to allow for a jitter coefficient between 0.8 and 1.2 and just say that the retry time is the minimum of that calculation and 30s.

RTB1

The overall retry time calculation would look like:

function getRetryTime(initValue, n) {
  return initValue * getBackoffCoefficient(n) * getJitterCoefficient();
}

The following code can be used to test it:

[1, 2, 3, 4, 5].forEach((n) => {
  console.log(getRetryTime(initValue, n));
});

Which, with an initValue of 15 (seconds) will print something like:

13.917470451245713
18.415226855678757
20.444851032898747
26.650729887092275
27.803382948778786

h3(#backoff-jitter). Incremental backoff and jitter
* @(RTB1)@ For connections in the @DISCONNECTED@ state and realtime channels in the @SUSPENDED@ state the time until retry is calculated as the product of the initial retry timeout (for @DISCONNECTED@ connections this is the @disconnectedRetryTimeout@, for @SUSPENDED@ channels this is the @channelRetryTimeout@), the backoff coefficient as defined by "@RTB1a@":#RTB1a, and the jitter coefficient as defined by "@RTB1b@":#RTB1b.
** @(RTB1a)@ The backoff coefficient for the nth retry is calculated as the minimum of @(n + 2) / 3@ and @2@.
** @(RTB1b)@ The jitter coefficient is a random number between 0.9 and 1. The randomness of this number doesn't need to be cryptographically secure but should be approximately uniformly distributed.
Copy link
Member

Choose a reason for hiding this comment

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

I think we should consider a wider range, eg (0.8, 1)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok cool, I've changed it to this in b7534ec and updated the description.

@lmars
Copy link
Member

lmars commented Jun 7, 2022

with an initValue of 15 (seconds) will print something like:

I wonder if it's worth putting this example in the spec to avoid any ambiguity, or perhaps even specifying it as a test fixture?

@owenpearson
Copy link
Member Author

I wonder if it's worth putting this example in the spec to avoid any ambiguity, or perhaps even specifying it as a test fixture?

@lmars I can't think of a way to do this without kind of bloating the spec point, and personally I think it would be a bit too much detail (I will include that example in the GitHub issues to implement this feature if that helps?). If you disagree would you mind making a suggested change?

@lmars
Copy link
Member

lmars commented Jun 8, 2022

I think we should put some examples in the test-resources directory in ably-common:

https://github.com/ably/ably-common/tree/main/test-resources

Then SDKs can use those as test fixtures, so we know all SDKs implement the feature in exactly the same way, much like we do for encryption.

That doesn't stop us merging this PR though, so I've approved.

@owenpearson
Copy link
Member Author

I've created ably/ably-common#153 to address lewis's comment regarding testing

@sacOO7
Copy link
Contributor

sacOO7 commented Jun 14, 2023

Expression to calculate upper and lower bounds for generated values using getRetryTime

upper = min((retryCount + 2) / 3, 2) *initialTimeout
lower = 0.8 * upper

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Revisit backoff strategy for client libraries
5 participants