chore: Follow up to #731 #743
Conversation
| ch := make(chan *archiveFile) | ||
| g, ctx := errgroup.WithContext(ctx) | ||
| semaphore := semaphore.NewWeighted(8) | ||
| semaphore := semaphore.NewWeighted(int64(runtime.GOMAXPROCS(0))) |
There was a problem hiding this comment.
This one is interesting, I'm not familiar with runtime.GOMAXPROCS(0), will definitely do some research here 👍
There was a problem hiding this comment.
It gives you the recommended number of processes/goroutines that can run in parallel (not just concurrently). It's relational to your cpu die count.
There was a problem hiding this comment.
Hmm, will this potentially reintroduce issues with open file limits? Or reduce the effectiveness of the semaphore in throttling remotely executed requests? In my understanding, this will scale the semaphore's limit to the host machine running src's resources?
There was a problem hiding this comment.
Number of cpu cores is way under the Linux file handle limit by many orders of magnitude
DaedalusG
left a comment
DaedalusG
left a comment
There was a problem hiding this comment.
This looks good and mostly seems like clarification of naming conventions. Except semaphore := semaphore.NewWeighted(int64(runtime.GOMAXPROCS(0))) which I'll need to learn more about.
Has the command been tested with these changes? If not I'm happy to test these out before merge.
Approving anyway
Following up from additional comments in #731 (some were hidden by the long timeline in the github view pre merge, and @DaedalusG and I forgot to address these in our 1-1).
Test plan
N/A.