Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

mydump: added character-set config to control schema decoding behavior #83

Merged
merged 2 commits into from
Nov 29, 2018

Conversation

kennytm
Copy link
Collaborator

@kennytm kennytm commented Nov 15, 2018

What problem does this PR solve?

https://internal.pingcap.net/jira/browse/TOOL-489

Allow the user to choose the encoding of the schema files.

What is changed and how it works?

Added the [mydumper] character-set key (values can be one of: auto, utf8mb4, gb18030 or binary).

Changed the old character auto-detection code to respect this new user setting.

Check List

Tests

  • Integration test

Code changes

  • Has exported function/method change

Side effects

  • Increased code complexity

Related changes

  • Need to update the documentation

@sre-bot
Copy link

sre-bot commented Nov 15, 2018

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@kennytm kennytm force-pushed the kennytm/mixed-encoding branch from ac4b07b to 3ca4246 Compare November 15, 2018 14:30
@kennytm
Copy link
Collaborator Author

kennytm commented Nov 15, 2018

/run-all-tests

@kennytm
Copy link
Collaborator Author

kennytm commented Nov 15, 2018

Lemme DNM it until #82 is merged to get a more reliable test result.

@kennytm kennytm added the status/DNM Do not merge, test is failing or blocked by another PR label Nov 15, 2018
@kennytm kennytm force-pushed the kennytm/mixed-encoding branch from 3ca4246 to c796e4e Compare November 20, 2018 15:19
@kennytm kennytm removed the status/DNM Do not merge, test is failing or blocked by another PR label Nov 20, 2018
@kennytm
Copy link
Collaborator Author

kennytm commented Nov 20, 2018

/run-all-tests

@csuzhangxc
Copy link
Member

LGTM

@kennytm kennytm added the status/LGT1 One reviewer already commented LGTM (LGTM1) label Nov 22, 2018
@IANTHEREAL
Copy link
Collaborator

please resove conflict

@kennytm kennytm force-pushed the kennytm/mixed-encoding branch from c796e4e to 8e7e0d0 Compare November 24, 2018 09:15
@kennytm
Copy link
Collaborator Author

kennytm commented Nov 24, 2018

/run-all-tests

@kennytm
Copy link
Collaborator Author

kennytm commented Nov 24, 2018

@GregoryIan Resolved conflicts

// perform `chardet` first.
fallthrough
case "gb18030":
decoded, err := simplifiedchinese.GB18030.NewDecoder().Bytes(data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

add log convert to what, for unexpected error (not here)

func decodeCharacterSet(data []byte, characterSet string) ([]byte, error) {
switch characterSet {
case "binary":
default:
Copy link
Collaborator

@IANTHEREAL IANTHEREAL Nov 28, 2018

Choose a reason for hiding this comment

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

default auto, how about make it deterministic

@IANTHEREAL
Copy link
Collaborator

Rest LGTM

@kennytm
Copy link
Collaborator Author

kennytm commented Nov 28, 2018

/run-all-tests

@GregoryIan PTAL again

@IANTHEREAL
Copy link
Collaborator

LGTM

@IANTHEREAL IANTHEREAL added status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) and removed status/LGT1 One reviewer already commented LGTM (LGTM1) labels Nov 29, 2018
@kennytm kennytm merged commit d6446d3 into master Nov 29, 2018
@kennytm kennytm deleted the kennytm/mixed-encoding branch November 29, 2018 06:08
@kennytm kennytm added the Should Update Docs Should update docs after this PR is merged. Remove this label once the docs are updated label Dec 4, 2018
@kennytm kennytm removed the Should Update Docs Should update docs after this PR is merged. Remove this label once the docs are updated label Dec 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants