Implement appender support #15
Conversation
|
Please take https://github.com/aos-dev/go-service-fs/blob/master/tests/storage_test.go#L17-L22 a look, run an integration test for oss. |
storage.go
Outdated
| rp := s.getAbsPath(path) | ||
|
|
||
| var nextPos int64 = 0 | ||
| isExist, err := s.bucket.IsObjectExist(rp) |
There was a problem hiding this comment.
We don't need to check if this object exists or not, just return error that we got.
storage.go
Outdated
| } | ||
|
|
||
| if isExist { | ||
| props, errGetMeta := s.bucket.GetObjectDetailedMeta(rp) |
There was a problem hiding this comment.
In createAppend, we can use AppendObject with position 0 and size 0 to create an Object.
There was a problem hiding this comment.
Does go-service-oss need to support append to an existing object?
There was a problem hiding this comment.
Yes, but not in this way.
CreateAppend needs to create an append object and should not do other jobs.
In order to append to an existing object, use can:
- Use
Statto get an append object. - Use
Createto create an append object if they know the offset.
In both way, the object has ModeAppend, so they can used in WriteAppend to append new content.
storage.go
Outdated
|
|
||
| func (s *Storage) writeAppend(ctx context.Context, o *Object, r io.Reader, size int64, opt pairStorageWriteAppend) (n int64, err error) { | ||
| rp := o.GetID() | ||
| nextPos := o.MustGetAppendOffset() |
There was a problem hiding this comment.
MustGetAppendOffset will panic if append offset does not exist.
In production code, we usually offset, ok := o.GetAppendOffset() and check the returning value to decide whether to return an error or not.
There was a problem hiding this comment.
Which error is suitable when the append offset is unset? Do I need to definite error code?
There was a problem hiding this comment.
For now, let's just use fmt.Errorf("append offset is not set"). We will add this error in go-storage.
Makefile
Outdated
| @@ -32,7 +32,7 @@ test: | |||
| go tool cover -html="coverage.txt" -o "coverage.html" | |||
|
|
|||
| integration_test: | |||
| go test -count=1 -race -covermode=atomic -v ./tests | |||
| STORAGE_OSS_INTEGRATION_TEST=on go test -count=1 -race -covermode=atomic -v ./tests | |||
There was a problem hiding this comment.
We don't need to set STORAGE_OSS_INTEGRATION_TEST=on here, we will use Makefile.env
storage.go
Outdated
| options := make([]oss.Option, 0) | ||
| options = append(options, oss.ContentLength(0)) | ||
|
|
||
| offset, err := s.bucket.AppendObject(rp, strings.NewReader(""), 0, options...) |
There was a problem hiding this comment.
Can we use nil instead of strings.NewReader("") here?
There was a problem hiding this comment.
Maybe the GetReaderLen() in https://github.com/aliyun/aliyun-oss-go-sdk/blob/master/oss/utils.go#L438 will return error if we use nil?
There was a problem hiding this comment.
Let's give it a try. Our integration tests will cover it.
storage.go
Outdated
| } | ||
|
|
||
| o = s.newObject(true) | ||
| o = s.newObject(false) |
There was a problem hiding this comment.
We should create this object with true, as there is no more info we cloud provide.
|
Other LGTM, please don't forget to update the docs and examples |
Implement appender support in ref: beyondstorage/go-storage#529.