backup: support explicitly set sst compression type #404

glorv · 2020-07-07T11:47:11Z

What problem does this PR solve?

Add command line parameter for sst file compression type

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Code changes

Side effects

Related changes

Release Note

CLAassistant · 2020-07-07T11:47:17Z

All committers have signed the CLA.

kennytm

rest LGTM

kennytm · 2020-07-07T14:08:55Z

pkg/task/backup.go

@@ -60,6 +62,7 @@ func DefineBackupFlags(flags *pflag.FlagSet) {
 	flags.String(flagBackupTS, "", "the backup ts support TSO or datetime,"+
 		" e.g. '400036290571534337', '2018-05-11 01:42:23'")
 	flags.Int64(flagGCTTL, backup.DefaultBRGCSafePointTTL, "the TTL (in seconds) that PD holds for BR's GC safepoint")
+	flags.String(flagCompressionType, "", "backup sst file compression algorithm, value can be one of 'lz4|zstd|snappy'")


provide a default value?

Suggested change

flags.String(flagCompressionType, "", "backup sst file compression algorithm, value can be one of 'lz4|zstd|snappy'")

flags.String(flagCompressionType, "zstd", "backup sst file compression algorithm, value can be one of 'lz4|zstd|snappy'")

the default value should be default, which means the compression type will be determined by tikv. In currently logic, tikv will choose the fastest algorithm and in most case it will be 'lz4'.

the default is called "unknown" though

e, so we should change the name in kv-proto to default?

We should provide a validate default value, "zstd" looks good to me.

Ok. Then I think we can set default to lz4 because it's the current compression type tikv uses. So if user don't set this flag, the behavior is same with previous version, so it's compatible will old version

kennytm · 2020-07-07T14:09:12Z

pkg/task/backup.go

+	case "":
+		ct = kvproto.CompressionType_UNKNOWN


Suggested change

case "":

ct = kvproto.CompressionType_UNKNOWN

overvenus · 2020-07-08T05:48:37Z

pkg/task/backup.go

+	flagBackupTimeago   = "timeago"
+	flagBackupTS        = "backupts"
+	flagLastBackupTS    = "lastbackupts"
+	flagCompressionType = "compression-type"


Suggested change

flagCompressionType = "compression-type"

flagCompressionType = "compression"

overvenus · 2020-07-08T05:48:40Z

pkg/task/backup.go

@@ -60,6 +62,7 @@ func DefineBackupFlags(flags *pflag.FlagSet) {
 	flags.String(flagBackupTS, "", "the backup ts support TSO or datetime,"+
 		" e.g. '400036290571534337', '2018-05-11 01:42:23'")
 	flags.Int64(flagGCTTL, backup.DefaultBRGCSafePointTTL, "the TTL (in seconds) that PD holds for BR's GC safepoint")
+	flags.String(flagCompressionType, "", "backup sst file compression algorithm, value can be one of 'lz4|zstd|snappy'")


We should provide a validate default value, "zstd" looks good to me.

kennytm

LGTM

ti-srebot · 2020-07-08T10:03:41Z

@kennytm,Thanks for your review.

…of better performance

overvenus

LGTM

ti-srebot · 2020-07-09T02:51:06Z

@overvenus,Thanks for your review.

3pointer · 2020-07-09T03:26:21Z

maybe we should add a integration test for "zstd"

ti-srebot · 2020-07-15T05:24:09Z

cherry pick to release-4.0 failed

* add compression type * fix * apply compresssion * fix lint * fix restore flag * provide default value for flag compression * change default backup compression type from 'lz4' to 'zstd' in favor of better performance * fix a log bug

* backup: support explicitly set sst compression type (#404) * add compression type * fix * apply compresssion * fix lint * fix restore flag * provide default value for flag compression * change default backup compression type from 'lz4' to 'zstd' in favor of better performance * fix a log bug * Fix the problem that backup time may be negative (#405) * backup: make sure backup time greater than 0 * backup: rename some variables * backup: make CI happy * glue: set no register itself to dashboard (#421) * glue: set no register itself to dashboard * glue: set no register when initlizating gluetidb * go.mod: update tidb to latest release-4.0 * *: update TiDB version * *: update tidb to 4.0 confirmed hash * glue: add a todo in a place that inconsistant to master * glue: set query DDL when executing DDLs (#406) * glue: set query DDL when executing DDLs * glue: fix some linting * *: update TiDB version * *: run go mod tidy * Update tests/br_db/run.sh Co-authored-by: kennytm <kennytm@gmail.com> * glue: use double quote in comment * tests: use leaser DDL grep regex * tests: use from(br) as comment Co-authored-by: kennytm <kennytm@gmail.com> * *: run go mod tidy Co-authored-by: glorv <glorvs@163.com> Co-authored-by: kennytm <kennytm@gmail.com>

glorv added 4 commits July 3, 2020 16:45

add compression type

67d0793

fix

3591bb1

apply compresssion

5cdd6d0

Merge branch 'master' of https://github.com/pingcap/br into compression

3669199

glorv requested review from 3pointer and kennytm July 7, 2020 11:47

fix lint

28003a7

kennytm reviewed Jul 7, 2020

View reviewed changes

fix restore flag

c02c5e8

overvenus reviewed Jul 8, 2020

View reviewed changes

provide default value for flag compression

02ef270

kennytm reviewed Jul 8, 2020

View reviewed changes

ti-srebot added the status/LGT1 LGTM1 label Jul 8, 2020

glorv added 3 commits July 8, 2020 19:22

Merge branch 'master' into compression

0b6bb3e

change default backup compression type from 'lz4' to 'zstd' in favor …

60a5bef

…of better performance

fix a log bug

550b868

overvenus approved these changes Jul 9, 2020

View reviewed changes

ti-srebot removed the status/LGT1 LGTM1 label Jul 9, 2020

ti-srebot approved these changes Jul 9, 2020

View reviewed changes

ti-srebot added the status/LGT2 LGTM2 label Jul 9, 2020

kennytm merged commit de452e1 into pingcap:master Jul 9, 2020

YuJuncen added the needs-cherry-pick-release-4.0 label Jul 15, 2020

kennytm mentioned this pull request Jul 29, 2020

Support different compression algorithm in BR #395

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backup: support explicitly set sst compression type #404

backup: support explicitly set sst compression type #404

glorv commented Jul 7, 2020

CLAassistant commented Jul 7, 2020 •

edited

Loading

kennytm left a comment

kennytm Jul 7, 2020

glorv Jul 8, 2020

kennytm Jul 8, 2020

glorv Jul 8, 2020

overvenus Jul 8, 2020

glorv Jul 8, 2020

kennytm Jul 7, 2020

overvenus Jul 8, 2020

overvenus Jul 8, 2020

kennytm left a comment •

edited

Loading

ti-srebot commented Jul 8, 2020

overvenus left a comment

ti-srebot commented Jul 9, 2020

3pointer commented Jul 9, 2020

ti-srebot commented Jul 15, 2020

	flags.String(flagCompressionType, "", "backup sst file compression algorithm, value can be one of 'lz4\|zstd\|snappy'")
	flags.String(flagCompressionType, "zstd", "backup sst file compression algorithm, value can be one of 'lz4\|zstd\|snappy'")

	flagCompressionType = "compression-type"
	flagCompressionType = "compression"

backup: support explicitly set sst compression type #404

backup: support explicitly set sst compression type #404

Conversation

glorv commented Jul 7, 2020

What problem does this PR solve?

What is changed and how it works?

Check List

Release Note

CLAassistant commented Jul 7, 2020 • edited Loading

kennytm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennytm left a comment • edited Loading

Choose a reason for hiding this comment

ti-srebot commented Jul 8, 2020

overvenus left a comment

Choose a reason for hiding this comment

ti-srebot commented Jul 9, 2020

3pointer commented Jul 9, 2020

ti-srebot commented Jul 15, 2020

CLAassistant commented Jul 7, 2020 •

edited

Loading

kennytm left a comment •

edited

Loading