--checksum=false won't work. #223

YuJuncen · 2020-04-03T03:46:29Z

What problem does this PR solve?

The flag --checksum won't work properly, when you specify --checksum=false, you will(probably) receive a error message like "failed to checksum."

What is changed and how it works?

I have modified the RunBackup and RunRestore methods so they won't check checksums when disabled.
But for raw kv, I have not done any change yet.
One more thing, maybe use --skip-checksum instead of --checksum=false will be better, but this will break backward compatibility.

Check List

Tests

Unit test
Integration test
Code changes
Has exported function/method change
Related changes
Need to cherry-pick to the release branch

…flag won't work properly. Some code of backup and restore ignored the flag (a.k.a. config.Checksum), so when checksum is disabled, we will face failure.

…isabled. Some code of backup and restore ignored the flag (a.k.a. config.Checksum), so when checksum is disabled, we will face failure.

Some of log has been lose. They are in ChecksumMatches now.

pkg/backup/client.go

YuJuncen · 2020-04-03T06:59:26Z

/run-all-tests

pkg/backup/client.go

pkg/task/restore.go

…ically. for backup, ChecksumMatches returns error now.

pkg/task/backup.go

… append at its end.

the bug would cause: when multi tables are backup, the metadata contains only one table.

3pointer · 2020-04-07T07:35:51Z

/run-all-tests

3pointer · 2020-04-07T08:01:11Z

/run-all-tests

…be too large.

...to make br_full_index happy!

codecov · 2020-04-07T12:05:21Z

Codecov Report

Merging #223 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #223   +/-   ##
=======================================
  Coverage   72.65%   72.65%           
=======================================
  Files          48       48           
  Lines        5079     5079           
=======================================
  Hits         3690     3690           
  Misses        949      949           
  Partials      440      440

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a838be7...61bc8da. Read the comment docs.

3pointer

Rest LGTM

pkg/task/backup.go

…um-flag

pkg/backup/client.go

kennytm · 2020-04-09T04:40:03Z

pkg/backup/client.go

