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

[Blob][Fix]fix downloader.chunks() return chunks in different size #17559

Merged
merged 9 commits into from
Apr 20, 2021

Conversation

xiafu-msft
Copy link
Contributor

@xiafu-msft xiafu-msft commented Mar 25, 2021

when iterate through downloader.chunks() the chunk size is different. The first chunk size is always equals to max_single_get_size while it should equal to max_chunk_get_size
This will fix #9419 and #15648

@ghost ghost added the Storage Storage Service (Queues, Blobs, Files) label Mar 25, 2021
@xiafu-msft xiafu-msft marked this pull request as ready for review March 25, 2021 18:26
@xiafu-msft xiafu-msft changed the title [Blob][Fix]fix downloader.chunks() [Blob][Fix]fix downloader.chunks() return chunks in different size Mar 25, 2021
Copy link
Contributor

@tasherif-msft tasherif-msft left a comment

Choose a reason for hiding this comment

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

Great work and test LGTM :) please add the comment I mentioned in my review for that case as it will be helpful to understand the code for future devs.


return self._current_content
return self._get_chunk_data()
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for calling self._get_chunk_data() in this case? as far as I understand if we hit this case (we're done iterating the first single get call and now iterating through our download chunks like we usually do) then current content should always be the same as the chunk size so we can just return self.current_content. It seems from your try-except that there is an edge case where there will be excess content - could you just put a small comment on that in the code - its not obvious to me haha

EDIT: I just realized the case is when the current content from the first get is still there but smaller than chunk size therefore we want to make sure its also included. I think adding a comment in the code would be very helpful here!
Good work thinking of all the cases

@xiafu-msft xiafu-msft enabled auto-merge (squash) April 16, 2021 23:21
@xiafu-msft xiafu-msft closed this Apr 20, 2021
auto-merge was automatically disabled April 20, 2021 01:10

Pull request was closed

@xiafu-msft xiafu-msft reopened this Apr 20, 2021
@xiafu-msft xiafu-msft merged commit 68095e0 into Azure:master Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document how to download blob with asynchronous generator
2 participants