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

RUMM-709 Upload delay increases in case of network error #249

Merged
merged 2 commits into from Sep 11, 2020

Conversation

buranmert
Copy link
Contributor

@buranmert buranmert commented Sep 10, 2020

What and why?

SDK was decreasing upload interval as long as there is a batch to upload
That caused problems in case of network errors

Example case: the device can't resolve upload URL for some reason, yet SDK keeps trying to upload at short intervals.

How?

Now if there is a network error or client token error, interval increases (interval used to decrease in that case)

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference

@buranmert buranmert self-assigned this Sep 10, 2020
@buranmert buranmert added this to the rum milestone Sep 10, 2020
clientTokenError added:
unlike clientError, it doesn't decrease interval
@buranmert buranmert force-pushed the buranmert/RUMM-709-delay-upload-on-network-errors branch from 53e336f to da2e3f4 Compare September 11, 2020 14:09
@buranmert buranmert marked this pull request as ready for review September 11, 2020 14:09
@buranmert buranmert requested a review from a team as a code owner September 11, 2020 14:09
Copy link
Collaborator

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Looks good 👌, just one nit and convention. Let's fix and merge.

Comment on lines 9 to 13
internal protocol Delay {
func nextUploadDelay() -> TimeInterval
mutating func decrease()
mutating func increase()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: the name, Delay is domain-agnostic, but the nextUploadDelay() has "upload" meaning. This

func next() -> TimeInterval
mutating func decrease()
mutating func increase()

would be more coherent IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i didn't pay attention before but actually current makes more sense than next, wdyt?

}

// swiftlint:disable multiline_arguments_brackets
func test_whenThereIsNoBatch_thenIntervalIncreases() throws {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Convention: we commonly use testWhen, not test_when

Screenshot 2020-09-11 at 17 33 40

@buranmert buranmert force-pushed the buranmert/RUMM-709-delay-upload-on-network-errors branch from 52afb1b to b65dc0e Compare September 11, 2020 16:00
@buranmert buranmert merged commit 67fda73 into master Sep 11, 2020
@buranmert buranmert deleted the buranmert/RUMM-709-delay-upload-on-network-errors branch September 11, 2020 16:30
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.

None yet

2 participants