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

Add max wait count for transfer leader operator. #147

Merged

Conversation

qiuyesuifeng
Copy link
Contributor

No description provided.

@siddontang
Copy link
Contributor

why not handling directly in LeaderTransfer?
I think it breaks the semantics for Once.

@qiuyesuifeng qiuyesuifeng force-pushed the qiuyesuifeng/leader-transfer-operator branch from 491ff11 to 3132e46 Compare June 11, 2016 04:48
@qiuyesuifeng qiuyesuifeng changed the title server: wrap leader transfer operator as must succeed once operator. server: make transfer leader operator must be successful. Jun 11, 2016
@qiuyesuifeng
Copy link
Contributor Author

@siddontang Agreed, PTAL.

@@ -176,6 +176,8 @@ func (co *ChangePeerOperator) Do(region *metapb.Region, leader *metapb.Peer) (bo

// TransferLeaderOperator is used to do leader transfer.
type TransferLeaderOperator struct {
mustSuc bool
Copy link
Member

@ngaut ngaut Jun 11, 2016

Choose a reason for hiding this comment

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

s/mustSuc/mustSuccess/ ?

@siddontang
Copy link
Contributor

LGTM

@shenli
Copy link
Member

shenli commented Jun 12, 2016

LGTM

@qiuyesuifeng
Copy link
Contributor Author

@shenli @siddontang
After the discuss, we all think using hb count is better, so i update this check.
PTAL

newLeader: newLeader,
oldLeader: oldLeader,
newLeader: newLeader,
count: 0,
Copy link
Member

Choose a reason for hiding this comment

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

The default value of count is already zero.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is a variable like index, we should update it in Do function.

Copy link
Contributor

Choose a reason for hiding this comment

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

count and maxWaitCount confuse me.

you can initialize count as waitCount and then use lto.count < 0 lt.count-- to check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems not, we must store the origin maxWaitCount to make sure the first time we return the transfer leader response.
If we only use count, then we cannot distinct it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it.

@shenli
Copy link
Member

shenli commented Jun 12, 2016

LGTM

@qiuyesuifeng qiuyesuifeng changed the title server: make transfer leader operator must be successful. Add max wait count for transfer leader operator. Jun 12, 2016
@qiuyesuifeng qiuyesuifeng merged commit 02f8226 into qiuyesuifeng/auto-balance Jun 12, 2016
@qiuyesuifeng qiuyesuifeng deleted the qiuyesuifeng/leader-transfer-operator branch June 12, 2016 06:51
JmPotato pushed a commit to JmPotato/pd that referenced this pull request Sep 25, 2023
* fix watch keyspace test case

Signed-off-by: ystaticy <y_static_y@sina.com>
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.

4 participants