+
+// CopyMetaFrom copies schema metadata directly from pending backupSchemas, without calculating checksum.
+// use this when user skip the checksum generating.
+func (bc *Client) CopyMetaFrom(backupSchemas *Schemas) {


why do we need a deep copy here? i.e. why running

bc.backupMeta.Schemas = backupSchemas.schemas

isn't sufficient?

I'm not sure whether it's OK to just shallow copy(i.e. will the slice be unexpected changed somewhere?), the original version(schemas.Start() -> schemas.finishTableChecksum() -> client.CompleteMeta()) makes a deep copy of the slice, so I just followed it...

BTW, I think it won't be costly since the number of schema is usually small... and this method is invoked just once per backup...

pkg/restore/client.go

pkg/task/backup.go

Co-Authored-By: kennytm <kennytm@gmail.com>

kennytm

LGTM

* backup,restore: work on progress to fix a bug that causes --checksum flag won't work properly. Some code of backup and restore ignored the flag (a.k.a. config.Checksum), so when checksum is disabled, we will face failure. * backup: backup will report total bytes and kvs when checksums check disabled. Some code of backup and restore ignored the flag (a.k.a. config.Checksum), so when checksum is disabled, we will face failure. * backup: backup will report total bytes and kvs when checksums check disabled. Some code of backup and restore ignored the flag (a.k.a. config.Checksum), so when checksum is disabled, we will face failure. * backup: add log to ChecksumMatches and new version of FastChecksum. Some of log has been lose. They are in ChecksumMatches now. * restore: restore could find non-checksum tables and skip them automatically. for backup, ChecksumMatches returns error now. * misc: add document for Table::NoChecksum. * backup: omit checksum progress bar when user specify `--checksum=false`. * backup: `CopyMetaFrom` overrides original `client.Schemes` instead of append at its end. * backup: refactor about checksum logic, fix a bug. the bug would cause: when multi tables are backup, the metadata contains only one table. * backup: do some lints. * backup,restore: do some refactor so that cyclomatic complexity won't be too large. * misc: don't use underscore on receiver. * backup: print "quick checksum success" message per table. ...to make br_full_index happy! * backup: refactor a MinInt pattern. * backup: Apply suggestions from code review Co-Authored-By: kennytm <kennytm@gmail.com> Co-authored-by: 3pointer <luancheng@pingcap.com> Co-authored-by: kennytm <kennytm@gmail.com>

* enable tidb config by default (#230) * enable tidb config by default * backup,restore: fix --checksum flag. (#223) * backup,restore: work on progress to fix a bug that causes --checksum flag won't work properly. Some code of backup and restore ignored the flag (a.k.a. config.Checksum), so when checksum is disabled, we will face failure. * backup: backup will report total bytes and kvs when checksums check disabled. Some code of backup and restore ignored the flag (a.k.a. config.Checksum), so when checksum is disabled, we will face failure. * backup: backup will report total bytes and kvs when checksums check disabled. Some code of backup and restore ignored the flag (a.k.a. config.Checksum), so when checksum is disabled, we will face failure. * backup: add log to ChecksumMatches and new version of FastChecksum. Some of log has been lose. They are in ChecksumMatches now. * restore: restore could find non-checksum tables and skip them automatically. for backup, ChecksumMatches returns error now. * misc: add document for Table::NoChecksum. * backup: omit checksum progress bar when user specify `--checksum=false`. * backup: `CopyMetaFrom` overrides original `client.Schemes` instead of append at its end. * backup: refactor about checksum logic, fix a bug. the bug would cause: when multi tables are backup, the metadata contains only one table. * backup: do some lints. * backup,restore: do some refactor so that cyclomatic complexity won't be too large. * misc: don't use underscore on receiver. * backup: print "quick checksum success" message per table. ...to make br_full_index happy! * backup: refactor a MinInt pattern. * backup: Apply suggestions from code review Co-Authored-By: kennytm <kennytm@gmail.com> Co-authored-by: 3pointer <luancheng@pingcap.com> Co-authored-by: kennytm <kennytm@gmail.com> * fill size in SSTMeta (#233) * Use table info (#231) * create with info during incremental restore * add test * fix log * address comment * fix ci * address commemnt * update 3.1 dependency * remove conf.Experimental.AllowsExpressionIndex Co-authored-by: 山岚 <36239017+YuJuncen@users.noreply.github.com> Co-authored-by: kennytm <kennytm@gmail.com>

* backup,restore: work on progress to fix a bug that causes --checksum flag won't work properly. Some code of backup and restore ignored the flag (a.k.a. config.Checksum), so when checksum is disabled, we will face failure. * backup: backup will report total bytes and kvs when checksums check disabled. Some code of backup and restore ignored the flag (a.k.a. config.Checksum), so when checksum is disabled, we will face failure. * backup: backup will report total bytes and kvs when checksums check disabled. Some code of backup and restore ignored the flag (a.k.a. config.Checksum), so when checksum is disabled, we will face failure. * backup: add log to ChecksumMatches and new version of FastChecksum. Some of log has been lose. They are in ChecksumMatches now. * restore: restore could find non-checksum tables and skip them automatically. for backup, ChecksumMatches returns error now. * misc: add document for Table::NoChecksum. * backup: omit checksum progress bar when user specify `--checksum=false`. * backup: `CopyMetaFrom` overrides original `client.Schemes` instead of append at its end. * backup: refactor about checksum logic, fix a bug. the bug would cause: when multi tables are backup, the metadata contains only one table. * backup: do some lints. * backup,restore: do some refactor so that cyclomatic complexity won't be too large. * misc: don't use underscore on receiver. * backup: print "quick checksum success" message per table. ...to make br_full_index happy! * backup: refactor a MinInt pattern. * backup: Apply suggestions from code review Co-Authored-By: kennytm <kennytm@gmail.com> Co-authored-by: 3pointer <luancheng@pingcap.com> Co-authored-by: kennytm <kennytm@gmail.com>

* enable tidb config by default (#230) * enable tidb config by default * backup,restore: fix --checksum flag. (#223) * backup,restore: work on progress to fix a bug that causes --checksum flag won't work properly. Some code of backup and restore ignored the flag (a.k.a. config.Checksum), so when checksum is disabled, we will face failure. * backup: backup will report total bytes and kvs when checksums check disabled. Some code of backup and restore ignored the flag (a.k.a. config.Checksum), so when checksum is disabled, we will face failure. * backup: backup will report total bytes and kvs when checksums check disabled. Some code of backup and restore ignored the flag (a.k.a. config.Checksum), so when checksum is disabled, we will face failure. * backup: add log to ChecksumMatches and new version of FastChecksum. Some of log has been lose. They are in ChecksumMatches now. * restore: restore could find non-checksum tables and skip them automatically. for backup, ChecksumMatches returns error now. * misc: add document for Table::NoChecksum. * backup: omit checksum progress bar when user specify `--checksum=false`. * backup: `CopyMetaFrom` overrides original `client.Schemes` instead of append at its end. * backup: refactor about checksum logic, fix a bug. the bug would cause: when multi tables are backup, the metadata contains only one table. * backup: do some lints. * backup,restore: do some refactor so that cyclomatic complexity won't be too large. * misc: don't use underscore on receiver. * backup: print "quick checksum success" message per table. ...to make br_full_index happy! * backup: refactor a MinInt pattern. * backup: Apply suggestions from code review Co-Authored-By: kennytm <kennytm@gmail.com> Co-authored-by: 3pointer <luancheng@pingcap.com> Co-authored-by: kennytm <kennytm@gmail.com> * Use table info (#231) * create with info during incremental restore * add test * fix log * address comment * fix ci * address commemnt * backup: generate backupmeta when backup empty. (#235) * cmd: don't use ':' in the default log file name (#236) * Update build status badge (#239) * pass sse_kms_key_id to S3 (#243) * redirect kvproto Signed-off-by: Yi Wu <yiwu@pingcap.com> * pass sse_kms_key_id Signed-off-by: Yi Wu <yiwu@pingcap.com> * fix hound Signed-off-by: Yi Wu <yiwu@pingcap.com> * update kvproto Signed-off-by: Yi Wu <yiwu@pingcap.com> * go mod tidy Signed-off-by: Yi Wu <yiwu@pingcap.com> Co-authored-by: kennytm <kennytm@gmail.com> * storage: support placing the S3/GCS options into the storage URL (#246) * update 4.0 dependency * go mod tidy * Update README.md Co-Authored-By: Neil Shen <overvenus@gmail.com> Co-authored-by: 山岚 <36239017+YuJuncen@users.noreply.github.com> Co-authored-by: kennytm <kennytm@gmail.com> Co-authored-by: Neil Shen <overvenus@gmail.com> Co-authored-by: yiwu-arbug <yiwu@pingcap.com>

backup,restore: work on progress to fix a bug that causes --checksum …

1c935f2

…flag won't work properly. Some code of backup and restore ignored the flag (a.k.a. config.Checksum), so when checksum is disabled, we will face failure.

sre-bot added the contribution label Apr 3, 2020

YuJuncen changed the title ~~backup,restore: work on progress to fix a bug that causes --checksum …~~ --checksum=false won't work. Apr 3, 2020

3pointer added the WIP label Apr 3, 2020

Hillium added 3 commits April 3, 2020 12:14

backup: backup will report total bytes and kvs when checksums check d…

dc04789

…isabled. Some code of backup and restore ignored the flag (a.k.a. config.Checksum), so when checksum is disabled, we will face failure.

backup: backup will report total bytes and kvs when checksums check d…

0fabef5

…isabled. Some code of backup and restore ignored the flag (a.k.a. config.Checksum), so when checksum is disabled, we will face failure.

backup: add log to ChecksumMatches and new version of FastChecksum.

ee9937a

Some of log has been lose. They are in ChecksumMatches now.

kennytm reviewed Apr 3, 2020

View reviewed changes

pkg/backup/client.go Outdated Show resolved Hide resolved

3pointer reviewed Apr 3, 2020

View reviewed changes

pkg/backup/client.go Show resolved Hide resolved

pkg/task/restore.go Show resolved Hide resolved

Hillium added 2 commits April 3, 2020 16:44

restore: restore could find non-checksum tables and skip them automat…

b43e8c1

…ically. for backup, ChecksumMatches returns error now.

misc: add document for Table::NoChecksum.

180bd8f

3pointer reviewed Apr 3, 2020

View reviewed changes

pkg/task/backup.go Outdated Show resolved Hide resolved

Hillium added 3 commits April 3, 2020 18:08

backup: omit checksum progress bar when user specify --checksum=false.

2557f51

backup: CopyMetaFrom overrides original client.Schemes instead of…

17e1fd6

… append at its end.

backup: refactor about checksum logic, fix a bug.

ef32dc7

the bug would cause: when multi tables are backup, the metadata contains only one table.

3pointer added type/feature-request New feature or request and removed WIP labels Apr 7, 2020

Hillium added 3 commits April 7, 2020 17:31

backup: do some lints.

61ee97e

backup,restore: do some refactor so that cyclomatic complexity won't …

19080f6

…be too large.

misc: don't use underscore on receiver.

5eabf96

kennytm removed the contribution label Apr 7, 2020

backup: print "quick checksum success" message per table.

7f73440

...to make br_full_index happy!

3pointer reviewed Apr 9, 2020

View reviewed changes

pkg/task/backup.go Outdated Show resolved Hide resolved

3pointer and others added 3 commits April 9, 2020 11:48

Merge branch 'master' into fix-checksum-flag

3422cb2

backup: refactor a MinInt pattern.

f8822ff

Merge remote-tracking branch 'mine/fix-checksum-flag' into fix-checks…

d081c60

…um-flag

kennytm reviewed Apr 9, 2020

View reviewed changes

backup: Apply suggestions from code review

61bc8da

Co-Authored-By: kennytm <kennytm@gmail.com>

3pointer added the status/LGT1 LGTM1 label Apr 10, 2020

kennytm approved these changes Apr 10, 2020

View reviewed changes

kennytm added status/LGT2 LGTM2 and removed status/LGT1 LGTM1 labels Apr 10, 2020

YuJuncen merged commit 593c33d into pingcap:master Apr 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

--checksum=false won't work. #223

--checksum=false won't work. #223

YuJuncen commented Apr 3, 2020

YuJuncen commented Apr 3, 2020

3pointer commented Apr 7, 2020

3pointer commented Apr 7, 2020

codecov bot commented Apr 7, 2020 •

edited

Loading

3pointer left a comment

kennytm Apr 9, 2020

YuJuncen Apr 9, 2020

YuJuncen Apr 10, 2020

kennytm left a comment

--checksum=false won't work. #223

--checksum=false won't work. #223

Conversation

YuJuncen commented Apr 3, 2020

What problem does this PR solve?

What is changed and how it works?

Check List

YuJuncen commented Apr 3, 2020

3pointer commented Apr 7, 2020

3pointer commented Apr 7, 2020

codecov bot commented Apr 7, 2020 • edited Loading

Codecov Report

3pointer left a comment

Choose a reason for hiding this comment

kennytm Apr 9, 2020

Choose a reason for hiding this comment

YuJuncen Apr 9, 2020

Choose a reason for hiding this comment

YuJuncen Apr 10, 2020

Choose a reason for hiding this comment

kennytm left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 7, 2020 •

edited

Loading