Skip to content

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

Merged
xiang90 merged 1 commit intoetcd-io:masterfrom
absolute8511:optimize-raft-drop
Jan 12, 2018
Merged

raft: let raft step return error when proposal is dropped to allow fail-fast#9067
xiang90 merged 1 commit intoetcd-io:masterfrom
absolute8511:optimize-raft-drop

Conversation

@absolute8511
Copy link
Copy Markdown
Contributor

@absolute8511 absolute8511 commented Dec 25, 2017

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
Copy Markdown
Contributor Author

absolute8511 commented Jan 3, 2018

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

@xiang90
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

absolute8511 commented Jan 8, 2018

@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
Copy Markdown

codecov-io commented Jan 11, 2018

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
campaignTransfer CampaignType = "CampaignTransfer"
)

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

@xiang90 xiang90 Jan 11, 2018

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
Copy Markdown
Contributor

@xiang90 xiang90 Jan 12, 2018

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
Copy Markdown
Contributor

xiang90 commented Jan 11, 2018

lgtm.

/cc @siddontang @bdarnell

@xiang90
Copy link
Copy Markdown
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
Copy Markdown
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

2 more reviewers

@xiang90 xiang90 xiang90 left review comments

@bdarnell bdarnell bdarnell approved these changes

Reviewers whose approvals may not affect merge requirements

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