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

namespaceChecker #755

Merged
merged 11 commits into from Sep 20, 2017

Conversation

Projects
None yet
7 participants
@dantin
Collaborator

dantin commented Sep 15, 2017

Add namespaceChecker to move peer to the targeted store

@sre-bot

This comment has been minimized.

Show comment
Hide comment
@sre-bot

sre-bot Sep 15, 2017

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.

sre-bot commented Sep 15, 2017

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

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Sep 15, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Sep 15, 2017

CLA assistant check
All committers have signed the CLA.

@dantin dantin assigned disksing and siddontang and unassigned siddontang and disksing Sep 18, 2017

@dantin dantin requested review from huachaohuang and disksing Sep 18, 2017

@dantin dantin removed the request for review from huachaohuang Sep 18, 2017

dantin added some commits Sep 18, 2017

add check logic
check if peer is located on a store that is already belongs to the right namespace.
@disksing

This comment has been minimized.

Show comment
Hide comment
@disksing

disksing Sep 19, 2017

Member

The rest LGTM. PTAL @nolouch @siddontang

Member

disksing commented Sep 19, 2017

The rest LGTM. PTAL @nolouch @siddontang

dantin added some commits Sep 19, 2017

add test case
Move the right region when some region is in the right place while others are not
add another test case
Move region with multiple peer to the right place
@disksing

This comment has been minimized.

Show comment
Hide comment
@disksing

disksing Sep 19, 2017

Member

LGTM.

Member

disksing commented Sep 19, 2017

LGTM.

for _, peer := range region.GetPeers() {
// check whether the peer has been already located on a store that is belong to the target namespace
if n.isExists(targetStores, peer.StoreId) {
continue

This comment has been minimized.

@nolouch

nolouch Sep 19, 2017

Member

you can directly use Namespace filter in SelectBestPeerToRelocate function

@nolouch

nolouch Sep 19, 2017

Member

you can directly use Namespace filter in SelectBestPeerToRelocate function

This comment has been minimized.

@dantin

dantin Sep 20, 2017

Collaborator

I don't think it's a good idea to call getNamespaceStores() every time in the loop.

@dantin

dantin Sep 20, 2017

Collaborator

I don't think it's a good idea to call getNamespaceStores() every time in the loop.

This comment has been minimized.

@nolouch

nolouch Sep 20, 2017

Member

got it

@nolouch

nolouch Sep 20, 2017

Member

got it

This comment has been minimized.

@dantin

dantin Sep 20, 2017

Collaborator

thx

@dantin

dantin Sep 20, 2017

Collaborator

thx

@nolouch

LGTM

@nolouch nolouch merged commit 183069f into namespace Sep 20, 2017

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
jenkins-ci-pd/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@nolouch nolouch deleted the dantin/checker branch Sep 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment