Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

drainer: fix failure to update BIT columns with downstream TiDB when message passes through Kafka #667

Merged
merged 14 commits into from
Jul 13, 2019
Merged

drainer: fix failure to update BIT columns with downstream TiDB when message passes through Kafka #667

merged 14 commits into from
Jul 13, 2019

Conversation

lichunzhu
Copy link
Contributor

What problem does this PR solve?

Issue TOOL-1374, which is caused by pingcap/tidb#10988

What is changed and how it works?

Encode BIT types as integers instead of byte strings. Since the longest value is BIT(64), it is safe to always convert as uint64.

  1. This PR is an improved version of pingcap/binlog PR#655. It mainly puts test in tests/update_bit1/ into tests/dailytest/case.go. This can simplify test code and test both drainer -> mysq and drainer -> kafka -> mysql.
  2. This PR also encodes BIT types as integers in pkg/loader/translate.go to avoid fault in pingcap/tidb#10988.
  3. Change the table name in tests/dailytest/case.go from 'a' to 'binlog_case3' to make it easier for debug.

Check List

Tests

  • Manual test (add detailed scripts or steps below)
    • Verified the error in TOOL-1374 is no longer reproducible.

Code changes

Side effects

Related changes

@lichunzhu
1. move case in update_bit1 to /tests/dailytest/case.go 2. translate …
248b08f 
…[]byte data to uint64 in tranlate.go to avoid issue tidb#10988 3. rename the table a to binlog_case3 to make it easy to check later
@CLAassistant
Copy link

CLAassistant commented Jul 4, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

july2993
july2993 reviewed Jul 4, 2019
View reviewed changes
Copy link
Contributor

@july2993 july2993 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please fix ci, you can run "make check" locally

pkg/loader/translate.go Outdated Show resolved Hide resolved
tests/dailytest/case.go Show resolved Hide resolved
@lichunzhu
Copy link
Contributor Author

/rebuild

@lichunzhu
Copy link
Contributor Author

/run-all-tests

july2993
july2993 reviewed Jul 5, 2019
View reviewed changes
tests/dailytest/case.go Outdated Show resolved Hide resolved
pkg/loader/translate.go Outdated Show resolved Hide resolved
tests/dailytest/case.go Outdated Show resolved Hide resolved
pkg/loader/translate.go Show resolved Hide resolved
@lichunzhu
Copy link
Contributor Author

/run-all-tests

july2993
july2993 reviewed Jul 5, 2019
View reviewed changes
arbiter/server.go Show resolved Hide resolved
pkg/loader/translate.go Outdated Show resolved Hide resolved
pkg/loader/translate.go Outdated Show resolved Hide resolved
pkg/loader/translate.go Outdated Show resolved Hide resolved
tests/kafka/kafka.go Show resolved Hide resolved
@lichunzhu
Copy link
Contributor Author

/run-all-tests

@lichunzhu
Copy link
Contributor Author

/run-all-tests

@lichunzhu
Copy link
Contributor Author

/run-all-tests

@july2993
Copy link
Contributor

/run-all-tests

july2993
july2993 reviewed Jul 10, 2019
View reviewed changes
Copy link
Contributor

@july2993 july2993 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@suzaku PTAL

suzaku
suzaku reviewed Jul 10, 2019
View reviewed changes
arbiter/server.go Show resolved Hide resolved
arbiter/server_test.go Outdated Show resolved Hide resolved
pkg/loader/translate.go Outdated Show resolved Hide resolved
tests/dailytest/case.go Outdated Show resolved Hide resolved
@lichunzhu
Copy link
Contributor Author

All comments have been revised. PTAL. @suzaku

@suzaku
Copy link
Contributor

suzaku commented Jul 13, 2019

/run-all-tests

suzaku
suzaku reviewed Jul 13, 2019
View reviewed changes
Copy link
Contributor

@suzaku suzaku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, LGTM.

@july2993 july2993 merged commit bda8ebd into pingcap:master Jul 13, 2019
This was referenced Aug 12, 2019
Closed
Merged
@lichunzhu lichunzhu mentioned this pull request Sep 4, 2019
Merged
july2993 pushed a commit that referenced this pull request Sep 9, 2019
@lichunzhu @july2993
Refine BIT test & Fix arbiter bug(#667 & #714) (#740)
1c18a1c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@july2993 july2993 july2993 left review comments

@suzaku suzaku suzaku approved these changes

Assignees
No one assigned
Labels
status/LGT1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lichunzhu @CLAassistant @july2993 @suzaku