Repository: pingcap/dumpling
Pull request #252 (2021-03-24 09:38:28)

[release-5.0] *: refine user experience
This is an automated cherry-pick of #245

/assign lichunzhu

ti-chi-bot: [REVIEW NOTIFICATION]

This pull request has not been approved.


To complete the [pull request process](https://book.prow.tidb.io/#/en/workflows/pr), please ask the reviewers in the [list](https://prow.tidb.io/tichi/repos/pingcap/dumpling/pulls/252/owners) to review by filling `/cc @reviewer` in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the [list](https://prow.tidb.io/tichi/repos/pingcap/dumpling/pulls/252/owners) by filling  `/assign @committer` in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found [here](https://prow.tidb.io/command-help?repo=pingcap%2Fdumpling).

<details>

Reviewer can indicate their review by writing `/lgtm` in a comment.
Reviewer can cancel approval by writing `/lgtm cancel` in a comment.
</details>

<!--Review Notification Identifier-->
lance6716: /lgtm
ti-chi-bot: [REVIEW NOTIFICATION]

This pull request has been approved by:

- lance6716




To complete the [pull request process](https://book.prow.tidb.io/#/en/workflows/pr), please ask the reviewers in the [list](https://prow.tidb.io/tichi/repos/pingcap/dumpling/pulls/252/owners) to review by filling `/cc @reviewer` in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the [list](https://prow.tidb.io/tichi/repos/pingcap/dumpling/pulls/252/owners) by filling  `/assign @committer` in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found [here](https://prow.tidb.io/command-help?repo=pingcap%2Fdumpling).

<details>

Reviewer can indicate their review by writing `/lgtm` in a comment.
Reviewer can cancel approval by writing `/lgtm cancel` in a comment.
</details>

<!--Review Notification Identifier-->
lichunzhu: /merge
ti-chi-bot: This pull request has been accepted and is ready to merge. <details>Commit hash: a0b223d13790d22d89c4d658fbafb9278630b938</details>
lance6716: /run-unit-tests
lance6716: /run-all-tests
lichunzhu: /run-all-tests


ti-chi-bot: Merge canceled because a new commit is pushed.
lichunzhu: /merge
ti-chi-bot: This pull request has been accepted and is ready to merge. <details>Commit hash: 5b88182686da0dabbd54591303bbff0c2582ef84</details>

Patch:
diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml
index 09a6caf..224046e 100644
--- a/.github/workflows/go.yml
+++ b/.github/workflows/go.yml
@@ -3,11 +3,11 @@ on:
   push:
     branches:
       - master
-      - 'release-.*'
+      - release-*
   pull_request:
     branches:
       - master
-      - 'release-.*'
+      - release-*
 
 jobs:
   unit-test:
diff --git a/v4/cli/versions.go b/v4/cli/versions.go
index c44c190..5ab75c1 100644
--- a/v4/cli/versions.go
+++ b/v4/cli/versions.go
@@ -15,6 +15,10 @@ package cli
 
 import (
 	"fmt"
+
+	"go.uber.org/zap"
+
+	"github.com/pingcap/dumpling/v4/log"
 )
 
 var (
@@ -45,3 +49,13 @@ func LongVersion() string {
 		GoVersion,
 	)
 }
+
+// LogLongVersion logs the version information of this program to the logger.
+func LogLongVersion(logger log.Logger) {
+	logger.Info("Welcome to dumpling",
+		zap.String("Release Version", ReleaseVersion),
+		zap.String("Git Commit Hash", GitHash),
+		zap.String("Git Branch", GitBranch),
+		zap.String("Build timestamp", BuildTimestamp),
+		zap.String("Go Version", GoVersion))
+}
diff --git a/v4/export/config.go b/v4/export/config.go
index 932e6e4..2548dbe 100644
--- a/v4/export/config.go
+++ b/v4/export/config.go
@@ -155,7 +155,7 @@ func DefaultConfig() *Config {
 		NoViews:            true,
 		Rows:               UnspecifiedSize,
 		Where:              "",
-		FileType:           FileFormatSQLTextString,
+		FileType:           "",
 		NoHeader:           false,
 		NoSchemas:          false,
 		NoData:             false,
@@ -221,12 +221,13 @@ func (conf *Config) DefineFlags(flags *pflag.FlagSet) {
 	flags.Uint64P(flagRows, "r", UnspecifiedSize, "Split table into chunks of this many rows, default unlimited")
 	flags.String(flagWhere, "", "Dump only selected records")
 	flags.Bool(flagEscapeBackslash, true, "use backslash to escape special characters")
-	flags.String(flagFiletype, FileFormatSQLTextString, "The type of export file (sql/csv)")
+	flags.String(flagFiletype, "", "The type of export file (sql/csv)")
 	flags.Bool(flagNoHeader, false, "whether not to dump CSV table header")
 	flags.BoolP(flagNoSchemas, "m", false, "Do not dump table schemas with the data")
 	flags.BoolP(flagNoData, "d", false, "Do not dump table data")
 	flags.String(flagCsvNullValue, "\\N", "The null value used when export to csv")
 	flags.StringP(flagSQL, "S", "", "Dump data with given sql. This argument doesn't support concurrent dump")
+	_ = flags.MarkHidden(flagSQL)
 	flags.StringSliceP(flagFilter, "f", []string{"*.*", DefaultTableFilter}, "filter to select which tables to dump")
 	flags.Bool(flagCaseSensitive, false, "whether the filter should be case-sensitive")
 	flags.Bool(flagDumpEmptyDatabase, true, "whether to dump empty database")
@@ -682,11 +683,22 @@ func validateSpecifiedSQL(conf *Config) error {
 	return nil
 }
 
-func validateFileFormat(conf *Config) error {
+func adjustFileFormat(conf *Config) error {
 	conf.FileType = strings.ToLower(conf.FileType)
 	switch conf.FileType {
-	case FileFormatSQLTextString, FileFormatCSVString:
-		return nil
+	case "":
+		if conf.SQL != "" {
+			conf.FileType = FileFormatCSVString
+		} else {
+			conf.FileType = FileFormatSQLTextString
+		}
+	case FileFormatSQLTextString:
+		if conf.SQL != "" {
+			return errors.Errorf("unsupported config.FileType '%s' when we specify --sql, please unset --filetype or set it to 'csv'", conf.FileType)
+		}
+	case FileFormatCSVString:
+	default:
+		return errors.Errorf("unknown config.FileType '%s'", conf.FileType)
 	}
-	return errors.Errorf("unknown config.FileType '%s'", conf.FileType)
+	return nil
 }
diff --git a/v4/export/dump.go b/v4/export/dump.go
index 50da732..9bfd089 100755
--- a/v4/export/dump.go
+++ b/v4/export/dump.go
@@ -11,6 +11,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/pingcap/dumpling/v4/cli"
 	tcontext "github.com/pingcap/dumpling/v4/context"
 	"github.com/pingcap/dumpling/v4/log"
 
@@ -48,7 +49,7 @@ func NewDumper(ctx context.Context, conf *Config) (*Dumper, error) {
 	err := adjustConfig(conf,
 		registerTLSConfig,
 		validateSpecifiedSQL,
-		validateFileFormat)
+		adjustFileFormat)
 	if err != nil {
 		return nil, err
 	}
@@ -655,6 +656,7 @@ func initLogger(d *Dumper) error {
 		if err != nil {
 			return errors.Trace(err)
 		}
+		cli.LogLongVersion(logger)
 	}
 	d.tctx = d.tctx.WithLogger(logger)
 	return nil


Test patch:
diff --git a/tests/basic/run.sh b/tests/basic/run.sh
index a91f9b2..c160469 100644
--- a/tests/basic/run.sh
+++ b/tests/basic/run.sh
@@ -15,12 +15,17 @@ run_sql "create database \`$DB_NAME\`;"
 run_sql "create table \`$DB_NAME\`.\`$TABLE_NAME\` (a int);"
 run_sql "insert into \`$DB_NAME\`.\`$TABLE_NAME\` values (1), (2);"
 
-run_dumpling -f "$DB_NAME.$TABLE_NAME"
+run_dumpling -f "$DB_NAME.$TABLE_NAME" -L ${DUMPLING_OUTPUT_DIR}/dumpling.log
 
-cnt=`grep -w "(.*)" ${DUMPLING_OUTPUT_DIR}/${DB_NAME}.${TABLE_NAME}.000000000.sql|wc -l`
+cnt=$(grep -w "(.*)" ${DUMPLING_OUTPUT_DIR}/${DB_NAME}.${TABLE_NAME}.000000000.sql|wc -l)
 echo "records count is ${cnt}"
 [ "$cnt" = 2 ]
 
+# make sure that dumpling log contains version infomation
+cnt=$(grep -w "Welcome to dumpling.*Release Version.*Git Commit Hash.*Go Version" ${DUMPLING_OUTPUT_DIR}/dumpling.log|wc -l)
+echo "version info count is ${cnt}"
+[ "$cnt" = 1 ]
+
 # Test for simple WHERE case.
 run_sql "drop database if exists \`$DB_NAME\`;"
 run_sql "create database \`$DB_NAME\`;"
@@ -35,6 +40,15 @@ expected=$(seq 3 9)
 echo "expected ${expected}, actual ${actual}"
 [ "$actual" = "$expected" ]
 
+# Test for specifying --filetype sql with --sql, should report an error
+set +e
+run_dumpling --sql "select * from \`$DB_NAME\`.\`$TABLE_NAME\`" --filetype sql > ${DUMPLING_OUTPUT_DIR}/dumpling.log
+set -e
+
+actual=$(grep -w "unsupported config.FileType 'sql' when we specify --sql, please unset --filetype or set it to 'csv'" ${DUMPLING_OUTPUT_DIR}/dumpling.log|wc -l)
+echo "expected 1 return error when specifying --filetype sql and --sql, actual ${actual}"
+[ "$actual" = 1 ]
+
 export DUMPLING_TEST_PORT=4000
 # Test for --sql option.
 run_sql "drop database if exists \`$DB_NAME\`;"
@@ -43,36 +57,36 @@ run_sql "create sequence \`$DB_NAME\`.\`$SEQUENCE_NAME\` increment by 1;"
 
 run_dumpling --sql "select nextval(\`$DB_NAME\`.\`$SEQUENCE_NAME\`)"
 
-actual=$(grep -w "(.*)[,|;]" ${DUMPLING_OUTPUT_DIR}/result.000000000.sql | cut -c2-2)
+actual=$(sed -n '2p' ${DUMPLING_OUTPUT_DIR}/result.000000000.csv)
 echo "expected 1, actual ${actual}"
 [ "$actual" = 1 ]
 
 run_dumpling --sql "select nextval(\`$DB_NAME\`.\`$SEQUENCE_NAME\`)"
 
-actual=$(grep -w "(.*)[,|;]" ${DUMPLING_OUTPUT_DIR}/result.000000000.sql | cut -c2-2)
+actual=$(sed -n '2p' ${DUMPLING_OUTPUT_DIR}/result.000000000.csv)
 echo "expected 2, actual ${actual}"
 [ "$actual" = 2 ]
 
 # Test for tidb_mem_quota_query configuration
 export GO_FAILPOINTS="github.com/pingcap/dumpling/v4/export/PrintTiDBMemQuotaQuery=1*return"
 run_dumpling > ${DUMPLING_OUTPUT_DIR}/dumpling.log
-actual=`grep -w "tidb_mem_quota_query == 1073741824" ${DUMPLING_OUTPUT_DIR}/dumpling.log|wc -l`
+actual=$(grep -w "tidb_mem_quota_query == 1073741824" ${DUMPLING_OUTPUT_DIR}/dumpling.log|wc -l)
 echo "expected 1, actual ${actual}"
 [ "$actual" = 1 ]
 
 export GO_FAILPOINTS=""
 
-set +e
 # Test for wrong sql causing panic problem: https://github.com/pingcap/dumpling/pull/234#issuecomment-759996695
-run_dumpling --sql "test" >> ${DUMPLING_OUTPUT_DIR}/dumpling.log 2>> ${DUMPLING_OUTPUT_DIR}/dumpling.err
+set +e
+run_dumpling --sql "test" > ${DUMPLING_OUTPUT_DIR}/dumpling.log 2> ${DUMPLING_OUTPUT_DIR}/dumpling.err
 set -e
 
 # check stderr, should not contain panic info
-actual=`grep -w "panic" ${DUMPLING_OUTPUT_DIR}/dumpling.err|wc -l`
+actual=$(grep -w "panic" ${DUMPLING_OUTPUT_DIR}/dumpling.err|wc -l)
 echo "expected panic 0, actual ${actual}"
 [ "$actual" = 0 ]
 
 # check stdout, should contain mysql error log
-actual=`grep -w "Error 1064: You have an error in your SQL syntax" ${DUMPLING_OUTPUT_DIR}/dumpling.log|wc -l`
+actual=$(grep -w "Error 1064: You have an error in your SQL syntax" ${DUMPLING_OUTPUT_DIR}/dumpling.log|wc -l)
 echo "expect contain Error 1064, actual ${actual}"
 [ "$actual" -ge 1 ]
diff --git a/tests/no_table_and_db_name/run.sh b/tests/no_table_and_db_name/run.sh
index 0058e06..b13333d 100644
--- a/tests/no_table_and_db_name/run.sh
+++ b/tests/no_table_and_db_name/run.sh
@@ -30,12 +30,13 @@ assert [ $( ls -lh $DUMPLING_OUTPUT_DIR | grep -e ".csv$" | wc -l ) -eq 10 ]
 # 10 files with header.
 assert [ $( cat $DUMPLING_OUTPUT_DIR/*.csv | wc -l ) -eq $(( 100 + 10 )) ]
 
+# TODO: disable --filetype sql --sql "xxx" until dumpling can product a correct sql file
 # dumping with file size = 311 bytes, actually 10 rows
-run_dumpling -F 311B --filetype sql --sql "select * from $TEST_NAME.t"
+# run_dumpling -F 311B --filetype sql --sql "select * from $TEST_NAME.t"
 
-assert [ $( ls -lh $DUMPLING_OUTPUT_DIR | grep -e ".sql$" | wc -l ) -eq 10 ]
+# assert [ $( ls -lh $DUMPLING_OUTPUT_DIR | grep -e ".sql$" | wc -l ) -eq 10 ]
 
 # 10 files with header.
-assert [ $( cat $DUMPLING_OUTPUT_DIR/*.sql | wc -l ) -eq $(( 100 + 10 * 2 )) ]
+# assert [ $( cat $DUMPLING_OUTPUT_DIR/*.sql | wc -l ) -eq $(( 100 + 10 * 2 )) ]
 
 echo "TEST: [$TEST_NAME] passed."
\ No newline at end of file
diff --git a/v4/export/config_test.go b/v4/export/config_test.go
index d875064..466492d 100644
--- a/v4/export/config_test.go
+++ b/v4/export/config_test.go
@@ -13,7 +13,7 @@ var _ = Suite(&testConfigSuite{})
 type testConfigSuite struct{}
 
 func (s *testConfigSuite) TestCreateExternalStorage(c *C) {
-	mockConfig := DefaultConfig()
+	mockConfig := defaultConfigForTest(c)
 	loc, err := mockConfig.createExternalStorage(context.Background())
 	c.Assert(err, IsNil)
 	c.Assert(loc.URI(), Matches, "file:.*")
diff --git a/v4/export/consistency_test.go b/v4/export/consistency_test.go
index f434449..2f1b883 100644
--- a/v4/export/consistency_test.go
+++ b/v4/export/consistency_test.go
@@ -36,7 +36,7 @@ func (s *testConsistencySuite) TestConsistencyController(c *C) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	tctx := tcontext.Background().WithContext(ctx)
-	conf := DefaultConfig()
+	conf := defaultConfigForTest(c)
 	resultOk := sqlmock.NewResult(0, 1)
 
 	conf.Consistency = consistencyTypeNone
@@ -85,7 +85,7 @@ func (s *testConsistencySuite) TestConsistencyLockControllerRetry(c *C) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	tctx := tcontext.Background().WithContext(ctx)
-	conf := DefaultConfig()
+	conf := defaultConfigForTest(c)
 	resultOk := sqlmock.NewResult(0, 1)
 
 	conf.Consistency = consistencyTypeLock
@@ -106,7 +106,7 @@ func (s *testConsistencySuite) TestConsistencyLockControllerRetry(c *C) {
 }
 
 func (s *testConsistencySuite) TestResolveAutoConsistency(c *C) {
-	conf := DefaultConfig()
+	conf := defaultConfigForTest(c)
 	cases := []struct {
 		serverTp            ServerType
 		resolvedConsistency string
@@ -134,7 +134,7 @@ func (s *testConsistencySuite) TestConsistencyControllerError(c *C) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	tctx := tcontext.Background().WithContext(ctx)
-	conf := DefaultConfig()
+	conf := defaultConfigForTest(c)
 
 	conf.Consistency = "invalid_str"
 	_, err = NewConsistencyController(ctx, conf, db)
diff --git a/v4/export/prepare_test.go b/v4/export/prepare_test.go
index 9c73a4e..2883254 100644
--- a/v4/export/prepare_test.go
+++ b/v4/export/prepare_test.go
@@ -27,7 +27,7 @@ func (s *testPrepareSuite) TestPrepareDumpingDatabases(c *C) {
 		AddRow("db3").
 		AddRow("db5")
 	mock.ExpectQuery("SHOW DATABASES").WillReturnRows(rows)
-	conf := DefaultConfig()
+	conf := defaultConfigForTest(c)
 	conf.Databases = []string{"db1", "db2", "db3"}
 	result, err := prepareDumpingDatabases(conf, conn)
 	c.Assert(err, IsNil)
@@ -113,7 +113,7 @@ func (s *testPrepareSuite) TestListAllTables(c *C) {
 }
 
 func (s *testPrepareSuite) TestConfigValidation(c *C) {
-	conf := DefaultConfig()
+	conf := defaultConfigForTest(c)
 	conf.Where = "id < 5"
 	conf.SQL = "select * from t where id > 3"
 	c.Assert(validateSpecifiedSQL(conf), ErrorMatches, "can't specify both --sql and --where at the same time. Please try to combine them into --sql")
@@ -121,9 +121,19 @@ func (s *testPrepareSuite) TestConfigValidation(c *C) {
 	c.Assert(validateSpecifiedSQL(conf), IsNil)
 
 	conf.FileType = FileFormatSQLTextString
-	c.Assert(validateFileFormat(conf), IsNil)
+	c.Assert(adjustFileFormat(conf), ErrorMatches, ".*please unset --filetype or set it to 'csv'.*")
 	conf.FileType = FileFormatCSVString
-	c.Assert(validateFileFormat(conf), IsNil)
+	c.Assert(adjustFileFormat(conf), IsNil)
+	conf.FileType = ""
+	c.Assert(adjustFileFormat(conf), IsNil)
+	c.Assert(conf.FileType, Equals, FileFormatCSVString)
+	conf.SQL = ""
+	conf.FileType = FileFormatSQLTextString
+	c.Assert(adjustFileFormat(conf), IsNil)
+	conf.FileType = ""
+	c.Assert(adjustFileFormat(conf), IsNil)
+	c.Assert(conf.FileType, Equals, FileFormatSQLTextString)
+
 	conf.FileType = "rand_str"
-	c.Assert(validateFileFormat(conf), ErrorMatches, "unknown config.FileType 'rand_str'")
+	c.Assert(adjustFileFormat(conf), ErrorMatches, "unknown config.FileType 'rand_str'")
 }
diff --git a/v4/export/sql_test.go b/v4/export/sql_test.go
index 28dce05..61597f8 100644
--- a/v4/export/sql_test.go
+++ b/v4/export/sql_test.go
@@ -63,7 +63,7 @@ func (s *testSQLSuite) TestBuildSelectAllQuery(c *C) {
 	conn, err := db.Conn(context.Background())
 	c.Assert(err, IsNil)
 
-	mockConf := DefaultConfig()
+	mockConf := defaultConfigForTest(c)
 	mockConf.SortByPk = true
 
 	// Test TiDB server.
@@ -181,7 +181,7 @@ func (s *testSQLSuite) TestBuildOrderByClause(c *C) {
 	conn, err := db.Conn(context.Background())
 	c.Assert(err, IsNil)
 
-	mockConf := DefaultConfig()
+	mockConf := defaultConfigForTest(c)
 	mockConf.SortByPk = true
 
 	// Test TiDB server.
diff --git a/v4/export/writer_test.go b/v4/export/writer_test.go
index 8f55d46..c4db7d0 100644
--- a/v4/export/writer_test.go
+++ b/v4/export/writer_test.go
@@ -20,6 +20,12 @@ var _ = Suite(&testWriterSuite{})
 
 type testWriterSuite struct{}
 
+func defaultConfigForTest(c *C) *Config {
+	config := DefaultConfig()
+	c.Assert(adjustFileFormat(config), IsNil)
+	return config
+}
+
 func (s *testWriterSuite) newWriter(conf *Config, c *C) *Writer {
 	b, err := storage.ParseBackend(conf.OutputDirPath, &conf.BackendOptions)
 	c.Assert(err, IsNil)
@@ -35,7 +41,7 @@ func (s *testWriterSuite) newWriter(conf *Config, c *C) *Writer {
 func (s *testWriterSuite) TestWriteDatabaseMeta(c *C) {
 	dir := c.MkDir()
 
-	config := DefaultConfig()
+	config := defaultConfigForTest(c)
 	config.OutputDirPath = dir
 
 	writer := s.newWriter(config, c)
@@ -52,7 +58,7 @@ func (s *testWriterSuite) TestWriteDatabaseMeta(c *C) {
 func (s *testWriterSuite) TestWriteTableMeta(c *C) {
 	dir := c.MkDir()
 
-	config := DefaultConfig()
+	config := defaultConfigForTest(c)
 	config.OutputDirPath = dir
 
 	writer := s.newWriter(config, c)
@@ -69,7 +75,7 @@ func (s *testWriterSuite) TestWriteTableMeta(c *C) {
 func (s *testWriterSuite) TestWriteViewMeta(c *C) {
 	dir := c.MkDir()
 
-	config := DefaultConfig()
+	config := defaultConfigForTest(c)
 	config.OutputDirPath = dir
 
 	writer := s.newWriter(config, c)
@@ -97,7 +103,7 @@ func (s *testWriterSuite) TestWriteViewMeta(c *C) {
 func (s *testWriterSuite) TestWriteTableData(c *C) {
 	dir := c.MkDir()
 
-	config := DefaultConfig()
+	config := defaultConfigForTest(c)
 	config.OutputDirPath = dir
 
 	writer := s.newWriter(config, c)
@@ -136,7 +142,7 @@ func (s *testWriterSuite) TestWriteTableData(c *C) {
 func (s *testWriterSuite) TestWriteTableDataWithFileSize(c *C) {
 	dir := c.MkDir()
 
-	config := DefaultConfig()
+	config := defaultConfigForTest(c)
 	config.OutputDirPath = dir
 	config.FileSize = 50
 	specCmts := []string{
@@ -186,7 +192,7 @@ func (s *testWriterSuite) TestWriteTableDataWithFileSize(c *C) {
 func (s *testWriterSuite) TestWriteTableDataWithFileSizeAndRows(c *C) {
 	dir := c.MkDir()
 
-	config := DefaultConfig()
+	config := defaultConfigForTest(c)
 	config.OutputDirPath = dir
 	config.FileSize = 50
 	config.Rows = 4
@@ -237,7 +243,7 @@ func (s *testWriterSuite) TestWriteTableDataWithFileSizeAndRows(c *C) {
 func (s *testWriterSuite) TestWriteTableDataWithStatementSize(c *C) {
 	dir := c.MkDir()
 
-	config := DefaultConfig()
+	config := defaultConfigForTest(c)
 	config.OutputDirPath = dir
 	config.StatementSize = 50
 	config.StatementSize += uint64(len("INSERT INTO `employee` VALUES\n"))