Bug 1742093 - Refactor upload management logic to be exactly like in glean-core #988
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
The worker will be responsible for assembling ping requests and performing the actual ping uploads.
i.e. Copied missing tests from https://github.com/mozilla/glean/blob/main/glean-core/src/upload/mod.rs
brizental
changed the title
brizental
requested review from
Dexterp37 and
badboy
and removed request for
Dexterp37
Contributor
Author
|
Adding @badboy here as a reviewer as well, because he knows this mechanism very well and also I think it wouldn't hurt to get 2 pairs of eyes looking at this change to be more confident we caught any issues before merging. |
Collaborator
Build size report
|
In the previous implementation we were tying the ability to retry sending recovarebly failed upload request to the rate limiter state i.e. a user could only retry after the rate limiter time window was over. In order to do that we introduced a new state to the rate limiter, the Stopped state. When max recoverable failures was fit, we would out the rate limiter in that state. It would work just a like the Throttled state in the sense that the rate limiter would only get out of it once the time window was complete. That is actually not how the implementation in glean-core works. In there we simply stop the upload worker sending it a Done signal once max recorable failures is hit. If the worker is re-started immediatelly after it will be allowed to get new tasks. That is now how the implementation works in this repository as well. I wonder if tying the retries to the rate limiter window is still a beter idea, but that has not caused us problems up until now in glean-core, so for now, I'll leave it like this.'
brizental
force-pushed
the
1742093-rate-limit-at-upload-time
branch
from
fc34fb7 to
32a966d
Compare
badboy
approved these changes
Nov 25, 2021
Member
badboy
left a comment
badboy
left a comment
There was a problem hiding this comment.
Looks good!
Minimal things inline, but no need for me to re-review.
Comment on lines
+214
to
+215
| assert.strictEqual(resultInternalServerError.result, UploadResultStatus.Success); | ||
| assert.strictEqual(resultInternalServerError.status, 500); |
Contributor
Author
There was a problem hiding this comment.
Hah, yeah naming is hard.
| continue; | ||
| // TODO(bug1727076): Actually set a timer to continue once timeout is complete. | ||
| // Note that the timer must be cleared in case an abort signal is issued. | ||
| case UploadTaskTypes.Wait: |
Member
There was a problem hiding this comment.
Does this fall through to the Done case? If yes, all good (though maybe add a comment // explicit fallthrough).
If not it should explicitly return or it will run into the throttling 3 times immediately.
Contributor
Author
There was a problem hiding this comment.
Yeap it falls through.
Co-authored-by: Jan-Erik Rediger <badboy@archlinux.us>
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
With the previous design it was hard and hacky to enforce the rate limit defaults at the correct time.
The new design (or old, depends on how you look at it) allows to do that easily.
It also has the following added benefits 1) logic is better encapsulated in different structures 2) the design is almost exactly the same as the one in glean-core, so transfer of knowledge and ensurace of standardized functionality is easier to do 3) it doesn't have an internal dispatcher instance 4) it's just easier to reason about it, IMO.
Note: I suggest reviewing this PR commit by commit. I did my best to isolate changes, but this is a big change and there is no going around that.
Pull Request checklist
glean/folder, run:npm run testRuns all testsnpm run lintRuns all lintersCHANGELOG.mdor an explanation of why it does not need one