-
Notifications
You must be signed in to change notification settings - Fork 101
Conversation
/rebuild |
1 similar comment
/rebuild |
/rebuild |
1 similar comment
/rebuild |
@@ -56,8 +55,6 @@ func NewBackupCommand() *cobra.Command { | |||
|
|||
// Do not run ddl worker in BR. | |||
ddl.RunWorker = false | |||
// Do not run stat worker in BR. | |||
session.DisableStats4Test() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we disable the stats loop after we have dumped all JSONTables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when we reached here, we have a domain already. so it seems we can not touch the statsLease
any more, https://github.com/pingcap/tidb/blob/e136429d8dc5d70f43cd3f94179b0b9f47595097/session/tidb.go#L68
Co-authored-by: kennytm <kennytm@gmail.com>
/rebuild |
/run-all-tests |
/build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a test involving restoring from an older archive where the stats are missing (nil).
(perhaps add a flag like br backup --stats=false
too to simplify testing)
/rebuild |
pkg/restore/batcher_test.go
Outdated
for _, t := range tables { | ||
log.Info("entering", zap.Int64("table ID", t.Table.ID)) | ||
manager[t.Table.ID] = true | ||
manager.lock.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this lock outsize the for loop
pkg/restore/batcher_test.go
Outdated
for _, t := range tables { | ||
if !manager[t.Table.ID] { | ||
manager.lock.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
for _, t := range tables { | ||
ids = append(ids, t.Table.ID) | ||
} | ||
for id, contains := range manager { | ||
for id, contains := range manager.m { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read option should be protected by lock
Done, PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/rebuild |
/build |
1 similar comment
/build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rest LGTM
LGTM |
/merge |
/run-all-tests |
cherry pick to release-4.0 failed |
What problem does this PR solve?
close #598
What is changed and how it works?
Check List
Tests
Related changes
Release Note