Skip to content

Implement backoff and jitter timeout by spec RTB1 - #852

Merged
QuintinWillison merged 10 commits into
mainfrom
feature/backoff_jitter
Nov 23, 2022
Merged

Implement backoff and jitter timeout by spec RTB1#852
QuintinWillison merged 10 commits into
mainfrom
feature/backoff_jitter

Conversation

@qsdigor

@qsdigorqsdigor commented Nov 15, 2022

Copy link
Copy Markdown
Contributor

No description provided.

@qsdigorqsdigor added the enhancement New feature or improved functionality. label Nov 15, 2022
@qsdigorqsdigor self-assigned this Nov 15, 2022
@qsdigorqsdigor linked an issue Nov 15, 2022 that may be closed by this pull request
Implement incremental backoff and jitter #795
Closed
@github-actions
github-actionsBot temporarily deployed to staging/pull/852/javadoc November 15, 2022 14:49 Inactive
QSD_igor
Fix TimerUtil backoff and jitter calculation
a953b29
Add test for TimerUtil range test
@github-actions
github-actionsBot temporarily deployed to staging/pull/852/javadoc November 16, 2022 13:54 Inactive
@github-actions
github-actionsBot temporarily deployed to staging/pull/852/javadoc November 16, 2022 13:57 Inactive
@github-actions
github-actionsBot temporarily deployed to staging/pull/852/javadoc November 17, 2022 09:40 Inactive
@github-actions
github-actionsBot temporarily deployed to staging/pull/852/javadoc November 17, 2022 15:38 Inactive
QSD_igor
Add test for disconnect channel retry jitter
6e30963
Improve test for TimerUtilsTest
@github-actions
github-actionsBot temporarily deployed to staging/pull/852/javadoc November 18, 2022 16:15 Inactive
@github-actions
github-actionsBot temporarily deployed to staging/pull/852/javadoc November 21, 2022 09:56 Inactive
@github-actions
github-actionsBot temporarily deployed to staging/pull/852/javadoc November 21, 2022 10:00 Inactive
@qsdigor
qsdigor marked this pull request as ready for review November 21, 2022 11:55
owenpearson
owenpearson requested changes Nov 21, 2022
View reviewed changes

@owenpearsonowenpearson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The implementation looks correct to me, I've left a couple of minor comments. I would also strongly consider using Random with a seed for testing if possible, that will give you a much cleaner and more predictable way of testing this.

Comment threadlib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java
Comment threadlib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java Outdated
@github-actions
github-actionsBot temporarily deployed to staging/pull/852/javadoc November 21, 2022 13:03 Inactive
owenpearson
owenpearson approved these changes Nov 21, 2022
View reviewed changes

@owenpearsonowenpearson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks

QuintinWillison
QuintinWillison suggested changes Nov 22, 2022
View reviewed changes
Comment threadlib/src/main/java/io/ably/lib/realtime/ChannelBase.java Outdated
Comment threadlib/src/main/java/io/ably/lib/transport/ConnectionManager.java
KacperKluka
KacperKluka approved these changes Nov 22, 2022
View reviewed changes

@KacperKlukaKacperKluka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 👍

ikbalkaya
ikbalkaya approved these changes Nov 23, 2022
View reviewed changes

@ikbalkayaikbalkaya left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have not carefully checked spec against this change and I just generally went through code which looks good to me

@QuintinWillison
QuintinWillison merged commit f9e4919 into mainNov 23, 2022
@QuintinWillison
QuintinWillison deleted the feature/backoff_jitter branch November 23, 2022 14:17
sacOO7
sacOO7 reviewed Jun 14, 2023
View reviewed changes
Comment threadlib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java
for (int i = 0; i < retryValues.size(); i++) {
long retryTime = retryValues.get(i);
long higherRange = disconnectedRetryTimeout + Math.min(i, 3) * 50L;
double lowerRange = 0.6 * disconnectedRetryTimeout + Math.min(i, 3) * 50L;

@sacOO7sacOO7Jun 14, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

change to

double lowerRange = 0.8 * (disconnectedRetryTimeout + Math.min(i, 3) * 50L);

sacOO7
sacOO7 reviewed Jun 14, 2023
View reviewed changes
Comment threadlib/src/test/java/io/ably/lib/util/TimerUtilsTest.java
int defaultTimerMs = 150;
int timerMs = TimerUtil.getRetryTime(defaultTimerMs, i);
long higherRange = defaultTimerMs + Math.min(i, 3) * 50L;
double lowerRange = 0.3 * defaultTimerMs + Math.min(i, 3) * 50L;

@sacOO7sacOO7Jun 14, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

change to

double lowerRange = 0.8 * (defaultTimerMs + Math.min(i, 3) * 50L);

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sacOO7sacOO7sacOO7 left review comments
@owenpearsonowenpearsonowenpearson approved these changes
+3 more reviewers
@ikbalkayaikbalkayaikbalkaya approved these changes
@QuintinWillisonQuintinWillisonQuintinWillison approved these changes
@KacperKlukaKacperKlukaKacperKluka approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@qsdigorqsdigor

Labels

enhancementNew feature or improved functionality.

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Implement incremental backoff and jitter

6 participants

@qsdigor@ikbalkaya@QuintinWillison@sacOO7@owenpearson@KacperKluka