Repository: pingcap/dumpling
Pull request #245 (2021-03-19 04:16:03)

*: refine user experience
<!--
Thank you for contributing to Dumpling! Please read the [CONTRIBUTING](https://github.com/pingcap/dumpling/blob/master/CONTRIBUTING.md) document **BEFORE** filing this PR.

If you want to open the **Challenge Program** pull request, please use the following template:
https://raw.githubusercontent.com/pingcap/.github/master/.github/PULL_REQUEST_TEMPLATE/challenge-program.md

You can use it with query parameters: https://github.com/pingcap/dumpling/compare/master...${you branch}?template=challenge-program.md
-->

### What problem does this PR solve? <!--add issue link with summary if exists-->
Users may get confused with the SQLs dumped by `--sql` option because it's illegal.

### What is changed and how it works?
1. Mark `--sql` as hidden since it's incompatible with lightning.
2. Add version info in dumpling.log.
3. Adjust `filetype` automatically.

### Check List <!--REMOVE the items that are not applicable-->

Tests <!-- At least one of them must be included. -->

 - Unit test
 - Integration test

Related changes

 - Need to cherry-pick to the release branch
 - Need to update the documentation
 
### Release note

<!-- bugfixes or new feature need a release note, must in the form of a list, such as

- support -T/--tables-list argument

or if no need to be included in the release note, just add the following line

- No release note
-->
- Mark `--sql` as hidden.

lance6716: /lgtm

Patch:
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"))