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

Add config advertise-addr for drainer #634

Merged
merged 9 commits into from Jun 18, 2019
Merged

Add config advertise-addr for drainer #634

merged 9 commits into from Jun 18, 2019

Conversation

suzaku
Copy link
Contributor

@suzaku suzaku commented Jun 13, 2019

What problem does this PR solve?

Fix #265

What is changed and how it works?

Add advertise-addr config for drainer, the default value is compatible
with older version, which is the same as listen-addr.

Check List

Tests

  • Unit test
  • Integration test

Code changes

Side effects

Related changes

@suzaku suzaku force-pushed the advertise-addr-for-drainer branch from ce56fe1 to 6ef58f4 Compare June 13, 2019 01:58
@suzaku
Copy link
Contributor Author

suzaku commented Jun 13, 2019

/run-all-tests

@suzaku suzaku force-pushed the advertise-addr-for-drainer branch 2 times, most recently from df8f011 to 902be22 Compare June 14, 2019 01:52
@suzaku
Copy link
Contributor Author

suzaku commented Jun 14, 2019

/run-all-tests

kennytm
kennytm reviewed Jun 14, 2019
View reviewed changes
drainer/config.go Outdated Show resolved Hide resolved
drainer/config.go Outdated Show resolved Hide resolved
@suzaku
Copy link
Contributor Author

suzaku commented Jun 14, 2019

/run-all-tests

kennytm
kennytm reviewed Jun 14, 2019
View reviewed changes
drainer/config.go Outdated Show resolved Hide resolved
kennytm
kennytm reviewed Jun 14, 2019
View reviewed changes
Copy link
Contributor

@kennytm kennytm left a 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 showing that, after calling .adjustConfig(), an empty AdvertiseAddr will be filled in with Addr, and an existing AdvertiseAddr will not get overridden.

Rest LGTM.

@suzaku suzaku force-pushed the advertise-addr-for-drainer branch from 73ffe6f to b33fb96 Compare June 17, 2019 01:54
@suzaku
Copy link
Contributor Author

suzaku commented Jun 17, 2019

Please add a test showing that, after calling .adjustConfig(), an empty AdvertiseAddr will be filled in with Addr, and an existing AdvertiseAddr will not get overridden.

Rest LGTM.

It seems like there's already one such test for util.AdjustString here: https://github.com/pingcap/tidb-binlog/blob/master/pkg/util/util_test.go#L207

@suzaku
Copy link
Contributor Author

suzaku commented Jun 17, 2019

/run-all-tests

IANTHEREAL
IANTHEREAL reviewed Jun 17, 2019
View reviewed changes
drainer/config.go Outdated Show resolved Hide resolved
IANTHEREAL
IANTHEREAL reviewed Jun 17, 2019
View reviewed changes
drainer/config.go Outdated Show resolved Hide resolved
suzaku
suzaku commented Jun 17, 2019
View reviewed changes
drainer/config.go Outdated Show resolved Hide resolved
@suzaku
Copy link
Contributor Author

suzaku commented Jun 17, 2019

/run-all-tests

@IANTHEREAL
Copy link
Collaborator

LGTM

@kennytm
Copy link
Contributor

kennytm commented Jun 17, 2019

It seems like there's already one such test for util.AdjustString here: https://github.com/pingcap/tidb-binlog/blob/master/pkg/util/util_test.go#L207

I mean specifically a test for AdvertiseAddr and Addr, as AdjustString working correctly isn't a proof that it's used correctly.

@suzaku
Copy link
Contributor Author

suzaku commented Jun 17, 2019

/run-all-tests

@suzaku
Copy link
Contributor Author

suzaku commented Jun 17, 2019

It seems like there's already one such test for util.AdjustString here: https://github.com/pingcap/tidb-binlog/blob/master/pkg/util/util_test.go#L207

I mean specifically a test for AdvertiseAddr and Addr, as AdjustString working correctly isn't a proof that it's used correctly.

OK, new cases added in TestAdjustConfig.

kennytm
kennytm previously approved these changes Jun 17, 2019
View reviewed changes
Copy link
Contributor

@kennytm kennytm 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
Copy link
Contributor Author

suzaku commented Jun 18, 2019

/run-all-tests

@IANTHEREAL IANTHEREAL merged commit 968b6ac into pingcap:master Jun 18, 2019
@suzaku suzaku deleted the advertise-addr-for-drainer branch June 18, 2019 07:40
suzaku added a commit to suzaku/tidb-binlog that referenced this pull request Jun 21, 2019
@suzaku
Add config advertise-addr for drainer (pingcap#634)
c8ff177
@suzaku suzaku mentioned this pull request Jun 21, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IANTHEREAL IANTHEREAL IANTHEREAL left review comments

@kennytm kennytm kennytm approved these changes

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

Successfully merging this pull request may close these issues.

drainer should add advertise-address config
3 participants
@suzaku @IANTHEREAL @kennytm