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

Add Concurrent Download Support for artifacts #11531

Merged

Conversation

gowthamgts
Copy link
Contributor

This PR adds support for concurrent downloads as requested on #11244. Currently 3 is set as the max number of downloads that can be active at a time, but I'm happy to add that to client or task configuration.

resolves #11244

@hashicorp-cla
Copy link

hashicorp-cla commented Nov 18, 2021

CLA assistant check
All committers have signed the CLA.

@gowthamgts
Copy link
Contributor Author

@lgfa29 @schmichael - can you please take a look and let me know if there are any changes needed?

Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

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

Looks good but there should be a test for the case where there are 4 artifacts and the first one fails to download as currently I believe that leaks a goroutine.

A https://pkg.go.dev/net/http/httptest test server should allow testing against an artifact server where you can control the error rate.

@gowthamgts
Copy link
Contributor Author

gowthamgts commented Dec 21, 2021

hey sorry folks, got caught up with other work, i'll try to get this closed by this weekend.

@vercel vercel bot temporarily deployed to Preview – nomad December 25, 2021 08:56 Inactive
@gowthamgts gowthamgts changed the title Add Concurrent Download Support Add Concurrent Download Support for artifacts Dec 26, 2021
Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

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

Looks great! I pushed a changelog entry. Mind cleaning up the tests a bit more?

I'd like to hold off merging this to main until we do one more 1.2.x release. I'll label it so it doesn't get lost.

client/allocrunner/taskrunner/artifact_hook_test.go Outdated Show resolved Hide resolved
client/allocrunner/taskrunner/artifact_hook_test.go Outdated Show resolved Hide resolved
client/allocrunner/taskrunner/artifact_hook_test.go Outdated Show resolved Hide resolved
client/allocrunner/taskrunner/artifact_hook_test.go Outdated Show resolved Hide resolved
@schmichael schmichael added this to the 1.3.0 milestone Jan 3, 2022
@schmichael
Copy link
Member

I'll label it so it doesn't get lost.

To be fair, this did work. It didn't get lost! However I did almost forget to merge it for 1.3.0! 😬

Merging now and will be released in 1.3.0-rc1! Thanks @gowthamgts

@schmichael schmichael merged commit f601cc3 into hashicorp:main Apr 20, 2022
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Download artifacts concurrently
5 participants