fix: update request method of HttpStorageRpc to properly configure offset on requests #1434
Conversation
product-auto-label bot added size: m 
...e-cloud-storage/src/test/java/com/google/cloud/storage/DefaultRetryHandlingBehaviorTest.java Outdated Show resolved Hide resolved
| mediaHttpDownloader.setBytesDownloaded(position); | ||
| mediaHttpDownloader.setDirectDownloadEnabled(true); | ||
| req.executeMediaAndDownloadTo(outputStream); | ||
| return mediaHttpDownloader.getNumBytesDownloaded(); |
There was a problem hiding this comment.
Is this a refactor or is there magical Apiary functionality hidden in this?
There was a problem hiding this comment.
There seems to be magical apiary behavior behind this. The previous code wouldn't set the Range header ever, but the new code does.
I don't fully understand what's the subtle difference between req.executeMedia().download(os) and req.executeMediaAndDownloadTo(os) but the latter results in the Range header being set if position > 0.
There was a problem hiding this comment.
I have since moved away from the previous executeAndDownload method change. This was not properly respecting returnRawInputStream which would break gzip automatic transcoding if you wanted to actually download the zipped bytes. Instead, we are now setting the range header ourselves.
…fset on requests When invoking downloadTo(..., OutputStream) if a retry was attempted the proper byte offset was not being sent in the retried request. Update logic of HttpStorageRpc.read to manually set the range header rather than trying to rely on MediaDownloader to do it along with not automatically decompressing the byte stream. Update ITRetryConformanceTest to run Scenario 8 test cases, which cover resuming a download which could have caught this error sooner. Update StorageException.translate(IOException) to classify `IOException: Premature EOF` as the existing retryable reason `connectionClosedPrematurely`. Add case to DefaultRetryHandlingBehaviorTest to ensure conformance to this categorization. Break downloadTo integration test out into their own class, and separate the multiple scenarios being tested in the same method. Related to #1425
BenWhitehead force-pushed the add-s8-conformance-tests branch from 4c21e20 to 0f2ef8a Compare product-auto-label bot added size: l 
2 participants
When invoking downloadTo(..., OutputStream) if a retry was attempted the proper byte
offset was not being sent in the retried request. Update logic of HttpStorageRpc.read
to manually set the range header rather than trying to rely on MediaDownloader to do
it along with not automatically decompressing the byte stream.
Update ITRetryConformanceTest to run Scenario 8 test cases, which cover resuming a
download which could have caught this error sooner.
Update StorageException.translate(IOException) to classify
IOException: Premature EOFas the existing retryable reason
connectionClosedPrematurely. Add case toDefaultRetryHandlingBehaviorTest to ensure conformance to this categorization.
Break downloadTo integration test out into their own class, and separate
the multiple scenarios being tested in the same method.
Related to #1425