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

Incremental backoff and jitter #1520

Closed
wants to merge 7 commits into from
Closed

Conversation

maratal
Copy link
Collaborator

@maratal maratal commented Nov 6, 2022

Closes #1431
There is an unresolved conversation in the reference implementation for channel - ably/ably-js#1008 (comment) I've decided to take state check from Owen's implementation.

@maratal maratal marked this pull request as draft November 6, 2022 20:55
@github-actions github-actions bot temporarily deployed to staging/pull/1520/jazzydoc November 6, 2022 21:01 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1520/jazzydoc November 11, 2022 14:05 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1520/jazzydoc November 11, 2022 18:38 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1520/jazzydoc November 11, 2022 20:06 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1520/jazzydoc November 11, 2022 20:37 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1520/jazzydoc November 11, 2022 21:05 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1520/jazzydoc November 11, 2022 21:28 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1520/jazzydoc November 12, 2022 20:56 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1520/jazzydoc November 12, 2022 21:36 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1520/jazzydoc November 12, 2022 22:15 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1520/jazzydoc November 12, 2022 22:45 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1520/jazzydoc November 12, 2022 23:36 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1520/jazzydoc November 13, 2022 00:10 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1520/jazzydoc November 13, 2022 00:30 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1520/jazzydoc November 13, 2022 13:16 Inactive
@maratal maratal force-pushed the feature/1431-backoff-and-jitter branch from d5930a6 to 467b893 Compare November 13, 2022 14:01
@github-actions github-actions bot temporarily deployed to staging/pull/1520/jazzydoc November 13, 2022 14:08 Inactive
@maratal maratal marked this pull request as ready for review November 13, 2022 14:39
Copy link
Member

@owenpearson owenpearson left a comment

Choose a reason for hiding this comment

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

The implementation LGTM, although I don't know much objective-c so it's hard to say for sure.

A couple of things to mention:

  • There are some newly skipped tests here which shouldn't be the case - is there a particular reason why these tests can't be fixed?
  • It might be worth considering using a seed for random number generation in the tests here to make retry intervals deterministic (this wasn't possible in ably-js because Math.random() doesn't take a seed argument).

Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian left a comment

Choose a reason for hiding this comment

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

Will review the content of the PR next, but re the skipped tests, I think ideally we should investigate and fix this here, and not leave it for #1526 and #1527. The newly-skipped tests are tests that were passing before, and have only started to fail after we added these new tests. Is there something that makes you think that the problem lies with the existing tests, and not the new ones that we've added?

@maratal
Copy link
Collaborator Author

maratal commented Nov 17, 2022 via email

@lawrence-forooghian
Copy link
Collaborator

Are the tests that you've marked as skipped failing consistently after adding the new tests?

Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian left a comment

Choose a reason for hiding this comment

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

I need to dive into this properly and understand the tests, but here are some initial thoughts from looking over the code.

Source/ARTRealtime.m Outdated Show resolved Hide resolved
Source/ARTRealtime+Private.h Outdated Show resolved Hide resolved
Source/ARTRealtime+Private.h Outdated Show resolved Hide resolved
Spec/Tests/RealtimeClientConnectionTests.swift Outdated Show resolved Hide resolved
@lawrence-forooghian
Copy link
Collaborator

I just tried (locally) un-skipping the tests mentioned in #1526 and #1527, and I didn't see them failing. But what I did see was some other failures:

  • RealtimeClientConnectionTests.swift:2193 test__058__Connection__connection_request_fails__connection_attempt_should_fail_if_not_connected_within_the_default_realtime_request_timeout(): Waited more than 20.0 seconds
  • RealtimeClientConnectionTests.swift:2206 test__058__Connection__connection_request_fails__connection_attempt_should_fail_if_not_connected_within_the_default_realtime_request_timeout(): Start date or end date are empty
  • RealtimeClientConnectionTests.swift:2246 test__059__Connection__connection_request_fails__if_connection_attempt_fails_connection_state_will_transition_to_DISCONNECTED_with_periodic_connection_attempts_with_incremental_backoff_and_jitter_until_SUSPENDED(): expected to be close to <1.0933> (within 0.2187), got <1.3333>

@github-actions github-actions bot temporarily deployed to staging/pull/1520/jazzydoc November 19, 2022 17:01 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1520/features April 2, 2023 14:42 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1520/jazzydoc April 2, 2023 14:48 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1520/features April 2, 2023 17:40 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1520/features April 2, 2023 17:41 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1520/jazzydoc April 2, 2023 17:46 Inactive
@lawrence-forooghian lawrence-forooghian force-pushed the refactor-transitions branch 4 times, most recently from 1d72363 to 7665b51 Compare April 3, 2023 20:14
Base automatically changed from refactor-transitions to main April 3, 2023 20:54
@lawrence-forooghian lawrence-forooghian changed the base branch from main to emit-retry-attempt April 4, 2023 12:25
@github-actions github-actions bot temporarily deployed to staging/pull/1520/features April 4, 2023 12:26 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1520/jazzydoc April 4, 2023 12:33 Inactive
Base automatically changed from emit-retry-attempt to main April 5, 2023 00:28
@lawrence-forooghian
Copy link
Collaborator

I'm going to close this PR — I intend to land the remainder of this work in #1611, #1612, #1613.

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.

Implement incremental backoff and jitter
5 participants