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

Fix default tidb-drainer config #793

Merged
merged 4 commits into from
Aug 22, 2019

Conversation

sokada1221
Copy link
Contributor

What problem does this PR solve?

#792

What is changed and how does it work?

Default tidb-drainer config is adjusted to be compatible with the latest drainer config format.
To be more specific:
detect-interval is moved out from [syncer]
db-type value is changed from pb to file
compression is moved out from [syncer] and renamed to compressor

Check List

Tests

  • Manual test (add detailed scripts or steps below)
  1. Deploy tidb-drainer chart out-of-box to AWS
  2. Drainer is successfully deployed and running

Code changes

  • Has Helm charts change

Side effects

  • Breaking backward compatibility

Related changes

  • N/A

Does this PR introduce a user-facing change?:

NONE

@sre-bot
Copy link
Contributor

sre-bot commented Aug 20, 2019

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.

1 similar comment
@sre-bot
Copy link
Contributor

sre-bot commented Aug 20, 2019

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.

@CLAassistant
Copy link

CLAassistant commented Aug 20, 2019

CLA assistant check
All committers have signed the CLA.

@aylei
Copy link
Contributor

aylei commented Aug 20, 2019

/ok-to-test

@aylei aylei self-requested a review August 20, 2019 15:53
@aylei
Copy link
Contributor

aylei commented Aug 20, 2019

/run-e2e-in-kind

aylei
aylei previously approved these changes Aug 20, 2019
Copy link
Contributor

@aylei aylei left a comment

Choose a reason for hiding this comment

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

LGTM

@aylei aylei requested review from tennix and weekface August 20, 2019 15:57
@aylei
Copy link
Contributor

aylei commented Aug 21, 2019

@tennix @weekface PTAL

@aylei
Copy link
Contributor

aylei commented Aug 21, 2019

@WangXiangUSTC @july2993 Could you please take a look?

@aylei aylei requested a review from july2993 August 21, 2019 05:50
weekface
weekface previously approved these changes Aug 21, 2019
@july2993
Copy link
Contributor

july2993 commented Aug 21, 2019

start from 2.1.16, pump/drainer will return error when loading some undefined configs: pingcap/tidb-binlog#708

  • detect-interval should be moved out from [syncer] for all drainer version, i think it's wrongly set here before and will take no any effect for drainer version before 2.1.16
  • compressor means Use the specified compressor to compress payload between pump and drainer, I will suggest set it as empty as default.
  • Syncer.compression is a wrongly used config by the older version, it's removed since v2.1.9
    and change 'pb' to 'file'(but pb is still accepted by drainer for compatibility)

I will suggest compressor as "" default, rest LGTM

@sokada1221 sokada1221 dismissed stale reviews from weekface and aylei via 1afc4d2 August 21, 2019 07:12
@sokada1221
Copy link
Contributor Author

@july2993 Addressed your review comment.

To all reviewers: PTAL again. Thanks.

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

Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

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

LGTM

@aylei
Copy link
Contributor

aylei commented Aug 21, 2019

/run-e2e-in-kind

@aylei
Copy link
Contributor

aylei commented Aug 22, 2019

/run-e2e-test

@aylei
Copy link
Contributor

aylei commented Aug 22, 2019

/run-e2e-test

@aylei aylei merged commit d2d1c75 into pingcap:master Aug 22, 2019
@sokada1221 sokada1221 deleted the fix-default-drainer-config branch August 22, 2019 03:14
aylei added a commit that referenced this pull request Nov 18, 2019
…compatible with the (#1164)

* Fix default tidb-drainer config to be compatible with the latest drainer config format

* Disable payload compression by default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants