[Storage]fix retry on large block upload #17909
Conversation
xiafu-msft
requested review from
amishra-dev,
annatisch,
kasobol-msft,
tasherif-msft and
zezha-msft
as code owners
ghost
added
the
xiafu-msft
force-pushed
the
fix-large-block
branch
from
57443b9
to
bee97a4
Compare
|
Just to confirm this fix is related to https://github.com/Azure/azure-sdk-for-python/pull/17078/files? I think its important to give a little bit of description to this PR for future reference. As far as I understand, core introduced a change to retry when blob content downloading is interrupted. This code path (for the retry) never really worked and a bug was introduced where incomplete downloads can occur without any exception raised. With the PR I mentioned above core now removed the retries on their side and fixed the exception issue (so now it raises if download is interrupted). Could you please confirm that my description here is accurate? |
|
@tasherif-msft good suggestion! Also I will make this pr into two since these two commits are not related, then I will quote your description there |
| @@ -54,7 +54,6 @@ async def send(self, request): | |||
| request.context.options.pop('raw_response_hook', self._response_callback) | |||
|
|
|||
| response = await self.next.send(request) | |||
| await response.http_response.load_body() | |||
There was a problem hiding this comment.
What was the point of this line anyway? I assume the line above it already ensures this is done anyway?
There was a problem hiding this comment.
this is to load all data in stream, even we've already specifies stream=True.
| self._encryption_options | ||
| ) | ||
| retry_active = False | ||
| except (requests.exceptions.ChunkedEncodingError, requests.exceptions.ConnectionError) as error: |
There was a problem hiding this comment.
is ContentDecodingError also a possibility here? I saw that it can be raised in
def generate():
# Special case for urllib3.
if hasattr(self.raw, 'stream'):
try:
for chunk in self.raw.stream(chunk_size, decode_content=True):
yield chunk
except ProtocolError as e:
raise ChunkedEncodingError(e)
except DecodeError as e:
raise ContentDecodingError(e)
except ReadTimeoutError as e:
raise ConnectionError(e)
Just want to make sure its not applicable here
There was a problem hiding this comment.
For example if you get a partial gzip file and try to decode you will get ContentDecodingError, we want to throw immediately since retry wouldn't help.
sdk/storage/azure-storage-file-share/azure/storage/fileshare/_shared/uploads.py
Show resolved
Hide resolved
xiafu-msft
force-pushed
the
fix-large-block
branch
from
bee97a4
to
7b1651b
Compare
| # so when we retry we don't want to read from current position of wrapped stream, | ||
| # instead we should seek to where we want to read from. | ||
| if self._wrapped_stream.tell() != absolute_position: | ||
| self._wrapped_stream.seek(absolute_position, SEEK_SET) |
There was a problem hiding this comment.
note to self: we could also always seek but this is bad because it wouldn't be as efficient (as Emily explained offline)
add xms-ids for notificationhubs (Azure#17909)
No description provided.