Feat s3 transfer manager v2 PutObject #2733
Conversation
|
Tests are failing. I know that some of the business logic is duplicated from the original version here, but I'm reviewing it as if it's all new. |
| Close() | ||
| } | ||
|
|
||
| type maxSlicePool struct { |
There was a problem hiding this comment.
See my comment elsewhere about non-necessity of shared pool. I think we can greatly simplify this by just having one fixed pool per upload call.
There was a problem hiding this comment.
You removed the shared-ness of the pool but you didn't really simplify anything:
- The pool sized is now fixed, since it's not shared, it never has to grow or shrink. The entire
ModifyCapacityAPI and business logic to support it should not longer be necessary. - None of the read/write locking here should be necessary. Buffered channels inherently give us synchronization across goroutines, this should be as simple as Get() and Put() calls that pull and push to such a channel.
Frankly I would recommend you start from scratch here. A buffered-channel-based fixed size pool should be trivial to implement in significantly less code vs. inheriting/bastardizing the old version of this.
| } | ||
|
|
||
| // This upload exceeded maximum number of supported parts, error now. | ||
| if part > DefaultMaxUploadParts { |
There was a problem hiding this comment.
This should be MaxUploadParts then instead of just DefaultMax.... But same question as previous, why do we want to limit the number of parts used?
There was a problem hiding this comment.
It appears there is in fact a 10k part limit for MPUs, I didn't catch this at first.
We're going to need to discuss offline how we want to handle this. The error case we're hitting here is unacceptable (failing a 10,000 part MPU only after we realize there are 10k parts).
There was a problem hiding this comment.
We need to seriously re-examine the part pool behavior. It seems entirely overcomplicated for what we need out of it.
We need to discuss how we manage maximum part size of 10k for MPUs - failing on an in-progress download from exceeding the limit after the fact isn't acceptable behavior.
| Close() | ||
| } | ||
|
|
||
| type maxSlicePool struct { |
There was a problem hiding this comment.
You removed the shared-ness of the pool but you didn't really simplify anything:
- The pool sized is now fixed, since it's not shared, it never has to grow or shrink. The entire
ModifyCapacityAPI and business logic to support it should not longer be necessary. - None of the read/write locking here should be necessary. Buffered channels inherently give us synchronization across goroutines, this should be as simple as Get() and Put() calls that pull and push to such a channel.
Frankly I would recommend you start from scratch here. A buffered-channel-based fixed size pool should be trivial to implement in significantly less code vs. inheriting/bastardizing the old version of this.
| } | ||
|
|
||
| // This upload exceeded maximum number of supported parts, error now. | ||
| if part > DefaultMaxUploadParts { |
There was a problem hiding this comment.
It appears there is in fact a 10k part limit for MPUs, I didn't catch this at first.
We're going to need to discuss offline how we want to handle this. The error case we're hitting here is unacceptable (failing a 10,000 part MPU only after we realize there are 10k parts).
Implement v2 s3 transfer manager's putobject api bound to single union client which mimics normal service client's initialization and api call