resolve (fix or suppress) all linter warnings (#37)

* resolve (fix or suppress) all linter warnings * wsl is removed from linters list because produces too many noise * several linters are enabled because they are usefull: gosimple, staticcheck, fofmt, nilerr and etc * bugfix: "BucketStat" always returns non-nil err, fixed (found by staticcheck linter), although this function may need additional fixes. * bugfix: "DiscoveryAllBuckets" returns nil even if errGr.Wait() returns err, fixed (found by nilerr) * bugfix: misusage of atomics in "DiscoveryHandleBuckets", fixed (found by govet). This function does not seem correct yet, it would be discussed in another issue * review fixes for PR #37 * update CHANGELOG.md for PR #37
KaymeKaydex · Aug 26, 2024 · ef71933 · ef71933
1 parent 87be7d5
commit ef71933
Show file tree

Hide file tree

Showing 16 changed files with 80 additions and 47 deletions.
diff --git a/.golangci.yaml b/.golangci.yaml
@@ -21,10 +21,22 @@ linters:
     - goconst
     - goimports
     - gosec
+    - gosimple
     - govet
     - ineffassign
     - revive
     - typecheck
     - exportloopref
     - prealloc
-    - wsl
+    # - wls # excluded from linters list because produces too many noise
+    - staticcheck
+    - unused
+    - contextcheck
+    - durationcheck
+    - errname
+    - exhaustive
+    - gocritic
+    - gofmt
+    - nilerr
+    - nilnil
+    - usestdlibvars
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,9 @@ BUG FIXES:
 
 * RouterCallImpl: fix decoding the response from vshard.storage.call
 * RouterCallImpl: do not return nil error when StorageCallAssertError has happened
+* BucketStat: always returns non-nil err, fixed
+* DiscoveryAllBuckets returns nil even if errGr.Wait() returns err, fixed
+* DiscoveryHandleBuckets: misusage of atomics, fixed
 
 FEATURES:
 
@@ -13,6 +16,7 @@ FEATURES:
 REFACTOR:
 
 * Refactored docs (add QuickStart doc) and that library base on vhsard router
+* Several linters are enabled because they are usefull
 
 
 

diff --git a/api.go b/api.go
@@ -1,4 +1,4 @@
-package vshard_router
+package vshard_router //nolint:revive
 
 import (
 	"context"
@@ -64,6 +64,10 @@ type CallOpts struct {
 	Timeout    time.Duration
 }
 
+// revive warns us: time-naming: var CallTimeoutMin is of type time.Duration; don't use unit-specific suffix "Min".
+// But the original lua vshard implementation uses this naming, so we use it too.
+//
+//nolint:revive
 const CallTimeoutMin = time.Second / 2
 
 // RouterCallImpl Perform shard operation function will restart operation

diff --git a/discovery.go b/discovery.go
@@ -1,4 +1,4 @@
-package vshard_router
+package vshard_router //nolint:revive
 
 import (
 	"context"
@@ -127,13 +127,11 @@ func (r *Router) DiscoveryHandleBuckets(ctx context.Context, rs *Replicaset, buc
 			count++
 
 			if oldRs != nil {
-				bc := oldRs.bucketCount
-
 				if _, exists := affected[oldRs]; !exists {
-					affected[oldRs] = int(bc.Load())
+					affected[oldRs] = int(oldRs.bucketCount.Load())
 				}
 
-				oldRs.bucketCount.Store(bc.Load() - 1)
+				oldRs.bucketCount.Add(-1)
 			} else {
 				//                 router.known_bucket_count = router.known_bucket_count + 1
 				r.knownBucketCount.Add(1)
@@ -143,13 +141,13 @@ func (r *Router) DiscoveryHandleBuckets(ctx context.Context, rs *Replicaset, buc
 	}
 
 	if count != rs.bucketCount.Load() {
-		r.cfg.Logger.Info(ctx, fmt.Sprintf("Updated %s buckets: was %d, became %d", rs.info.Name, rs.bucketCount, count))
+		r.cfg.Logger.Info(ctx, fmt.Sprintf("Updated %s buckets: was %d, became %d", rs.info.Name, rs.bucketCount.Load(), count))
 	}
 
 	rs.bucketCount.Store(count)
 
 	for rs, oldBucketCount := range affected {
-		r.log().Info(ctx, fmt.Sprintf("Affected buckets of %s: was %d, became %d", rs.info.Name, oldBucketCount, rs.bucketCount))
+		r.log().Info(ctx, fmt.Sprintf("Affected buckets of %s: was %d, became %d", rs.info.Name, oldBucketCount, rs.bucketCount.Load()))
 	}
 }
 
@@ -218,7 +216,7 @@ func (r *Router) DiscoveryAllBuckets(ctx context.Context) error {
 
 	err := errGr.Wait()
 	if err != nil {
-		return nil
+		return fmt.Errorf("errGr.Wait() err: %w", err)
 	}
 	r.log().Info(ctx, fmt.Sprintf("discovery done since: %s", time.Since(t)))
 

diff --git a/discovery_test.go b/discovery_test.go
@@ -1,4 +1,4 @@
-package vshard_router
+package vshard_router //nolint:revive
 
 import (
 	"sync"

diff --git a/error.go b/error.go
@@ -1,4 +1,4 @@
-package vshard_router
+package vshard_router //nolint:revive
 
 import "fmt"
 

diff --git a/providers.go b/providers.go
@@ -1,4 +1,4 @@
-package vshard_router
+package vshard_router //nolint:revive
 
 import (
 	"context"

diff --git a/providers/static/provider_test.go b/providers/static/provider_test.go
@@ -17,7 +17,7 @@ func TestNewProvider(t *testing.T) {
 		{nil},
 		{make(map[vshardrouter.ReplicasetInfo][]vshardrouter.InstanceInfo)},
 		{map[vshardrouter.ReplicasetInfo][]vshardrouter.InstanceInfo{
-			vshardrouter.ReplicasetInfo{}: {
+			{}: {
 				vshardrouter.InstanceInfo{},
 				vshardrouter.InstanceInfo{},
 			},
@@ -51,7 +51,7 @@ func TestProvider_Validate(t *testing.T) {
 		{
 			Name: "empty name",
 			Source: map[vshardrouter.ReplicasetInfo][]vshardrouter.InstanceInfo{
-				vshardrouter.ReplicasetInfo{}: {
+				{}: {
 					vshardrouter.InstanceInfo{},
 					vshardrouter.InstanceInfo{},
 				},
@@ -61,7 +61,7 @@ func TestProvider_Validate(t *testing.T) {
 		{
 			Name: "no uuid",
 			Source: map[vshardrouter.ReplicasetInfo][]vshardrouter.InstanceInfo{
-				vshardrouter.ReplicasetInfo{Name: "rs_1"}: {
+				{Name: "rs_1"}: {
 					vshardrouter.InstanceInfo{},
 					vshardrouter.InstanceInfo{},
 				},
@@ -71,7 +71,7 @@ func TestProvider_Validate(t *testing.T) {
 		{
 			Name: "valid",
 			Source: map[vshardrouter.ReplicasetInfo][]vshardrouter.InstanceInfo{
-				vshardrouter.ReplicasetInfo{Name: "rs_1", UUID: uuid.New()}: {
+				{Name: "rs_1", UUID: uuid.New()}: {
 					vshardrouter.InstanceInfo{},
 					vshardrouter.InstanceInfo{},
 				},

diff --git a/providers/viper/provider.go b/providers/viper/provider.go
@@ -108,9 +108,7 @@ func (p *Provider) Init(c vshardrouter.TopologyController) error {
 	return c.AddReplicasets(context.TODO(), p.rs)
 }
 
-func (p *Provider) Close() {
-	return
-}
+func (p *Provider) Close() {}
 
 type ClusterInfo struct {
 	ReplicasetUUID string `yaml:"replicaset_uuid" mapstructure:"replicaset_uuid"`

diff --git a/replicaset.go b/replicaset.go
@@ -1,4 +1,4 @@
-package vshard_router
+package vshard_router //nolint:revive
 
 import (
 	"context"
@@ -38,36 +38,49 @@ func (rs *Replicaset) String() string {
 }
 
 func (rs *Replicaset) BucketStat(ctx context.Context, bucketID uint64) (BucketStatInfo, error) {
-	bsInfo := &BucketStatInfo{}
-	bsError := &BucketStatError{}
+	const bucketStatFnc = "vshard.storage.bucket_stat"
 
-	req := tarantool.NewCallRequest("vshard.storage.bucket_stat").
+	var bsInfo BucketStatInfo
+
+	req := tarantool.NewCallRequest(bucketStatFnc).
 		Args([]interface{}{bucketID}).
 		Context(ctx)
 
 	future := rs.conn.Do(req, pool.RO)
 	respData, err := future.Get()
 	if err != nil {
-		return BucketStatInfo{}, err
+		return bsInfo, err
 	}
 
-	var tmp interface{} // todo: fix non-panic crutch
+	if len(respData) < 1 {
+		return bsInfo, fmt.Errorf("respData len is 0 for %s", bucketStatFnc)
+	}
 
 	if respData[0] == nil {
-		err := future.GetTyped(&[]interface{}{tmp, bsError})
-		if err != nil {
-			return BucketStatInfo{}, err
+
+		if len(respData) < 2 {
+			return bsInfo, fmt.Errorf("respData len < 2 when respData[0] is nil for %s", bucketStatFnc)
 		}
-	} else {
-		// fucking key-code 1
-		// todo: fix after https://github.com/tarantool/go-tarantool/issues/368
-		err := mapstructure.Decode(respData[0], bsInfo)
+
+		var tmp interface{} // todo: fix non-panic crutch
+		bsError := &BucketStatError{}
+
+		err := future.GetTyped(&[]interface{}{tmp, bsError})
 		if err != nil {
-			return BucketStatInfo{}, err
+			return bsInfo, err
 		}
+
+		return bsInfo, bsError
+	}
+
+	// A problem with key-code 1
+	// todo: fix after https://github.com/tarantool/go-tarantool/issues/368
+	err = mapstructure.Decode(respData[0], bsInfo)
+	if err != nil {
+		return bsInfo, fmt.Errorf("can't decode bsInfo: %w", err)
 	}
 
-	return *bsInfo, bsError
+	return bsInfo, nil
 }
 
 // ReplicaCall perform function on remote storage

diff --git a/replicaset_test.go b/replicaset_test.go
@@ -1,4 +1,4 @@
-package vshard_router
+package vshard_router //nolint:revive
 
 import (
 	"bytes"

diff --git a/topology.go b/topology.go
@@ -1,4 +1,4 @@
-package vshard_router
+package vshard_router //nolint:revive
 
 import (
 	"context"
@@ -123,7 +123,7 @@ func (c *controller) AddReplicasets(ctx context.Context, replicasets map[Replica
 	return nil
 }
 
-func (c *controller) RemoveReplicaset(ctx context.Context, rsID uuid.UUID) []error {
+func (c *controller) RemoveReplicaset(_ context.Context, rsID uuid.UUID) []error {
 	r := c.r
 
 	rs := r.idToReplicaset[rsID]

diff --git a/topology_test.go b/topology_test.go
@@ -1,4 +1,4 @@
-package vshard_router
+package vshard_router //nolint:revive
 
 import (
 	"context"

diff --git a/vshard.go b/vshard.go
@@ -1,4 +1,4 @@
-package vshard_router
+package vshard_router //nolint:revive
 
 import (
 	"context"
@@ -92,7 +92,7 @@ func (ii InstanceInfo) Validate() error {
 func NewRouter(ctx context.Context, cfg Config) (*Router, error) {
 	var err error
 
-	cfg, err = prepareCfg(ctx, cfg)
+	cfg, err = prepareCfg(cfg)
 	if err != nil {
 		return nil, err
 	}
@@ -184,7 +184,7 @@ func (r *Router) RouteMapClean() {
 	}
 }
 
-func prepareCfg(ctx context.Context, cfg Config) (Config, error) {
+func prepareCfg(cfg Config) (Config, error) {
 	err := validateCfg(cfg)
 	if err != nil {
 		return Config{}, fmt.Errorf("%v: %v", ErrInvalidConfig, err)
@@ -238,8 +238,12 @@ func (r *Router) RouterBucketIDStrCRC32(shardKey string) uint64 {
 	return BucketIDStrCRC32(shardKey, r.cfg.TotalBucketCount)
 }
 
-// RouterBucketIDMPCRC32 is not supported now
-func RouterBucketIDMPCRC32(total uint64, keys ...string) {}
+// RouterBucketIDMPCRC32 is not implemented yet
+func RouterBucketIDMPCRC32(total uint64, keys ...string) {
+	// todo: implement
+	_, _ = total, keys
+	panic("RouterBucketIDMPCRC32 is not implemented yet")
+}
 
 func (r *Router) RouterBucketCount() uint64 {
 	return r.cfg.TotalBucketCount

diff --git a/vshard_shadow_test.go b/vshard_shadow_test.go
@@ -24,7 +24,7 @@ func TestNewRouter_InvalidReplicasetUUID(t *testing.T) {
 
 	router, err := vshard_router.NewRouter(ctx, vshard_router.Config{
 		TopologyProvider: static.NewProvider(map[vshard_router.ReplicasetInfo][]vshard_router.InstanceInfo{
-			vshard_router.ReplicasetInfo{
+			{
 				Name: "123",
 			}: {
 				{Addr: "first.internal:1212"},
@@ -41,7 +41,7 @@ func TestNewRouter_InstanceAddr(t *testing.T) {
 
 	router, err := vshard_router.NewRouter(ctx, vshard_router.Config{
 		TopologyProvider: static.NewProvider(map[vshard_router.ReplicasetInfo][]vshard_router.InstanceInfo{
-			vshard_router.ReplicasetInfo{
+			{
 				Name: "123",
 				UUID: uuid.New(),
 			}: {

diff --git a/vshard_test.go b/vshard_test.go
@@ -1,4 +1,4 @@
-package vshard_router
+package vshard_router //nolint:revive
 
 import (
 	"testing"