-
-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
Implement concurrent downloads in brew fetch
.
#17756
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me so far! Some questions:
- what made you decide on concurrent-ruby and whirly gems?
- does this use threads or ractors?
- does this slow down/add additional requires to a
brew help command
at all?
Thanks!
Threads. From reading ruby-concurrency/concurrent-ruby#899, it seems ractors are quite limited in what they can do. In any case, network is the bottleneck here.
No, the additional |
Improved output which shows all concurrent downloads at once. concurrent-downloads.mov |
ed41a39
to
f74113f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! A few tiny nits here and then can self-merge. Nice work @reitermarkus!
9075145
to
8bb8b5a
Compare
Looks good when comments addressed. |
12143e0
to
bf4acae
Compare
@reitermarkus converted to draft until you're ready to merge |
6cca97d
to
f9726d6
Compare
@reitermarkus do you have any ETA on this being mergeable? |
255b99a
to
0f0e991
Compare
# Conflicts: # Library/Homebrew/Gemfile.lock
0f0e991
to
231ac72
Compare
@MikeMcQuaid, just rebased and technically it should be mergeable, but I'd consider it an alpha-quality PoC, given there are many things still missing. Probable makes sense to merge this and open an issue for the things still missing. |
eb5c035
to
90ced30
Compare
90ced30
to
fb0bf3b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok though I have a few questions that might have been from me reading things in the wrong order as there's a few different things going on.
|
||
sig { returns(String) } | ||
def download_type | ||
T.must(self.class.name&.split("::")&.last).gsub(/([[:lower:]])([[:upper:]])/, '\1 \2').downcase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very gnarly. May be worth adding a comment example.
We only seem to not be overriding this in one place so maybe it's worth just making it abstract
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only seem to not be overriding this in one place
Actually seven places, i.e. every subclass of Resource
, Resource
itself, and Bottle
.
@reitermarkus Sounds good. Could you address @Bo98's comments, consider adding an opt-in undocumented environment variable ( |
Co-authored-by: Bo Anderson <[email protected]>
4761817
to
69a04bd
Compare
I forgot: you can do this for @reitermarkus good to merge when @Bo98 is ✅ |
It's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested brew fetch
locally with and without --concurrency
. Seems to work well.
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?concurrent-downloads.mov