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

Fetch artifacts in parallel. Treat warnings as errors #376

Merged
merged 1 commit into from
Nov 2, 2018

Conversation

bootstraponline
Copy link
Contributor

Fix #323

downloadFile.parent.toFile().mkdirs()
blob.downloadTo(downloadFile)
runBlocking {
filtered.forEach { matrix ->
Copy link
Contributor

Choose a reason for hiding this comment

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

should the amount of parallel executions be limited?

Copy link
Contributor Author

@bootstraponline bootstraponline Nov 2, 2018

Choose a reason for hiding this comment

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

with the current code, there's one parallel execution per matrix that's finished & not yet downloaded.

I don't think this will be a problem. The download requests for each matrix are still run synchronously.

if it is an issue then we could look into placing an upper limit on the amount of concurrency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https:/Kotlin/kotlinx.coroutines/blob/d6a5a399d1724ff56bbb285b25df071dbc98b715/core/kotlinx-coroutines-core/src/CommonPool.kt#L23

If the property is not specified, Runtime.getRuntime().availableProcessors() - 1 will be used

Kotlin/kotlinx.coroutines#261

based on the above, looks like we're good

@bootstraponline bootstraponline merged commit 1e5815a into master Nov 2, 2018
@bootstraponline bootstraponline deleted the parallel_artifacts branch November 2, 2018 15:21
sravanmedarapu pushed a commit to sravanmedarapu/flank that referenced this pull request Nov 3, 2018
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