api/complete-multipart: fixes and tests.#2719
Conversation
hackintoshrao
force-pushed
the
complete-mul-fix
branch
from
7a77510 to
ba94e73
Compare
Current coverage is 65.67% (diff: 100%)@@ master #2719 diff @@
==========================================
Files 132 132
Lines 15289 15265 -24
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 10007 10025 +18
+ Misses 4567 4529 -38
+ Partials 715 711 -4
|
krishnasrinivas
left a comment
krishnasrinivas
left a comment
There was a problem hiding this comment.
@hackintoshrao @harshavardhana the complication for completemultipart upload is arising because we have to accomodate the sendwhitespace() function. sendwhitespace() is not useful for XL, it was written because of slow concat process of FS. But since we now do append in background as parts come, FS's completemultipart() is also quick now. That means we can just remove sendwhitespace() functionality and avoid all complications.
Downside is we will not send whitespace chars for the slow concat fall back in FS. But it's a compromise we can make to keep code simple. Because anyhow we saw cloudberry timeout even if we sent white space chars. So for well behaved client uploads we work just fine.
I agree that is what i was leaning towards, we are okay to not do that since sending whitespace character is a S3 spec and as we have seen with experience by now the doc SPEC is not the direct representation of their implementation. Let's remove the go-routine at the top and we can avoid all the complications that came with it. |
| import "net/http" | ||
|
|
||
| // Represents additional fields necessary for ErrPartTooSmall S3 error. | ||
| type completeMultipartAPIError struct { |
There was a problem hiding this comment.
Do not introduce these custom errors just yet, i am fixing this differently. For now do not validate other fields other than the common ones that occur in APIErrorResponse
There was a problem hiding this comment.
OK, for now I'll make changes to send usual error response.
cmd/object-handlers.go
Outdated
| doneCh <- struct{}{} | ||
| }(doneCh) | ||
|
|
||
| sendWhiteSpaceChars(w, doneCh) |
There was a problem hiding this comment.
Let's remove this and we will solve all the problems and we will not need to change the function signature of CompleteMultipartUpload
There was a problem hiding this comment.
So I'll keep the CompleteMultipart API signature as it was and remove the sending of white space characters and running complete multipart in a new routine.
hackintoshrao
force-pushed
the
complete-mul-fix
branch
7 times, most recently
from
db7f7c3 to
e71adf8
Compare
- Removing the logic of sending white space characters. - Fix for incorrect HTTP response status for certain cases. - Tests for New Multipart Upload and Complete Multipart Upload.
hackintoshrao
force-pushed
the
complete-mul-fix
branch
from
e71adf8 to
22da31b
Compare
4 participants
With detailed discussion with @krishnasrinivas , we found out this approach to be the simplest amongst the other alternatives.