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
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