backoff: Make duration bounds customizable #975
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gnprice
added
a-api
PIG208
reviewed
Oct 5, 2024
lib/api/backoff.dart
Outdated
| /// The actual duration will vary randomly up to this value; see [wait]. | ||
| static const firstBound = Duration(milliseconds: 100); | ||
|
|
||
| /// The maximum bound on the duration of each wait, even after many waits. |
There was a problem hiding this comment.
nit: Should we mention explicitly that this, and firstBound are upper bounds on the first line, respectively? I feel that these are the first reads for one when looking at the BackoffMachine interface.
|
Sorry for the delay! Posted a comment. This LGTM overall. |
This field is initialized but never looked at thereafter. So we can just cut it. It looks like it was never used in the previous history of this file, nor in the history of the corresponding code in zulip-mobile. Presumably it was left over from an earlier draft of that zulip-mobile implementation.
This code was originally adapted from an implementation in JavaScript (from zulip-mobile), and followed that implementation in representing durations mostly as doubles in milliseconds. Before making other changes here, and in particular exposing some of these durations in the class's API, let's adapt it more fully to Dart idioms and norms.
… body The use of an expression body (with `=>`) in this situation is forbidden in the upstream Flutter style guide: https://github.com/flutter/flutter/blob/e6c955416da/docs/contributing/Style-guide-for-Flutter-repo.md#consider-using--for-short-functions-and-methods https://github.com/flutter/flutter/blob/e6c955416da/docs/contributing/Style-guide-for-Flutter-repo.md#use--for-getters-and-callbacks-that-just-return-literals-or-switch-expressions and I've regularly asked in code reviews to replace such things with block bodies. Apparently I hadn't yet absorbed that point, though, when I wrote this code earlier this year. And then just now I was rereading it in order to refactor it, and found I had to visually scan the test's code several times in order to see its overall structure. Fix that.
Also make an editing pass over the [BackoffMachine.wait] doc.
|
Thanks for the reviews! Merging, with that tweak. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This will let us have some backoff loops that back off more patiently than others, e.g. because their work is happening in the background and isn't expected to be something the user is already waiting for.
In particular I plan to use this for backing off the fetch of server emoji data, for #669.