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 cassandra option test #644

Merged
merged 30 commits into from
Sep 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
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
51 changes: 43 additions & 8 deletions docs/contributing/coding-style.md
Original file line number Diff line number Diff line change
Expand Up @@ -451,20 +451,27 @@ The implementation may differ based on your use case.
In Vald, the functional option pattern is widely used in Vald.
You can refer to [this section](#Struct-initialization) for more details of the use case of this pattern.

We provide the following errors to describe the error to apply the option.

| Error | Description |
|----|----|
| errors.ErrInvalidOption | Error to apply the option, and the error is ignorable |
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
| errors.ErrCriticalOption | Critical error to apply the option, the error cannot be ignored and should be handled |
kevindiu marked this conversation as resolved.
Show resolved Hide resolved

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.
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`.

For example, the functional option name `WithVersion` should return the error with the argument `name` as `version`.

```go
func WithVersion(version string) Option {
return func(c *client) error {
if len(version) == 0 {
return errors.ErrInvalidOption("version", version)
return errors.NewErrInvalidOption("version", version)
vankichi marked this conversation as resolved.
Show resolved Hide resolved
}
c.version = version
return nil
Expand All @@ -478,11 +485,11 @@ 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 errors.ErrInvalidOption("timeout", dur)
return errors.NewErrInvalidOption("timeout", dur)
vankichi marked this conversation as resolved.
Show resolved Hide resolved
}
d, err := timeutil.Parse(dur)
if err != nil {
return err
return errors.NewErrInvalidOption("timeout", dur, err)
vankichi marked this conversation as resolved.
Show resolved Hide resolved
}
c.timeout = d
return nil
Expand All @@ -496,7 +503,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 errors.ErrInvalidOption("hosts", hosts)
return errors.NewErrInvalidOption("hosts", hosts)
vankichi marked this conversation as resolved.
Show resolved Hide resolved
}
if c.hosts == nil {
c.hosts = hosts
Expand All @@ -508,16 +515,44 @@ func WithHosts(hosts ...string) Option {
}
```

We recommend the following implementation to apply the options.
If the functional option error is a critical error, we should return `ErrCriticalOption` error instead of `ErrInvalidOption`.

```go
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
func WithConnectTimeout(dur string) Option {
hlts2 marked this conversation as resolved.
Show resolved Hide resolved
return func(c *client) error {
vankichi marked this conversation as resolved.
Show resolved Hide resolved
if dur == "" {
vankichi marked this conversation as resolved.
Show resolved Hide resolved
return errors.NewErrInvalidOption("connectTimeout", dur)
vankichi marked this conversation as resolved.
Show resolved Hide resolved
}
vankichi marked this conversation as resolved.
Show resolved Hide resolved
d, err := timeutil.Parse(dur)
vankichi marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
vankichi marked this conversation as resolved.
Show resolved Hide resolved
return errors.NewErrCriticalOption("connectTimeout", dur, err)
vankichi marked this conversation as resolved.
Show resolved Hide resolved
}
vankichi marked this conversation as resolved.
Show resolved Hide resolved

vankichi marked this conversation as resolved.
Show resolved Hide resolved
c.connectTimeout = d
return nil
}
vankichi marked this conversation as resolved.
Show resolved Hide resolved
}
```

In the caller side, we need to handle the error returned from the functional option.

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.

We recommend the following implementation to apply the options.

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

kevindiu marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

@vankichi vankichi Sep 14, 2020

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is better to keep this line break to differentiate werr and err

e := new(errors.ErrCriticalOption)
Copy link
Contributor

Choose a reason for hiding this comment

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

[LanguageTool] reported by reviewdog 🐶
This sentence does not start with an uppercase letter (UPPERCASE_SENTENCE_START)
Suggestions: E
Rule: https://community.languagetool.org/rule/show/UPPERCASE_SENTENCE_START?lang=en-US
Category: CASING

Copy link
Contributor

Choose a reason for hiding this comment

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

[LanguageTool] reported by reviewdog 🐶
Add a space between sentences (SENTENCE_WHITESPACE)
Suggestions: ErrCriticalOption
Rule: https://community.languagetool.org/rule/show/SENTENCE_WHITESPACE?lang=en-US
Category: TYPOGRAPHY

if errors.As(err, &e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[LanguageTool] reported by reviewdog 🐶
Add a space between sentences (SENTENCE_WHITESPACE)
Suggestions: As
Rule: https://community.languagetool.org/rule/show/SENTENCE_WHITESPACE?lang=en-US
Category: TYPOGRAPHY

Copy link
Contributor

Choose a reason for hiding this comment

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

[LanguageTool] reported by reviewdog 🐶
Don't put a space after the opening parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: {
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY

log.Error(werr)
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
return nil, werr
}
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
log.Warn(werr)
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down
120 changes: 68 additions & 52 deletions internal/db/nosql/cassandra/cassandra.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/scylladb/gocqlx"
"github.com/scylladb/gocqlx/qb"
"github.com/vdaas/vald/internal/errors"
"github.com/vdaas/vald/internal/log"
)

var (
Expand All @@ -50,29 +51,18 @@ type Cassandra interface {
Query(stmt string, names []string) *Queryx
}

type Session = gocql.Session
type Cmp = qb.Cmp
type BatchBuilder = qb.BatchBuilder
type InsertBuilder = qb.InsertBuilder
type DeleteBuilder = qb.DeleteBuilder
type UpdateBuilder = qb.UpdateBuilder
type Queryx = gocqlx.Queryx
type (
Session = gocql.Session
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
Cmp = qb.Cmp
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
BatchBuilder = qb.BatchBuilder
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
InsertBuilder = qb.InsertBuilder
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
DeleteBuilder = qb.DeleteBuilder
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
UpdateBuilder = qb.UpdateBuilder
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
Queryx = gocqlx.Queryx
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
)

type client struct {
hosts []string
cqlVersion string
protoVersion int
timeout time.Duration
connectTimeout time.Duration
port int
keyspace string
numConns int
consistency gocql.Consistency
compressor gocql.Compressor
username string
password string
authProvider func(h *gocql.HostInfo) (gocql.Authenticator, error)
retryPolicy struct {
type (
retryPolicy struct {
numRetries int
minDuration time.Duration
maxDuration time.Duration
Expand All @@ -93,42 +83,68 @@ type client struct {
dcHost string
whiteList []string
}
socketKeepalive time.Duration
maxPreparedStmts int
maxRoutingKeyInfo int
pageSize int
serialConsistency gocql.SerialConsistency
tls *tls.Config
tlsCertPath string
tlsKeyPath string
tlsCAPath string
enableHostVerification bool
defaultTimestamp bool
reconnectInterval time.Duration
maxWaitSchemaAgreement time.Duration
ignorePeerAddr bool
disableInitialHostLookup bool
disableNodeStatusEvents bool
disableTopologyEvents bool
disableSchemaEvents bool
disableSkipMetadata bool
queryObserver QueryObserver
batchObserver BatchObserver
connectObserver ConnectObserver
frameHeaderObserver FrameHeaderObserver
defaultIdempotence bool
dialer gocql.Dialer
writeCoalesceWaitTime time.Duration
client struct {
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
hosts []string
cqlVersion string
protoVersion int
timeout time.Duration
connectTimeout time.Duration
port int
keyspace string
numConns int
consistency gocql.Consistency
compressor gocql.Compressor
username string
password string
authProvider func(h *gocql.HostInfo) (gocql.Authenticator, error)
retryPolicy retryPolicy
reconnectionPolicy reconnectionPolicy
poolConfig poolConfig
hostFilter hostFilter
socketKeepalive time.Duration
maxPreparedStmts int
maxRoutingKeyInfo int
pageSize int
serialConsistency gocql.SerialConsistency
tls *tls.Config
tlsCertPath string
tlsKeyPath string
tlsCAPath string
enableHostVerification bool
defaultTimestamp bool
reconnectInterval time.Duration
maxWaitSchemaAgreement time.Duration
ignorePeerAddr bool
disableInitialHostLookup bool
disableNodeStatusEvents bool
disableTopologyEvents bool
disableSchemaEvents bool
disableSkipMetadata bool
queryObserver QueryObserver
batchObserver BatchObserver
connectObserver ConnectObserver
frameHeaderObserver FrameHeaderObserver
defaultIdempotence bool
dialer gocql.Dialer
writeCoalesceWaitTime time.Duration

cluster *gocql.ClusterConfig
session *gocql.Session
}
cluster *gocql.ClusterConfig
session *gocql.Session
}
)

func New(opts ...Option) (Cassandra, error) {
c := new(client)
for _, opt := range append(defaultOpts, opts...) {
if err := opt(c); err != nil {
return nil, errors.ErrOptionFailed(err, reflect.ValueOf(opt))
werr := errors.ErrOptionFailed(err, reflect.ValueOf(opt))

e := new(errors.ErrCriticalOption)
if errors.As(err, &e) {
log.Error(werr)
return nil, werr
}
log.Warn(werr)
}
}

Expand Down
29 changes: 29 additions & 0 deletions internal/db/nosql/cassandra/cassandra_mock_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
//
// Copyright (C) 2019-2020 Vdaas.org Vald team ( kpango, rinx, kmrmt )
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//
package cassandra

import (
"context"
"net"
)

type DialerMock struct {
DialContextFunc func(ctx context.Context, network, addr string) (net.Conn, error)
}

func (dm *DialerMock) DialContext(ctx context.Context, network, addr string) (net.Conn, error) {
return dm.DialContextFunc(ctx, network, addr)
}
Loading