Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
fix: correct lastChunk retry logic in BlobWriteChannel #918
fix: correct lastChunk retry logic in BlobWriteChannel #918
Changes from 1 commit
026b620
8926244
94cc687
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, this means that the caller would have made a request to do an upload, but they get an error from the request to check the upload status which is a separate RPC. This might be confusing-- is there a way to wrap this in an error that corresponds to the method they originally called?
Also, if the response is a 308, I would assume that instead of failing we should retry from the last offset, no?
(Ignore me if I'm just missing how this works).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally, when a resumable upload completes the shallow Object will be returned in the response. If the response is not received for any reason (network failure usually), this method allows for querying for that shallow Object. This method is used as part of the retry evaluation logic to determine if a final chunk must be retransmitted or not. This method will only return cleanly if the resumable upload returns 200, if a 308 is returned due to an incomplete upload a
StorageException
will be thrown here to signal that fact. In the case ofBlobWriteChannel
which uses this method, the exception would be caught by the retry handler and be evaluated there.I've added a javadoc to the method on there interface with an explanation of why it's there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, the javadoc really helps, thank you.
So, in the 308 case-- would we then (if in
BlobWriteChannel
) make another identical PUT request to query the correct offset to retry at? If so, is there a way to preserve the information from the 308 response here instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of
BlobWriteChannel
it performs a pre-check to try and determine the remote offset, if the remote offset indicates that the data has been written but for some reason it wasn't able to get the object metadata as part of the finalPUT
, it will then use this method to try and get that object metadata.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a 308 happens, that would land us in the normal retry flow, where right now unfortunately we don't have the ability to rewind and retry a chunk to try and recover.
In practice, with
BlobWriteChannel
any call to this new method is guarded by a check to see if the resumable session is complete or not (in fact I would have prefered to make this check an internal implementation detail but unfortunatelyBlobWriteChannel
does not have the means of accessing the http client directly in order to be able to make this call). So, for all intents and purposes we side step the need to be able to rewind a chunk.