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

fix(proposals): Incremental proposal key for zero proposals #8005

Merged
merged 3 commits into from
Sep 2, 2021

Conversation

ahsanbarkati
Copy link
Contributor

@ahsanbarkati ahsanbarkati commented Aug 30, 2021

Change proposal's unique key to an atomic counter instead of using randomly generated key. For a randomly generated key of 32bits, there is a considerable probability of getting the same key used again.


This change is Reviewable

Copy link
Contributor

@NamanJain8 NamanJain8 left a comment

Choose a reason for hiding this comment

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

LGTM

@ahsanbarkati ahsanbarkati changed the title fix(proposals): Incremental proposal key for zero fix(proposals): Incremental proposal key for zero proposals Sep 2, 2021
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.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ahsanbarkati)


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

func (n *node) initProposalKey(id uint64) {
	x.AssertTrue(id != 0)
	proposalKey = uint64(n.Id)<<48 | uint64(z.FastRand())<<16

You can use crypto.Rand instead of fast rand, which would provide more unique numbers.

@ahsanbarkati ahsanbarkati merged commit a515d0d into master Sep 2, 2021
@ahsanbarkati ahsanbarkati deleted the ahsan/proposal-key branch September 2, 2021 16:58
NamanJain8 pushed a commit that referenced this pull request Sep 3, 2021
Change the proposal's unique key to an atomic counter instead of using a randomly generated key.

(cherry picked from commit a515d0d)
all-seeing-code pushed a commit that referenced this pull request Jan 4, 2023
Change the proposal's unique key to an atomic counter instead of using a randomly generated key.

(cherry picked from commit a515d0d)
(cherry picked from commit 2aa3d3e)
all-seeing-code pushed a commit that referenced this pull request Jan 20, 2023
Change the proposal's unique key to an atomic counter instead of using a randomly generated key.

(cherry picked from commit a515d0d)
(cherry picked from commit 2aa3d3e)
all-seeing-code pushed a commit that referenced this pull request Jan 23, 2023
Change the proposal's unique key to an atomic counter instead of using a randomly generated key.

(cherry picked from commit a515d0d)
(cherry picked from commit 2aa3d3e)
all-seeing-code pushed a commit that referenced this pull request Jan 24, 2023
Change the proposal's unique key to an atomic counter instead of using a randomly generated key.

(cherry picked from commit a515d0d)
(cherry picked from commit 2aa3d3e)
all-seeing-code pushed a commit that referenced this pull request Jan 24, 2023
Change the proposal's unique key to an atomic counter instead of using a randomly generated key.

(cherry picked from commit a515d0d)
(cherry picked from commit 2aa3d3e)
all-seeing-code added a commit that referenced this pull request Feb 8, 2023
Change the proposal's unique key to an atomic counter instead of using a randomly generated key.

(cherry picked from commit a515d0d)
(cherry picked from commit 2aa3d3e)
all-seeing-code added a commit that referenced this pull request Feb 8, 2023
…8567)

Change the proposal's unique key to an atomic counter instead of using a
randomly generated key.

Proposal key initialisation has a bug where we want to reserve first 2 bytes for node id but do not do because we read random bytes in all 8 bytes and do a logical OR. This PR adds a test case and fixes the logic of initialising the proposal key.

(cherry picked from commit a515d0d)
(cherry picked from commit 2aa3d3e)
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.

3 participants