Skip to content

Avoid assigning duplicate RAFT IDs to new nodes.#5571

Merged
martinmr merged 5 commits intomasterfrom
martinmr/dup-raft-id
Jun 8, 2020
Merged

Avoid assigning duplicate RAFT IDs to new nodes.#5571
martinmr merged 5 commits intomasterfrom
martinmr/dup-raft-id

Conversation

@martinmr
Copy link
Copy Markdown
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
Copy Markdown
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 force-pushed the martinmr/dup-raft-id branch from 572f42e to fb8429b Compare June 4, 2020 21:53
@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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