async pause for get #506
async pause for get #506
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #506 +/- ##
==========================================
- Coverage 89.70% 88.97% -0.74%
==========================================
Files 20 20
Lines 6381 6447 +66
==========================================
+ Hits 5724 5736 +12
- Misses 657 711 +54
🚀 New features to boost your workflow:
|
| * - telemetry callback | ||
| * - body callback | ||
| * So that the status collected in the callback is the final status of the meta request. | ||
| * Notes: there is no guarantee that of the service status. |
There was a problem hiding this comment.
Notes: there is no guarantee that of the service status.
I am not sure what this line means.
include/aws/s3/s3_client.h
Outdated
| struct aws_s3_meta_request_resume_token *resume_token); | ||
|
|
||
| /* | ||
| * Upload id associated with operation. |
There was a problem hiding this comment.
Copy paste errors in the docs but ignoring since this is just a draft design review for now.
include/aws/s3/s3_client.h
Outdated
| AWS_S3_API | ||
| struct aws_byte_cursor aws_s3_meta_request_resume_object_last_modified( | ||
| struct aws_s3_meta_request_resume_token *resume_token); |
There was a problem hiding this comment.
Wouldn't an ETAG or versionID be a better option here than last modified time? If someone is trying to resume, they either want the same version or error out. I am not sure if object_last_modified time is supposed to be unique for different versions? What if two uploads complete at the same time?
Another question is that why is the pause token giving out object_modfiied_time from server? We have already delivered this in the on headers callback. This seems like something that doesn't belong in the pause token.
There was a problem hiding this comment.
What's the difference between aws_s3_meta_request_resume_object_last_modified and aws_s3_meta_request_resume_object_last_modified_time?
| .pause = s_s3_auto_ranged_get_pause, | ||
| .pause_async = aws_s3_meta_request_pause_async_default, |
There was a problem hiding this comment.
Having both pause and pause_async seems bad. I think complexity is stemming from trying to not break the existing interface. If Java_SDK is the only user of this interface, then it is probably fine to break this interface internally. If we do break it, can we simplify it like pause is always async or have different types of events callbacks (see third paragraph)?
I also think that for now, the workaround on the Java SDK side to just wait for the on_finish callback seems reasonable and might be less complicated if we want to recommend that they should wait for the on_finish callback in all cases, because in the future we might have code that does something between pause callback and finish callback?
Another option is instead of having a separate callback, just make it another type of progress event. If you pause the meta_request, you will get a final event callback to let you know it was paused, but this doesn't fit into our current design where the single purpose of the progress callback is to just let you know how many bytes are completed.
I'm interested to see what other people think.
3 participants
Issue #, if available:
finishcallback to gather more data from user's side.Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.