Implement GSP-134 Write Behavior Consistency #32
Conversation
| _, err = ap.WriteAppend(o, r, size) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
Do we need to CommitAppend now?
There was a problem hiding this comment.
Yes, you're right. We need CommitAppend here as for dropbox path will be assigned when CommitAppend. If we miss CommitAppend here, there will be no association between the two append operations and the first append object will not be deleted.
appender.go
Outdated
| } | ||
| }() | ||
|
|
||
| Convey("The first returning error should be nil", func() { |
There was a problem hiding this comment.
I think it should be "returned error", or simply "the first error"
appender.go
Outdated
|
|
||
| o, err = ap.CreateAppend(path) | ||
|
|
||
| Convey("The second returning error also should be nil", func() { |
| }) | ||
|
|
||
| Convey("The object append offset should be 0", func() { | ||
| So(o.MustGetAppendOffset(), ShouldBeZeroValue) |
There was a problem hiding this comment.
Is append offset the same as size?
There was a problem hiding this comment.
Yes, the next append offset should match the current object length.
Maybe I need to add the size(content-length) check.
There was a problem hiding this comment.
Does this mean that append offset is redundant?
There was a problem hiding this comment.
I mean we can retain the offset check and add the size check when CreateAppend with an existing appendable object.
There was a problem hiding this comment.
I know what you mean here. Of course yes.
But I was wondering whether we can remove the field if it's not needed.
There was a problem hiding this comment.
Some services require CommitAppend to finish an append process, and until it was committed, the object will not have valid content-length. That's why we introduced GSP-44: Add CommitAppend in Appender.
copier.go
Outdated
| defer func() { | ||
| err = store.Delete(src) | ||
| if err != nil { | ||
| t.Error(err) | ||
| } |
There was a problem hiding this comment.
This part should be moved ahead, otherwise src won't be deleted when dst has an error.
copier.go
Outdated
| defer func() { | ||
| err = store.Delete(src) | ||
| if err != nil { | ||
| t.Error(err) | ||
| } |
mover.go
Outdated
| defer func() { | ||
| err = store.Delete(src) | ||
| if err != nil { | ||
| t.Error(err) | ||
| } |
mover.go
Outdated
| defer func() { | ||
| err = store.Delete(src) | ||
| if err != nil { | ||
| t.Error(err) | ||
| } |
multiparter.go
Outdated
| @@ -22,17 +22,30 @@ func TestMultiparter(t *testing.T, store types.Storager) { | |||
| path := uuid.New().String() | |||
| o, err := m.CreateMultipart(path) | |||
|
|
|||
| defer func() { | |||
| err := store.Delete(path, pairs.WithMultipartID(o.MustGetMultipartID())) | |||
| Convey("The first returning error should be nil", func() { | |||
multiparter.go
Outdated
| Convey("The error should be nil", func() { | ||
| o, err = m.CreateMultipart(path) | ||
|
|
||
| Convey("The second returning error also should be nil", func() { |
storager.go
Outdated
| @@ -88,7 +88,16 @@ func TestStorager(t *testing.T, store types.Storager) { | |||
| } | |||
| }() | |||
|
|
|||
| Convey("The error should be nil", func() { | |||
| Convey("The first returning error should be nil", func() { | |||
storager.go
Outdated
|
|
||
| _, err = store.Write(path, bytes.NewReader(content), secondSize) | ||
|
|
||
| Convey("The second returning error also should be nil", func() { |
No description provided.