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

Sync pump status to tidb-cluster. #1292

Merged
merged 27 commits into from
Dec 21, 2019
Merged

Sync pump status to tidb-cluster. #1292

merged 27 commits into from
Dec 21, 2019

Conversation

shonge
Copy link
Member

@shonge shonge commented Dec 5, 2019

What problem does this PR solve?

#1271

What is changed and how does it work?

Add sync logic to pump_member_manager

Check List

Tests

  • Unit test
  • E2E test
  • Stability test

Code changes

  • Has Go code change

Side effects

Related changes

Does this PR introduce a user-facing change?:

Support sync pump status to tidb-cluster.

@shonge
Copy link
Member Author

shonge commented Dec 5, 2019

@aylei PTAL

aylei
aylei reviewed Dec 5, 2019
View reviewed changes
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.

An unit test case should be added to verify status syncing

pkg/manager/member/pump_member_manager.go Outdated Show resolved Hide resolved
pkg/manager/member/pump_member_manager.go Show resolved Hide resolved
@aylei
Copy link
Contributor

aylei commented Dec 5, 2019

@shonge Thanks! I've left some comments, feel free to discuss with us

@shonge shonge changed the title Sync pump status to tidb-cluster. [DNM] Sync pump status to tidb-cluster. Dec 10, 2019
@shonge shonge changed the title [DNM] Sync pump status to tidb-cluster. [WIP] Sync pump status to tidb-cluster. Dec 11, 2019
aylei
aylei reviewed Dec 12, 2019
View reviewed changes
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.

Rest LGTM

pkg/manager/member/pump_member_manager_test.go Outdated Show resolved Hide resolved
pkg/manager/member/pump_member_manager.go Outdated Show resolved Hide resolved
@shonge shonge changed the title [WIP] Sync pump status to tidb-cluster. Sync pump status to tidb-cluster. Dec 12, 2019
@aylei
Copy link
Contributor

aylei commented Dec 13, 2019

/ok-to-test

@aylei
Copy link
Contributor

aylei commented Dec 13, 2019

/run-e2e-test

@aylei
Copy link
Contributor

aylei commented Dec 13, 2019

@shonge Please fix the lint error

weekface
weekface reviewed Dec 13, 2019
View reviewed changes
Copy link
Contributor

@weekface weekface 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

pkg/apis/pingcap/v1alpha1/types.go Outdated Show resolved Hide resolved
@aylei
Copy link
Contributor

aylei commented Dec 16, 2019

/run-e2e-test

@shonge
Copy link
Member Author

shonge commented Dec 17, 2019

/run-e2e-test

aylei
aylei previously approved these changes Dec 18, 2019
View reviewed changes
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
Copy link
Contributor

aylei commented Dec 18, 2019

/run-e2e-test

@aylei aylei requested review from weekface and cofyc December 18, 2019 07:23
@aylei
Copy link
Contributor

aylei commented Dec 18, 2019

@weekface @tennix @cofyc PTAL

cofyc
cofyc previously approved these changes Dec 18, 2019
View reviewed changes
Copy link
Contributor

@cofyc cofyc left a comment

Choose a reason for hiding this comment

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

LGTM

tennix
tennix reviewed Dec 18, 2019
View reviewed changes
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.

Rest LGTM

@shonge shonge dismissed stale reviews from cofyc and aylei via 5486edd December 19, 2019 02:07
@shonge
Copy link
Member Author

shonge commented Dec 19, 2019

/run-e2e-test

tennix
tennix approved these changes Dec 19, 2019
View reviewed changes
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
aylei approved these changes Dec 19, 2019
View reviewed changes
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
Copy link
Contributor

aylei commented Dec 20, 2019

/run-e2e-test

@aylei
Copy link
Contributor

aylei commented Dec 20, 2019

/run-e2e-test

@aylei
Copy link
Contributor

aylei commented Dec 21, 2019

/merge

@claassistantio
Copy link

claassistantio commented Dec 21, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@sre-bot
Copy link
Contributor

sre-bot commented Dec 21, 2019

/run-all-tests

@aylei
Copy link
Contributor

aylei commented Dec 21, 2019

Seems a problem of CLA, force merging

@aylei aylei merged commit 90ead3d into pingcap:master Dec 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tennix tennix tennix approved these changes

@aylei aylei aylei approved these changes

@cofyc cofyc cofyc left review comments

@weekface weekface Awaiting requested review from weekface

Assignees
No one assigned
Labels
contribution status/can-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@shonge @aylei @claassistantio @sre-bot @weekface @cofyc @tennix