feat: add support for return-X-after-YK of downloaded data #274
Conversation
Codecov Report
@@ Coverage Diff @@
## main #274 +/- ##
==========================================
+ Coverage 97.61% 97.63% +0.01%
==========================================
Files 48 48
Lines 7582 7643 +61
==========================================
+ Hits 7401 7462 +61
Misses 181 181
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -242,7 +242,7 @@ curl -H "x-retry-test-id: 1d05c20627844214a9ff7cbcf696317d" "http://localhost:91 | |||
| | Failure Id | Description | |||
| | ----------------------- | --- | |||
| | return-X | Testbench will fail with HTTP code provided for `X`, e.g. return-503 returns a 503 | |||
| | return-X-after-YK | Testbench will return X after YKiB of uploaded data | |||
| | return-broken-stream-final-chunk-after-YB | Testbench will break connection on final chunk of a resumable upload after Y bytes. | |||
| | return-X-after-YK | Testbench will return X after YKiB of uploaded or downloaded data | |||
There was a problem hiding this comment.
nit: return-X-after-YK cannot really work for a download... the error code is returned at the beginning, after the headers but before the payload. Any client will receive a 200 if the download starts successfully.
There was a problem hiding this comment.
I see. I did not think of the fact that the error code is returned before the payload. So return-X-after-YK is unrealistic to happen for downloads in the live server...
I think it "works" here merely because I forcefully trigger the streamer first before returning the response (L694)
# Inject fault outside of the streamer considering execution flow and werkzeug wrappers
try:
response = flask.Response(streamer())
_ = response.data
return response
except testbench.error.RestException as e:
raise e
Thus it "works" when I test it against the python retry conformance test, like so
127.0.0.1 - - [02/Feb/2022:16:50:50 -0800] "GET /download/storage/v1/b/aef6bb4577734e59b6f41c5cdfe501fa/o/4678f04722e54979845541c381a5025e?alt=media HTTP/1.1" 504 97 "-" "python-requests/2.27.1"
127.0.0.1 - - [02/Feb/2022:16:50:51 -0800] "GET /download/storage/v1/b/aef6bb4577734e59b6f41c5cdfe501fa/o/4678f04722e54979845541c381a5025e?alt=media HTTP/1.1" 200 1100000 "-" "python-requests/2.27.1"
I will probably close this FR since it does not match live server behavior, unless we decide it is helpful for testing purposes. Will look into #173 as that seems more realistic. Thanks for the feedback!
| if bytes_downloaded >= limit: | ||
| database.dequeue_next_instruction(test_id, method) | ||
| raise testbench.error.RestException( | ||
| f"Retry Test: Caused a {error_code}. Fault injected after downloading {bytes_downloaded} bytes", |
There was a problem hiding this comment.
For a large buffer, and probably only when using a real TCP/IP connection, the error code here has no effect on the client-side, just FYI.
| @@ -752,6 +783,14 @@ def handle_retry_test_instruction(database, request, method): | |||
| grpc_code=StatusCode.INTERNAL, # not really used | |||
| context=None, | |||
| ) | |||
| # TODO(cathyo@): introduce Retry Test API method "storage.objects.download" for large file downloads | |||
There was a problem hiding this comment.
Please use a GitHub issue numbers to track TODO items, in case you win the lottery and or leave us for greener pa$ture$
| # Use the XML API to inject a larger object and smaller object. | ||
| media = self._create_block(UPLOAD_QUANTUM) | ||
| blob_larger = self.client.put( | ||
| "/bucket-name/256kb.txt", |
There was a problem hiding this comment.
nit: some inconsistency between the filename with kb (Kilobits) and the generated block (256KiB).
Fixes #273
As part of the GCS client library retry alignment project, this adds support in the Retry Test API to
return-X-after-YKof downloaded data.I've noted a TODO entry to introduce a Retry Test API method "storage.objects.download" to further support complex download test cases for larger files.