*: address lints (pingcap#40)

* *: address lints

Signed-off-by: Neil Shen <overvenus@gmail.com>

* MarkPersistentFlagRequired

Signed-off-by: Neil Shen <overvenus@gmail.com>

* fix tests and goimports

Signed-off-by: Neil Shen <overvenus@gmail.com>

* do not require pd and storage

Signed-off-by: Neil Shen <overvenus@gmail.com>

* address comments

Signed-off-by: Neil Shen <overvenus@gmail.com>

* Remove makefile variable

Signed-off-by: Neil Shen <overvenus@gmail.com>

.golangci.yml

@@ -0,0 +1,11 @@
+linters-settings:
+  gocyclo:
+    min-complexity: 40
+
+issues:
+  # Excluding configuration per-path, per-linter, per-text and per-source
+  exclude-rules:
+    - path: _test\.go
+      text: "Potential HTTP request made with variable url"
+      linters:
+        - gosec

Makefile

@@ -42,13 +42,15 @@ check: tools check-all
 static: export GO111MODULE=on
 static:
 	@ # Not running vet and fmt through metalinter becauase it ends up looking at vendor
-	gofmt -s -l $$($(PACKAGE_DIRECTORIES)) 2>&1 | $(GOCHECKER)
+	retool do goimports -w -d -format-only -local $(BR_PKG) $$($(PACKAGE_DIRECTORIES)) 2>&1 | $(GOCHECKER)
 	retool do govet --shadow $$($(PACKAGE_DIRECTORIES)) 2>&1 | $(GOCHECKER)
 
-	CGO_ENABLED=0 retool do golangci-lint run --disable-all --deadline 120s \
-		--enable misspell \
-		--enable staticcheck \
-		--enable ineffassign \
+	CGO_ENABLED=0 retool do golangci-lint run --enable-all --deadline 120s \
+		--disable gochecknoglobals \
+		--disable gochecknoinits \
+		--disable interfacer \
+		--disable goimports \
+		--disable gofmt \
 		$$($(PACKAGE_DIRECTORIES))
 
 lint:

cmd/cmd.go

@@ -5,11 +5,12 @@ import (
 	"net/http"
 	"sync"
 
-	"github.com/pingcap/br/pkg/meta"
 	"github.com/pingcap/errors"
 	"github.com/pingcap/log"
 	"github.com/spf13/cobra"
 	"go.uber.org/zap"
+
+	"github.com/pingcap/br/pkg/meta"
 )
 
 var (
@@ -54,8 +55,6 @@ func AddFlags(cmd *cobra.Command) {
 		"Set the log file path. If not set, logs will output to stdout")
 	cmd.PersistentFlags().String(FlagStatusAddr, "",
 		"Set the HTTP listening address for the status report service. Set to empty string to disable")
-	cmd.MarkFlagRequired(FlagPD)
-	cmd.MarkFlagRequired(FlagStorage)
 }
 
 // Init ...
@@ -100,7 +99,7 @@ func Init(cmd *cobra.Command) (err error) {
 			return
 		}
 	})
-	return
+	return err
 }
 
 // GetDefaultBacker returns the default backer for command line usage.

cmd/meta.go

@@ -6,10 +6,11 @@ import (
 	"encoding/hex"
 
 	"github.com/gogo/protobuf/proto"
-	"github.com/pingcap/br/pkg/utils"
 	"github.com/pingcap/errors"
 	"github.com/pingcap/kvproto/pkg/backup"
 	"github.com/spf13/cobra"
+
+	"github.com/pingcap/br/pkg/utils"
 )
 
 // NewMetaCommand return a meta subcommand.
@@ -25,39 +26,6 @@ func NewMetaCommand() *cobra.Command {
 			return nil
 		},
 	}
