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

update state without proposal when leader changes in zero #2319

merged 3 commits into from Apr 12, 2018


Copy link

@janardhan1993 janardhan1993 commented Apr 9, 2018

Fixes #2273

This change is Reviewable

Copy link
Contributor Author

Review status: 0 of 3 files reviewed at latest revision, all discussions resolved.

dgraph/cmd/zero/raft.go, line 407 at r1 (raw file):

func (n *node) triggerLeaderChange() {

We can update leader information on each node without proposal. This function is called on all nodes on leader change.

dgraph/cmd/zero/raft.go, line 408 at r1 (raw file):

	m := &intern.Member{Id: n.Id, Addr: n.RaftContext.Addr, Leader: n.AmLeader()}
	go n.proposeAndWait(context.Background(), &intern.ZeroProposal{Member: m})

Proposing it in goroutine can mess up the order and cause wrong state.

worker/groups.go, line 250 at r1 (raw file):

	defer g.Unlock()
	if g.state != nil && g.state.Counter >= state.Counter {

We don't update state if we get any old state. Counter stores the raftindex of last state. For leader changes at zero since we don't propose, state can get updated at same counter value.

Comments from Reviewable

Copy link

:lgtm: for the first two changes. Add the comments explaining them in the code please.

Not sure why we create another copy of the key. Does ReadPostingList modify the key?

Reviewed 3 of 3 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

@janardhan1993 janardhan1993 merged commit 3673f50 into master Apr 12, 2018
@janardhan1993 janardhan1993 deleted the jan/node_lockup branch Apr 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

4 participants