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

Bug 1780806 - Allow the uploader to signal it is done handling uploads. #2136

Merged
merged 1 commit into from
Aug 26, 2022

Conversation

badboy
Copy link
Member

@badboy badboy commented Jul 29, 2022

Putting this up so it's up.
I want to discuss this with chutten when he's back.

I'm not super happy with it because the UploadResult::Done needs to be handled on a different layer than all the other UploadResult variants.
(It's also only implemented for the RLB right now)

@badboy badboy requested a review from chutten July 29, 2022 09:04
glean-core/rlb/src/test.rs Outdated Show resolved Hide resolved
glean-core/rlb/src/net/mod.rs Outdated Show resolved Hide resolved
@badboy badboy force-pushed the 1780806-signal-uploader-shutdown branch from c7f2283 to 9a13b82 Compare August 19, 2022 14:41
@badboy
Copy link
Member Author

badboy commented Aug 19, 2022

Ok, I reworked this a bit now.
The Done is passed through and then glean_process_ping_upload_response returns something that instructs the upload loop whether to continue or end. I also implemented that for Kotlin now to show that it works.

Additionally I reworked the test. Instead of arbitrary sleep times it now uses a barrier to sync up the 2 threads. That should be much more reliable.

@badboy badboy force-pushed the 1780806-signal-uploader-shutdown branch 2 times, most recently from 3248c16 to db59268 Compare August 19, 2022 15:05
@badboy badboy requested a review from chutten August 22, 2022 15:58
@badboy badboy marked this pull request as ready for review August 22, 2022 15:58
@badboy badboy requested a review from a team as a code owner August 22, 2022 15:58
Copy link
Contributor

@chutten chutten left a comment

Choose a reason for hiding this comment

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

Needs CHANGELOG

glean-core/rlb/src/net/mod.rs Show resolved Hide resolved
.detekt.yml Show resolved Hide resolved
glean-core/rlb/src/test.rs Show resolved Hide resolved
glean-core/src/upload/result.rs Outdated Show resolved Hide resolved
@badboy badboy force-pushed the 1780806-signal-uploader-shutdown branch from db59268 to 1e77964 Compare August 23, 2022 10:16
@codecov
Copy link

codecov bot commented Aug 23, 2022

Codecov Report

Merging #2136 (f2ff862) into main (e45fb79) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2136   +/-   ##
=======================================
  Coverage   26.47%   26.47%           
=======================================
  Files           1        1           
  Lines          34       34           
=======================================
  Hits            9        9           
  Misses         25       25           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@badboy badboy force-pushed the 1780806-signal-uploader-shutdown branch from 1e77964 to f2ff862 Compare August 25, 2022 14:37
@badboy badboy requested a review from chutten August 25, 2022 14:38
@badboy badboy merged commit bc13073 into main Aug 26, 2022
@badboy badboy deleted the 1780806-signal-uploader-shutdown branch August 26, 2022 10:17
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.

2 participants