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: raft learners should be returned after applyConfChange #9116

Merged
merged 1 commit into from
Jan 11, 2018
Merged

raft: raft learners should be returned after applyConfChange #9116

merged 1 commit into from
Jan 11, 2018

Conversation

absolute8511
Copy link
Contributor

This should fix issue #9087

@xiang90
Copy link
Contributor

xiang90 commented Jan 8, 2018

/cc @siddontang can you please take a look?

@codecov-io
Copy link

codecov-io commented Jan 8, 2018 •
edited
Loading

Codecov Report

Merging #9116 into master will decrease coverage by 0.13%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9116      +/-   ##
==========================================
- Coverage   76.25%   76.12%   -0.14%     
==========================================
  Files         359      359              
  Lines       29983    29987       +4     
==========================================
- Hits        22864    22827      -37     
- Misses       5541     5576      +35     
- Partials     1578     1584       +6
Impacted Files Coverage Δ
raft/raft.go 91.54% <100%> (+0.04%) ⬆️
raft/rawnode.go 68.33% <50%> (ø) ⬆️
raft/node.go 90.62% <50%> (-0.9%) ⬇️
pkg/transport/timeout_conn.go 80% <0%> (-20%) ⬇️
etcdserver/api/v3rpc/lease.go 78.37% <0%> (-8.11%) ⬇️
pkg/fileutil/purge.go 73.68% <0%> (-7.9%) ⬇️
etcdctl/ctlv3/command/lease_command.go 65.34% <0%> (-5.95%) ⬇️
clientv3/concurrency/session.go 88.63% <0%> (-4.55%) ⬇️
pkg/adt/interval_tree.go 88.58% <0%> (-2.41%) ⬇️
rafthttp/msgappv2_codec.go 71.3% <0%> (-1.74%) ⬇️
... and 14 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...11fa4f0. Read the comment docs.

@siddontang
Copy link
Contributor

Do we use the node function outside of raft lib? if yes, we should also handle learner nodes now.

@absolute8511
Copy link
Contributor Author

Currently, as I know it is used only by raft lib. If any future usage maybe another pr needed.

xiang90
xiang90 reviewed Jan 10, 2018
View reviewed changes
raft/node_test.go Outdated
s.Append(rd.Entries)
t.Logf("raft: %v", rd.Entries)
for _, ent := range rd.Entries {
if ent.Type == raftpb.EntryConfChange {
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation is crazy here. let us kill one indentation by doing

if ent.Type != raftpb.EntryConfChange {
    continue
}
var cc raftpb.ConfChange
...

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

xiang90
xiang90 reviewed Jan 10, 2018
View reviewed changes
raft/node_test.go Outdated
if len(state.Learners) == 0 ||
state.Learners[0] != cc.NodeID ||
cc.NodeID != 2 {
t.Fatalf("apply conf change should return new added learner: %v", state.String())
Copy link
Contributor

Choose a reason for hiding this comment

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

use t.Errorf

xiang90
xiang90 reviewed Jan 10, 2018
View reviewed changes
raft/node_test.go Outdated
}

if len(state.Nodes) != 1 {
t.Fatalf("add learner should not change the nodes: %v", state.String())
Copy link
Contributor

Choose a reason for hiding this comment

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

use t.errorf

@xiang90
Copy link
Contributor

xiang90 commented Jan 10, 2018

@absolute8511 Can you rebase with master?

Looks good to me. @siddontang can you give this a final look?

@siddontang
Copy link
Contributor

Rest LGTM

@siddontang siddontang mentioned this pull request Jan 11, 2018
Closed
@xiang90
Copy link
Contributor

xiang90 commented Jan 11, 2018

@absolute8511 can you please rebase with master, and resolve the merge conflicts? thank you.

@absolute8511 absolute8511 force-pushed the fix-raft-learner branch 2 times, most recently from fa1a0e5 to 75cdeef Compare January 11, 2018 09:22
@absolute8511
Copy link
Contributor Author

All done @xiang90

@xiang90 xiang90 merged commit 285c7e6 into etcd-io:master Jan 11, 2018
@siddontang siddontang mentioned this pull request Jan 15, 2018
Closed
@xiang90 xiang90 mentioned this pull request Jan 18, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siddontang siddontang siddontang left review comments

@xiang90 xiang90 xiang90 left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

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