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

raft: let raft step return error when proposal is dropped to allow fail-fast #9067

Merged
merged 1 commit into from
Jan 12, 2018
Merged

raft: let raft step return error when proposal is dropped to allow fail-fast #9067

merged 1 commit into from
Jan 12, 2018

Conversation

absolute8511
Copy link
Contributor

While the leader is transferring or some other states which the raft node may drop the proposal, we should fail fast to notify the proposal cancelled.

This should fix issue #8975 and #8977

@absolute8511 absolute8511 force-pushed the optimize-raft-drop branch 3 times, most recently from ae6f2c4 to 63d14e3 Compare December 26, 2017 11:49
@absolute8511 absolute8511 changed the title Add a new ProposeWithCancel to allow propose fail fast while the proposal is dropped by the raft raft, etcdserver: Add a new ProposeWithCancel to allow propose fail fast while the proposal is dropped by the raft Dec 26, 2017
@absolute8511
Copy link
Contributor Author

Seems the commit message Merge pull request #1 from coreos/master failed? Could it be ignored?

@xiang90
Copy link
Contributor

xiang90 commented Jan 5, 2018

@absolute8511

can you separate the PR into smaller ones?

I think the first one can be let raft step return error when proposal is dropped to allow fail-fast.

then we can hook it up with etcd, and upper layers.

@absolute8511 absolute8511 changed the title raft, etcdserver: Add a new ProposeWithCancel to allow propose fail fast while the proposal is dropped by the raft raft: let raft step return error when proposal is dropped to allow fail-fast Jan 8, 2018
@absolute8511
Copy link
Contributor Author

@xiang90

The PR is separated now.

@absolute8511 absolute8511 force-pushed the optimize-raft-drop branch 4 times, most recently from b5b998b to a4b7655 Compare January 11, 2018 09:13
@codecov-io
Copy link

Codecov Report

Merging #9067 into master will decrease coverage by 0.28%.
The diff coverage is 89.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9067      +/-   ##
==========================================
- Coverage   76.25%   75.97%   -0.29%     
==========================================
  Files         359      359              
  Lines       29983    29988       +5     
==========================================
- Hits        22864    22783      -81     
- Misses       5541     5611      +70     
- Partials     1578     1594      +16
Impacted Files Coverage Δ
raft/raft.go 91.56% <89.65%> (+0.05%) ⬆️
auth/simple_token.go 87.03% <0%> (-6.49%) ⬇️
clientv3/namespace/watch.go 93.93% <0%> (-6.07%) ⬇️
pkg/adt/interval_tree.go 84.98% <0%> (-6.01%) ⬇️
etcdctl/ctlv3/command/lease_command.go 65.34% <0%> (-5.95%) ⬇️
clientv3/concurrency/session.go 88.63% <0%> (-4.55%) ⬇️
proxy/grpcproxy/watch.go 90.06% <0%> (-4.35%) ⬇️
etcdserver/api/v3rpc/watch.go 84.12% <0%> (-3.87%) ⬇️
clientv3/leasing/txn.go 88.09% <0%> (-3.18%) ⬇️
etcdserver/api/v3election/election.go 64.7% <0%> (-2.95%) ⬇️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52f73c5...a4b7655. Read the comment docs.

@xiang90 xiang90 self-assigned this Jan 11, 2018
xiang90
xiang90 reviewed Jan 11, 2018
View reviewed changes
raft/raft.go Outdated
@@ -67,6 +67,8 @@ const (
campaignTransfer CampaignType = "CampaignTransfer"
)

var ErrProposalDropped = errors.New("raft proposal dropped")
Copy link
Contributor

Choose a reason for hiding this comment

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

doc string on public error?

Copy link
Contributor

Choose a reason for hiding this comment

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

// ErrProposalDropped is returned by the Step function when the passed in proposal is dropped.

@xiang90
Copy link
Contributor

xiang90 commented Jan 11, 2018

lgtm.

/cc @siddontang @bdarnell

@xiang90
Copy link
Contributor

xiang90 commented Jan 12, 2018

@absolute8511 Can you please add the comment to the error message? then i will get this PR merged right away.

@xiang90 xiang90 merged commit c5532eb into etcd-io:master Jan 12, 2018
@xiang90
Copy link
Contributor

xiang90 commented Jan 12, 2018

@absolute8511 thanks. looking forward to follow up PRs to hook this up with etcd application layer.

@siddontang siddontang mentioned this pull request Jan 12, 2018
Closed
@absolute8511 absolute8511 mentioned this pull request Jan 12, 2018
Merged
@siddontang siddontang mentioned this pull request Jan 15, 2018
Closed
@bdarnell bdarnell mentioned this pull request Jan 28, 2018
Open
@langkeer langkeer mentioned this pull request Apr 9, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiang90 xiang90 xiang90 left review comments

@bdarnell bdarnell bdarnell approved these changes

Assignees

@xiang90 xiang90

Labels
area/raft
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@absolute8511 @xiang90 @codecov-io @bdarnell