Addition of AWS S3 uploader #1276
Conversation
martingallagher
force-pushed
the
s3-uploader
branch
from
0492674
to
8a11750
Compare
huitseeker
requested review from
m4ksio,
huitseeker,
peterargue,
synzhu and
vishalchangrani
There was a problem hiding this comment.
Overall looks good! However, can we add some tests for the new uploader?
Another suggestion I have (which doesn't necessarily need to be addressed in this PR but perhaps in a future one @m4ksio), is that we should probably modify Upload() method to take in a context, instead of passing a context into the constructors of the uploaders as is done in this PR as well as the existing implementation.
As I've mentioned in various other places before, storing a context inside a struct is an antipattern which is specifically advised against by the developers of Go. Here is an example illustrating some of the problems it can cause.
Furthermore, passing a context to Upload() allows the caller to cancel the upload, which enables things like timeouts. Currently, the code in Manager.ComputeBlock may block indefinitely while waiting for the upload to complete, which is definitely not something we want. (AsyncUploader mitigates this, but at the expense of reduced control over error handling).
| if err != nil { | ||
| return nil, fmt.Errorf("failed to upload block result: %w", err) | ||
| } |
There was a problem hiding this comment.
Nit: may be useful to output information on which specific uploader failed here. Using errgroup obfuscates this information, so I'd suggest maybe not using it and instead manually implementing some logic to capture each individual error from each uploader and creating a multierror out of them.
We explicitly don't want to pass context around, we decided to go with Anyway, as you noted, this PR isn't the best place for this discussion. |
| @@ -199,7 +202,7 @@ func main() { | |||
| collector, | |||
| ) | |||
|
|
|||
| blockDataUploader = asyncUploader | |||
| blockDataUploaders = append(blockDataUploaders, asyncUploader) | |||
There was a problem hiding this comment.
I wonder if maybe instead of slice of uploaders we should create a new uploader which calls multiple ones - MultiUploader of sort.
Codecov Report
@@ Coverage Diff @@
## master #1276 +/- ##
==========================================
+ Coverage 56.25% 56.31% +0.05%
==========================================
Files 500 503 +3
Lines 31162 31230 +68
==========================================
+ Hits 17531 17587 +56
- Misses 11262 11271 +9
- Partials 2369 2372 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
bors merge |
No description provided.