-	meta.AddCommand(&cobra.Command{
-		Use:   "version",
-		Short: "show cluster version",
-		RunE: func(cmd *cobra.Command, _ []string) error {
-			backer, err := GetDefaultBacker()
-			if err != nil {
-				return err
-			}
-			v, err := backer.GetClusterVersion()
-			if err != nil {
-				return err
-			}
-			cmd.Println(v)
-			return nil
-		},
-	})
-	meta.AddCommand(&cobra.Command{
-		Use:   "safepoint",
-		Short: "show the current GC safepoint of cluster",
-		RunE: func(cmd *cobra.Command, _ []string) error {
-			backer, err := GetDefaultBacker()
-			if err != nil {
-				return err
-			}
-			sp, err := backer.GetGCSafePoint()
-			if err != nil {
-				return err
-			}
-			cmd.Printf("Timestamp { Physical: %d, Logical: %d }\n",
-				sp.Physical, sp.Logical)
-			return nil
-		},
-	})
 	meta.AddCommand(&cobra.Command{
 		Use:   "checksum",
 		Short: "check the backup data",
@@ -94,7 +62,10 @@ func NewMetaCommand() *cobra.Command {
 				hexBytes := make([]byte, hex.EncodedLen(len(s)))
 				hex.Encode(hexBytes, s[:])
 				if !bytes.Equal(hexBytes, file.Sha256) {
-					return errors.Errorf("backup data checksum failed: %s may be changed\n calculated sha256 is %s\n, origin sha256 is %s", file.Name, s, file.Sha256)
+					return errors.Errorf(`
+backup data checksum failed: %s may be changed
+calculated sha256 is %s,
+origin sha256 is %s`, file.Name, s, file.Sha256)
 				}
 			}
 

cmd/raw.go

@@ -3,16 +3,17 @@ package cmd
 import (
 	"context"
 
-	"github.com/pingcap/br/pkg/raw"
-	"github.com/pingcap/br/pkg/utils"
 	"github.com/pingcap/errors"
 	"github.com/pingcap/log"
 	"github.com/spf13/cobra"
+
+	"github.com/pingcap/br/pkg/raw"
+	"github.com/pingcap/br/pkg/utils"
 )
 
 // NewBackupCommand return a full backup subcommand.
 func NewBackupCommand() *cobra.Command {
-	bp := &cobra.Command{
+	command := &cobra.Command{
 		Use:   "backup",
 		Short: "backup a TiKV cluster",
 		PersistentPreRunE: func(c *cobra.Command, args []string) error {
@@ -23,18 +24,21 @@ func NewBackupCommand() *cobra.Command {
 			return nil
 		},
 	}
-	bp.AddCommand(
+	command.AddCommand(
 		newFullBackupCommand(),
 		newTableBackupCommand(),
 	)
 
-	bp.PersistentFlags().StringP("timeago", "", "", "The history version of the backup task, e.g. 1m, 1h. Do not exceed GCSafePoint")
+	command.PersistentFlags().StringP(
+		"timeago", "", "",
+		"The history version of the backup task, e.g. 1m, 1h. Do not exceed GCSafePoint")
 
-	bp.PersistentFlags().Uint64P(
+	command.PersistentFlags().Uint64P(
 		"ratelimit", "", 0, "The rate limit of the backup task, MB/s per node")
-	bp.PersistentFlags().Uint32P(
+	command.PersistentFlags().Uint32P(
 		"concurrency", "", 4, "The size of thread pool on each node that execute the backup task")
-	return bp
+
+	return command
 }
 
 // newFullBackupCommand return a full backup subcommand.
@@ -238,7 +242,11 @@ func newTableBackupCommand() *cobra.Command {
 	}
 	command.Flags().StringP("db", "", "", "backup a table in the specific db")
 	command.Flags().StringP("table", "t", "", "backup the specific table")
-	command.MarkFlagRequired("db")
-	command.MarkFlagRequired("table")
+	if err := command.MarkFlagRequired("db"); err != nil {
+		panic(err)
+	}
+	if err := command.MarkFlagRequired("table"); err != nil {
+		panic(err)
+	}
 	return command
 }

cmd/restore.go

@@ -122,7 +122,9 @@ func newFullRestoreCommand() *cobra.Command {
 	command.Flags().String("connect", "", "the address to connect tidb, format: username:password@protocol(address)/")
 	command.Flags().Uint("concurrency", 128, "The size of thread pool that execute the restore task")
 
-	command.MarkFlagRequired("connect")
+	if err := command.MarkFlagRequired("connect"); err != nil {
+		panic(err)
+	}
 
 	return command
 }
@@ -213,8 +215,12 @@ func newDbRestoreCommand() *cobra.Command {
 
 	command.Flags().String("db", "", "database name")
 
-	command.MarkFlagRequired("connect")
-	command.MarkFlagRequired("db")
+	if err := command.MarkFlagRequired("connect"); err != nil {
+		panic(err)
+	}
+	if err := command.MarkFlagRequired("db"); err != nil {
+		panic(err)
+	}
 
 	return command
 }
@@ -310,9 +316,15 @@ func newTableRestoreCommand() *cobra.Command {
 	command.Flags().String("db", "", "database name")
 	command.Flags().String("table", "", "table name")
 
-	command.MarkFlagRequired("connect")
-	command.MarkFlagRequired("db")
-	command.MarkFlagRequired("table")
+	if err := command.MarkFlagRequired("connect"); err != nil {
+		panic(err)
+	}
+	if err := command.MarkFlagRequired("db"); err != nil {
+		panic(err)
+	}
+	if err := command.MarkFlagRequired("table"); err != nil {
+		panic(err)
+	}
 
 	return command
 }

cmd/version.go

@@ -1,8 +1,9 @@
 package cmd
 
 import (
-	"github.com/pingcap/br/pkg/utils"
 	"github.com/spf13/cobra"
+
+	"github.com/pingcap/br/pkg/utils"
 )
 
 // NewVersionCommand returns a restore subcommand

pkg/meta/meta.go

@@ -143,8 +143,6 @@ func (backer *Backer) GetClusterVersion() (string, error) {
 
 // GetRegionCount returns the total region count in the cluster
 func (backer *Backer) GetRegionCount() (int, error) {
-	var regionCountPrefix = "pd/api/v1/regions/count"
-
 	var err error
 	for _, addr := range backer.pdHTTP.addrs {
 		v, e := backer.PDHTTPGet(addr, regionCountPrefix, backer.pdHTTP.cli)

pkg/raw/full.go

@@ -155,10 +155,8 @@ func (bc *BackupClient) GetBackupTableRanges(
 	}
 	tableInfo.AutoIncID = globalAutoID
 
-	var tblChecksum, tblKvs, tblBytes uint64
-
 	dbSession.GetSessionVars().SnapshotTS = backupTS
-	tblChecksum, tblKvs, tblBytes, err = bc.getChecksumFromTiDB(dbSession, dbInfo, tableInfo)
+	tblChecksum, err := bc.getChecksumFromTiDB(dbSession, dbInfo, tableInfo)
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
@@ -177,9 +175,9 @@ func (bc *BackupClient) GetBackupTableRanges(
 	backupSchema := &backup.Schema{
 		Db:         dbData,
 		Table:      tableData,
-		Crc64Xor:   tblChecksum,
-		TotalKvs:   tblKvs,
-		TotalBytes: tblBytes,
+		Crc64Xor:   tblChecksum.checksum,
+		TotalKvs:   tblChecksum.totalKvs,
+		TotalBytes: tblChecksum.totalBytes,
 	}
 	log.Info("save table schema",
 		zap.Stringer("db", dbInfo.Name),
@@ -272,9 +270,7 @@ LoadDb:
 		}
 		idAlloc := autoid.NewAllocator(bc.backer.GetTiKV(), dbInfo.ID, false)
 		for _, tableInfo := range dbInfo.Tables {
-			var tblChecksum, tblKvs, tblBytes uint64
-
-			tblChecksum, tblKvs, tblBytes, err = bc.getChecksumFromTiDB(dbSession, dbInfo, tableInfo)
+			tblChecksum, err := bc.getChecksumFromTiDB(dbSession, dbInfo, tableInfo)
 			if err != nil {
 				return nil, errors.Trace(err)
 			}
@@ -292,9 +288,9 @@ LoadDb:
 			backupSchema := &backup.Schema{
 				Db:         dbData,
 				Table:      tableData,
-				Crc64Xor:   tblChecksum,
-				TotalKvs:   tblKvs,
-				TotalBytes: tblBytes,
+				Crc64Xor:   tblChecksum.checksum,
+				TotalKvs:   tblChecksum.totalKvs,
+				TotalBytes: tblChecksum.totalBytes,
 			}
 			log.Info("save table schema",
 				zap.Stringer("db", dbInfo.Name),
@@ -702,19 +698,30 @@ func (bc *BackupClient) FastChecksum() (bool, error) {
 	return true, nil
 }
 
-func (bc *BackupClient) getChecksumFromTiDB(dbSession session.Session, dbl *model.DBInfo, tbl *model.TableInfo) (checksum uint64, totalKvs uint64, totalBytes uint64, err error) {
+type tableChecksum struct {
+	checksum   uint64
+	totalKvs   uint64
+	totalBytes uint64
+}
+
+func (bc *BackupClient) getChecksumFromTiDB(
+	dbSession session.Session,
+	dbl *model.DBInfo,
+	tbl *model.TableInfo,
+) (*tableChecksum, error) {
 	var recordSets []sqlexec.RecordSet
 	// TODO figure out why
 	// must set to true to avoid load global vars, otherwise we got error
 	dbSession.GetSessionVars().CommonGlobalLoaded = true
 
-	recordSets, err = dbSession.Execute(bc.ctx, fmt.Sprintf("ADMIN CHECKSUM TABLE %s.%s", utils.EncloseName(dbl.Name.L), utils.EncloseName(tbl.Name.L)))
+	recordSets, err := dbSession.Execute(bc.ctx, fmt.Sprintf(
+		"ADMIN CHECKSUM TABLE %s.%s", utils.EncloseName(dbl.Name.L), utils.EncloseName(tbl.Name.L)))
 	if err != nil {
-		return 0, 0, 0, errors.Trace(err)
+		return nil, errors.Trace(err)
 	}
 	records, err := utils.ResultSetToStringSlice(bc.ctx, dbSession, recordSets[0])
 	if err != nil {
-		return 0, 0, 0, errors.Trace(err)
+		return nil, errors.Trace(err)
 	}
 	log.Info("tidb table checksum",
 		zap.String("db", dbl.Name.L),
@@ -723,17 +730,21 @@ func (bc *BackupClient) getChecksumFromTiDB(dbSession session.Session, dbl *mode
 	)
 
 	record := records[0]
-	checksum, err = strconv.ParseUint(record[2], 10, 64)
+	checksum, err := strconv.ParseUint(record[2], 10, 64)
 	if err != nil {
-		return 0, 0, 0, errors.Trace(err)
+		return nil, errors.Trace(err)
 	}
-	totalKvs, err = strconv.ParseUint(record[3], 10, 64)
+	totalKvs, err := strconv.ParseUint(record[3], 10, 64)
 	if err != nil {
-		return 0, 0, 0, errors.Trace(err)
+		return nil, errors.Trace(err)
 	}
-	totalBytes, err = strconv.ParseUint(record[4], 10, 64)
+	totalBytes, err := strconv.ParseUint(record[4], 10, 64)
 	if err != nil {
-		return 0, 0, 0, errors.Trace(err)
+		return nil, errors.Trace(err)
 	}
-	return
+	return &tableChecksum{
+		checksum:   checksum,
+		totalKvs:   totalKvs,
+		totalBytes: totalBytes,
+	}, nil
 }

pkg/raw/push.go

@@ -4,12 +4,13 @@ import (
 	"context"
 	"sync"
 
-	"github.com/pingcap/br/pkg/meta"
 	"github.com/pingcap/errors"
 	"github.com/pingcap/kvproto/pkg/backup"
 	"github.com/pingcap/kvproto/pkg/metapb"
 	"github.com/pingcap/log"
 	"go.uber.org/zap"
+
+	"github.com/pingcap/br/pkg/meta"
 )
 
 // pushDown warps a backup task.