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

_download_chunking.py: avoid to overwrite if_match #630

Merged
merged 6 commits into from
Aug 28, 2019

Conversation

marco-rossi29
Copy link
Contributor

@marco-rossi29 marco-rossi29 commented Aug 20, 2019

Solve issue #625

@marco-rossi29
Copy link
Contributor Author

@zezha-msft let me know if this is acceptable

@zezha-msft zezha-msft changed the base branch from master to dev August 27, 2019 21:49
@zezha-msft
Copy link
Contributor

Hi @marco-rossi29, sorry for the delayed reply! And thanks for contributing!

The current design has one problem: because of this line, self.if_match is never None.

In addition, when self.if_match is indeed None (which never happens right now) and we are downloading the blob in parallel chunks, then there's a data race to set its value. I propose to simply remove this line and the comments above it , as it's redundant and not needed.

Please let me know if you'd like to do that. Otherwise I can do it too. Thank you!

@marco-rossi29
Copy link
Contributor Author

marco-rossi29 commented Aug 27, 2019

No problem, thank you for responding! Yes, that's perfect for me. I removed it.

I see that you changed the base from master to dev and now there are some spurious changes. Do you want me to remove such changes from my PR by rebasing my two commits on top of 54e60c8?

@zezha-msft zezha-msft merged commit 1dc13d0 into Azure:dev Aug 28, 2019
@zezha-msft
Copy link
Contributor

@marco-rossi29 no it's ok, we had to pull back the changes from master into dev anyways. Thanks!

@marco-rossi29 marco-rossi29 deleted the patch-1 branch August 28, 2019 15:57
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.

3 participants