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

Avoid assigning duplicate RAFT IDs to new nodes. #5571

Merged
merged 5 commits into from
Jun 8, 2020

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Jun 3, 2020

Currently, zero uses the MaxRaftId to assign RAFT IDs to new nodes. However, the
MaxRaftId value is not immediately updated and the lock is released in between
creating the proposal and sending the proposal to the RAFT node. This can lead
to situations where more than one new node is assigned the same ID.

To solve this, this change introduces a new field to generate the IDs that is
updated immediately, thus preventing multiple nodes from being assigned
the same ID.

Fixes #5436


This change is Reviewable

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @manishrjain, @martinmr, and @vvbalaji-dgraph)


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

	}

	if has && m.Addr != member.Addr {

This could avoid an address change from happening.

@martinmr martinmr changed the title Check for duplicate RAFT IDs when applying membership updates. Avoid assigning duplicate RAFT IDs to new nodes. Jun 4, 2020
Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @manishrjain and @vvbalaji-dgraph)


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

Previously, manishrjain (Manish R Jain) wrote…

This could avoid an address change from happening.

Done. Reverted this change.

@martinmr martinmr requested a review from manishrjain June 4, 2020 22:03
@martinmr martinmr dismissed manishrjain’s stale review June 4, 2020 22:04

addressed comments

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @manishrjain, @martinmr, and @vvbalaji-dgraph)


dgraph/cmd/zero/zero.go, line 500 at r2 (raw file):

		if m.Id == 0 {
			m.Id = s.nextRaftId
			s.nextRaftId += 1

A proposal could error out, and then get applied later. Therefore, it is best to keep incrementing nextRaftId.

Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @manishrjain and @vvbalaji-dgraph)


dgraph/cmd/zero/zero.go, line 500 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

A proposal could error out, and then get applied later. Therefore, it is best to keep incrementing nextRaftId.

Done.

@martinmr martinmr merged commit c093805 into master Jun 8, 2020
@martinmr martinmr deleted the martinmr/dup-raft-id branch June 8, 2020 18:44
martinmr added a commit that referenced this pull request Jun 8, 2020
Currently, zero uses the MaxRaftId to assign RAFT IDs to new nodes. However, the
MaxRaftId value is not immediately updated and the lock is released in between
creating the proposal and sending the proposal to the RAFT node. This can lead
to situations where more than one new node is assigned the same ID.

To solve this, this change introduces a new field to generate the IDs that is
updated immediately, thus preventing multiple nodes from being assigned
the same ID.

Fixes #5436
martinmr added a commit that referenced this pull request Jun 8, 2020
Currently, zero uses the MaxRaftId to assign RAFT IDs to new nodes. However, the
MaxRaftId value is not immediately updated and the lock is released in between
creating the proposal and sending the proposal to the RAFT node. This can lead
to situations where more than one new node is assigned the same ID.

To solve this, this change introduces a new field to generate the IDs that is
updated immediately, thus preventing multiple nodes from being assigned
the same ID.

Fixes #5436
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
Currently, zero uses the MaxRaftId to assign RAFT IDs to new nodes. However, the
MaxRaftId value is not immediately updated and the lock is released in between
creating the proposal and sending the proposal to the RAFT node. This can lead
to situations where more than one new node is assigned the same ID.

To solve this, this change introduces a new field to generate the IDs that is
updated immediately, thus preventing multiple nodes from being assigned
the same ID.

Fixes dgraph-io#5436
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Zero assigned two Alphas the same Raft index.
2 participants