Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test case for storage/blob/s3/writer/option #656

Merged
merged 20 commits into from
Sep 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions docs/contributing/coding-style.md
Original file line number Diff line number Diff line change
Expand Up @@ -453,12 +453,20 @@ You can refer to [this section](#Struct-initialization) for more details of the

We strongly recommend the following implementation to set the value using functional option.

If an invalid value is set to the functional option, the `ErrInvalidOption` error defined in the [internal/errors/option.go](https://github.com/vdaas/vald/blob/master/internal/errors/option.go) should be returned.

The name argument (the first argument) of the `ErrInvalidOption` error should be the same as the functional option name without the `With` prefix.

For example, the functional option name `WithVersion` should return the error with the argument `name` as `version`.
Comment on lines +456 to +460
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one point, It would be better to write about option load loop error handling using errors.ErrOptionFailed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your review.
I think so too.
But regarding error handling, I think it is better to add it to the proposal's PR because there is a possibility that it will change when the proposal is applied. 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand about it, but we should write current error handling use case first, after proposal approved you should fix it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not so difficult.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for your opinion.
I will add it to this PR soon



```go
func WithVersion(version string) Option {
return func(c *client) error {
if len(version) != 0 {
c.version = version
if len(version) == 0 {
hlts2 marked this conversation as resolved.
Show resolved Hide resolved
hlts2 marked this conversation as resolved.
Show resolved Hide resolved
return errors.ErrInvalidOption("version", version)
hlts2 marked this conversation as resolved.
Show resolved Hide resolved
hlts2 marked this conversation as resolved.
Show resolved Hide resolved
}
c.version = version
return nil
}
}
Expand All @@ -470,7 +478,7 @@ We recommend the following implementation to parse the time string and set the t
func WithTimeout(dur string) Option {
func(c *client) error {
if dur == "" {
return nil
return errors.ErrInvalidOption("timeout", dur)
hlts2 marked this conversation as resolved.
Show resolved Hide resolved
}
d, err := timeutil.Parse(dur)
if err != nil {
Expand All @@ -488,7 +496,7 @@ We recommend the following implementation to append the value to the slice if th
func WithHosts(hosts ...string) Option {
return func(c *client) error {
if len(hosts) == 0 {
return nil
return errors.ErrInvalidOption("hosts", hosts)
hlts2 marked this conversation as resolved.
Show resolved Hide resolved
}
if c.hosts == nil {
c.hosts = hosts
Expand All @@ -500,6 +508,21 @@ func WithHosts(hosts ...string) Option {
}
```

We recommend the following implementation to apply the options.

If the option failed to apply, an error wrapped with `ErrOptionFailed` defined in the [internal/errors/errors.go](https://github.com/vdaas/vald/blob/master/internal/errors/errors.go) should be returned.

```go
func New(opts ...Option) (Server, error) {
hlts2 marked this conversation as resolved.
Show resolved Hide resolved
hlts2 marked this conversation as resolved.
Show resolved Hide resolved
srv := new(server)
for _, opt := range opts {
hlts2 marked this conversation as resolved.
Show resolved Hide resolved
if err := opt(srv); err != nil {
hlts2 marked this conversation as resolved.
Show resolved Hide resolved
return nil, errors.ErrOptionFailed(err, reflect.ValueOf(opt))
hlts2 marked this conversation as resolved.
Show resolved Hide resolved
hlts2 marked this conversation as resolved.
Show resolved Hide resolved
}
hlts2 marked this conversation as resolved.
Show resolved Hide resolved
}
hlts2 marked this conversation as resolved.
Show resolved Hide resolved
}
```

## Program comments

Program comments make easier to understand the source code. We suggest not to write many comments inside the source code unless the source code is very complicated and confusing; otherwise we should divide the source code into methods to keep the readability and usability of the source code.
Expand Down
53 changes: 40 additions & 13 deletions internal/db/storage/blob/s3/writer/option.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,13 @@ package writer

import (
"github.com/aws/aws-sdk-go/service/s3"
"github.com/aws/aws-sdk-go/service/s3/s3manager"
"github.com/vdaas/vald/internal/errgroup"
"github.com/vdaas/vald/internal/errors"
)

type Option func(w *writer)
// Option represents the functional option for writer.
type Option func(w *writer) error

var (
defaultOpts = []Option{
Expand All @@ -31,44 +34,68 @@ var (
}
)

// WithErrGroup returns the option to set eg for writer.
func WithErrGroup(eg errgroup.Group) Option {
return func(w *writer) {
if eg != nil {
w.eg = eg
return func(w *writer) error {
if eg == nil {
return errors.ErrInvalidOption("errgroup", eg)
}
w.eg = eg
return nil
}
}

// WithService returns the option to set s for writer.
func WithService(s *s3.S3) Option {
return func(w *writer) {
if s != nil {
w.service = s
return func(w *writer) error {
if s == nil {
return errors.ErrInvalidOption("service", s)
}
w.service = s
return nil
}
}

// WithBucket returns the option to set bucket for writer.
func WithBucket(bucket string) Option {
return func(w *writer) {
return func(w *writer) error {
if len(bucket) == 0 {
return errors.ErrInvalidOption("bucket", bucket)
}
w.bucket = bucket
return nil
}
}

// WithKey returns the option to set key for writer.
func WithKey(key string) Option {
return func(w *writer) {
return func(w *writer) error {
if len(key) == 0 {
return errors.ErrInvalidOption("key", key)
}
w.key = key
return nil
}
}

// WithContentType returns the option to set ct for writer.
func WithContentType(ct string) Option {
return func(w *writer) {
if ct != "" {
w.contentType = ct
return func(w *writer) error {
if len(ct) == 0 {
return errors.ErrInvalidOption("contentType", ct)
}
w.contentType = ct
return nil
}
}

// WithMaxPartSize returns the option to set max for writer.
func WithMaxPartSize(max int64) Option {
return func(w *writer) {
return func(w *writer) error {
if max < s3manager.MinUploadPartSize {
return errors.ErrInvalidOption("maxPartSize", max)
}
w.maxPartSize = max
return nil
}
}
Loading