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

server: refactor cluster related logic #1654

Merged
merged 4 commits into from Aug 8, 2019

Conversation

@rleungx
Copy link
Member

commented Jul 25, 2019

What problem does this PR solve?

Part of modularizing PD. Do not review it now.

What is changed and how it works?

This PR removes clusterInfo and merge its functions into RaftCluster to make the code less confused.

Check List

Tests

  • Unit test

@rleungx rleungx added the WIP label Jul 25, 2019

@@ -120,6 +137,18 @@ func (c *RaftCluster) loadBootstrapTime() (time.Time, error) {
return typeutil.ParseTimestamp([]byte(data))
}

func (c *RaftCluster) initCluster() {

This comment has been minimized.

Copy link
@nolouch

nolouch Jul 29, 2019

Member

Rename RaftCluster to Cluster?

This comment has been minimized.

Copy link
@rleungx

rleungx Jul 29, 2019

Author Member

As for the naming problem, I think we can leave it to another PR.

@rleungx rleungx force-pushed the rleungx:refactor-cluster branch from 2ced7f2 to d61d62a Jul 29, 2019

@rleungx rleungx removed the WIP label Jul 29, 2019

@rleungx rleungx requested review from disksing and shafreeck Jul 29, 2019

return c.meta.GetId()
}

func (c *RaftCluster) putMetaLocked(meta *metapb.Cluster) error {

This comment has been minimized.

Copy link
@shafreeck

shafreeck Aug 1, 2019

Contributor

Forgot the lock?

This comment has been minimized.

Copy link
@rleungx

rleungx Aug 2, 2019

Author Member

*Locked means we already have a lock outside this function, and thus we don't need the lock in this function.

return c.putStoreLocked(newStore)
}

func (c *RaftCluster) putStoreLocked(store *core.StoreInfo) error {

This comment has been minimized.

Copy link
@shafreeck

shafreeck Aug 1, 2019

Contributor

Does not have a lock

This comment has been minimized.

Copy link
@rleungx

rleungx Aug 2, 2019

Author Member

ditto

@@ -635,12 +1047,26 @@ func (c *RaftCluster) RemoveTombStoneRecords() error {
return nil
}

func (c *RaftCluster) deleteStoreLocked(store *core.StoreInfo) error {

This comment has been minimized.

Copy link
@shafreeck

shafreeck Aug 1, 2019

Contributor

Still does not have a lock

This comment has been minimized.

Copy link
@rleungx

rleungx Aug 2, 2019

Author Member

ditto

storesInfo := &StoresInfo{
Stores: make([]*StoreInfo, 0, len(stores)),
}

stores = filter.filter(stores)
for _, s := range stores {
store, err := cluster.GetStore(s.GetId())
store, err := cluster.TryGetStore(s.GetId())

This comment has been minimized.

Copy link
@disksing

disksing Aug 1, 2019

Member

Why need a Try here?

This comment has been minimized.

Copy link
@rleungx

rleungx Aug 2, 2019

Author Member

Because we already have a function named GetStore. It doesn't return err. We can merge these two functions into one. But we need to change a lot of code including interfaces. I will raise another PR to do it.

server/cluster.go Outdated Show resolved Hide resolved
server/cluster.go Outdated Show resolved Hide resolved
server/coordinator_test.go Outdated Show resolved Hide resolved

rleungx added some commits Jul 25, 2019

refactor cluster related logic
Signed-off-by: Ryan Leung <rleungx@gmail.com>
address comments
Signed-off-by: Ryan Leung <rleungx@gmail.com>
resolve the conflict
Signed-off-by: Ryan Leung <rleungx@gmail.com>

@rleungx rleungx force-pushed the rleungx:refactor-cluster branch from 042db87 to 06ba232 Aug 8, 2019

@nolouch

nolouch approved these changes Aug 8, 2019

@nolouch nolouch merged commit fa508a1 into pingcap:master Aug 8, 2019

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
ci/circleci Your tests passed on CircleCI!
Details
idc-jenkins-ci-pd/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.