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

Chunked file transfers for azurerm_data_lake_store_file #2633

Merged
merged 11 commits into from Jan 11, 2019

Conversation

dyindude
Copy link

@dyindude dyindude commented Jan 9, 2019

Files larger than 4MB fail to upload to ADLS with azurerm_data_lake_store_file due to lack of implementation of the API's buffersize parameter in the Azure SDK for go. There is an open issue for this on that project here: Azure/azure-sdk-for-go#3231

The referenced issue contains a workaround which transfers the file using 4MB chunks (which appears to be a static value in the API: https://docs.microsoft.com/en-us/rest/api/datalakestore/webhdfs-filesystem-apis )

I have implemented the workaround in this commit, and tested the change in a terraform project I was encountering this error with. I tried my best to make it match the style of the code already present, so let me know if something else needs to be changed.

I am relatively new to golang, so I welcome any constructive feedback! Thanks

@ghost ghost added the size/XS label Jan 9, 2019
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Hi @dyindude,

Thank you for fixing this bug! This looks pretty good to me, however could we get a new test for large files over 4Mb?

@katbyte
Copy link
Collaborator

katbyte commented Jan 10, 2019

Ideally generate a large local file rather then keep something in the repo itself

@ghost ghost added size/M and removed size/XS labels Jan 11, 2019
@dyindude
Copy link
Author

dyindude commented Jan 11, 2019

I took a stab at it, using the existing TestAccAzureRMDataLakeStoreFile_basic test as a template. I also ran the tests myself with make testacc TESTARGS='-run=TestAccAzureRMDataLakeStoreFile'

@ghost ghost removed the waiting-response label Jan 11, 2019
@katbyte
Copy link
Collaborator

katbyte commented Jan 11, 2019

Thanks @dyindude!

I hope you don't mind but I changed it to use a tmp file and to remove it after the test so I could merge it 🙂 now LGTM

@katbyte katbyte added this to the 2.0.0 milestone Jan 11, 2019
@dyindude
Copy link
Author

I don't mind at all, I have learned a lot from this process :) thanks

Copy link
Collaborator

@WodansSon WodansSon left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@katbyte katbyte merged commit 139aa76 into hashicorp:master Jan 11, 2019
katbyte added a commit that referenced this pull request Jan 11, 2019
@tombuildsstuff tombuildsstuff modified the milestones: 2.0.0, 1.22.0 Jan 21, 2019
@ghost
Copy link

ghost commented Mar 5, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@hashicorp hashicorp locked and limited conversation to collaborators Mar 5, 2019
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.

None yet

4 participants