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.

Sign up for GitHub

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

Jump to bottom

Improve threading in CDNSource #551

Merged
merged 5 commits into from
Jun 23, 2019
Merged

Improve threading in CDNSource #551

merged 5 commits into from
Jun 23, 2019

Conversation

igor-makarov
Copy link
Contributor

@igor-makarov igor-makarov commented Jun 17, 2019 •
edited

This PR addresses several issues in CDNSource:

  1. Thread exhaustion on low-performance machines (such as CI VMs). The default number of threads is now set to 50 with an environment variable allowing to set it differently (MAX_CDN_NETWORK_THREADS).
  2. Mangled output when concurrent code was failing. Now it will provide a more structured and readable output. Also using Futures as apparently this is what the ruby-concurrent docs suggest.
  3. Non-HTTP errors were raised as exceptions. Now these will be wrapped with data about the specific download.
  4. Added retry for network-related errors (MAX_NUMBER_OF_RETRIES = 5).

@dnkoutso
Copy link
Contributor

Do we want this for 1.7.3 or 1.8.0?

@dnkoutso
Copy link
Contributor

If 1.7 then point to 1-7-stable branch and include CHANGELOG entry.

@igor-makarov
Copy link
Contributor Author

You're right, 1.7 makes more sense.
Do I need to make a new PR for that?

@dnkoutso
Copy link
Contributor

No. git rebase your branch against 1-7-stable, push it, then come here and update the target branch at the top to point to 1-7-stable instead of master

@igor-makarov
Copy link
Contributor Author

Aaah, found the pointing picker. Coming up!

@dnkoutso
Copy link
Contributor

How hard/easy is to write tests for this change?

@igor-makarov igor-makarov changed the base branch from master to 1-7-stable June 17, 2019 21:20
@igor-makarov
Copy link
Contributor Author

I rebased/repointed. Will test for the error messages but I don't know how to test the thread count.

@igor-makarov igor-makarov reopened this Jun 17, 2019
@dnkoutso dnkoutso added this to the 1.7 milestone Jun 19, 2019
@igor-makarov igor-makarov mentioned this pull request Jun 21, 2019
Closed
1 task
@igor-makarov
Copy link
Contributor Author

I ended up adding network error retries to the same PR cause it was small extra effort.

amorde
amorde reviewed Jun 22, 2019
View reviewed changes
spec/cdn_source_spec.rb Show resolved Hide resolved
lib/cocoapods-core/cdn_source.rb Outdated Show resolved Hide resolved
@igor-makarov
Copy link
Contributor Author

Addressed @amorde's feedback.

@dnkoutso dnkoutso merged commit d329a91 into CocoaPods:1-7-stable Jun 23, 2019
@dnkoutso
Copy link
Contributor

For CocoaPods/CocoaPods#8926

@igor-makarov igor-makarov deleted the threading-improvements branch June 24, 2019 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orta orta orta approved these changes

@dnkoutso dnkoutso dnkoutso approved these changes

@amorde amorde amorde approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.7
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@igor-makarov @dnkoutso @orta @amorde