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.

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

drainer: remove pb compress config && change 'pb' to 'file' #559

Merged
merged 10 commits into from
Apr 26, 2019

Conversation

WangXiangUSTC
Copy link
Contributor

What problem does this PR solve?

  1. no need add compress config any more
  2. dest type 'pb' seems confused, changed to 'file'
    https://internal.pingcap.net/jira/browse/TOOL-1112

Check List

Tests

  • Unit test
  • Integration test

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation
  • Need to be included in the release note

@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

@@ -197,7 +197,7 @@ func (cfg *Config) Parse(args []string) error {
}

func (c *SyncerConfig) adjustWorkCount() {
if c.DestDBType == "pb" || c.DestDBType == "kafka" {
if c.DestDBType == "pb" || c.DestDBType == "file" || c.DestDBType == "kafka" {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about replacing pb with file in (cfg *Config).adjustConfig and only handle the file type in the rest of the source code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 0768e96, PTAL again

@@ -20,7 +20,7 @@ Usage of drainer:
-data-dir string
drainer data directory path (default data.drainer) (default "data.drainer")
-dest-db-type string
target db type: mysql or pb; see syncer section in conf/drainer.toml (default "mysql")
target db type: mysql or tidb or file or kafka; see syncer section in conf/drainer.toml (default "mysql")
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about file? @suzaku @july2993

Copy link
Contributor

Choose a reason for hiding this comment

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

I think file's OK, better than pb for an end user.

@IANTHEREAL
Copy link
Collaborator

LGTM

drainer/config.go Outdated Show resolved Hide resolved
@suzaku
Copy link
Contributor

suzaku commented Apr 25, 2019

/run-all-tests

@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

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.

rest LGTM

drainer/config.go Outdated Show resolved Hide resolved
@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

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

@july2993 july2993 merged commit 8d39e26 into master Apr 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants