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

Limit UIDs per variable in upsert #4268

Merged
merged 3 commits into from
Nov 27, 2019
Merged

Limit UIDs per variable in upsert #4268

merged 3 commits into from
Nov 27, 2019

Conversation

mangalaman93
Copy link
Contributor

@mangalaman93 mangalaman93 commented Nov 13, 2019

fixes #4021


This change is Reviewable

for i, gmu := range qc.gmuList {
isCondUpsert := strings.TrimSpace(gmu.Cond) != ""
if isCondUpsert {
qc.condVars[i] = fmt.Sprintf("__dgraph_%d__", rand.Int())

Choose a reason for hiding this comment

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

G404: Use of weak random number generator (math/rand instead of crypto/rand) (from gosec)

Copy link
Contributor

@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.

:lgtm:

Reviewed 2 of 13 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @danielmai, @mangalaman93, @manishrjain, @MichaelJCompton, and @pawanrawal)


dgraph/cmd/alpha/upsert_test.go, line 2559 at r2 (raw file):

}

// This test is supposed to be ignored by the CI

Maybe add a comment on why it should be ignored.

@mangalaman93 mangalaman93 changed the base branch from aman/multiple_mutations to master November 21, 2019 20:36
@mangalaman93 mangalaman93 changed the base branch from master to aman/multiple_mutations November 25, 2019 16:24
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: Got a comment.

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @danielmai, @mangalaman93, @martinmr, @MichaelJCompton, and @pawanrawal)


edgraph/server.go, line 996 at r3 (raw file):

		// don't do bad things to alpha and mutation doesn't become too big.
		if len(v.Uids.Uids) > 1e6 {
			return resp, errors.Errorf("variable [%v] has too many UIDs (>1m)", name)

"has over million UIDs. Please narrow .. down."

Copy link
Contributor

@pawanrawal pawanrawal 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, 3 unresolved discussions (waiting on @danielmai, @mangalaman93, @martinmr, @MichaelJCompton, and @pawanrawal)

@mangalaman93 mangalaman93 changed the base branch from aman/multiple_mutations to master November 27, 2019 08:47
@mangalaman93 mangalaman93 merged commit 0bbc0a2 into master Nov 27, 2019
@mangalaman93 mangalaman93 deleted the aman/limit branch November 27, 2019 09:45
@mangalaman93 mangalaman93 restored the aman/limit branch November 27, 2019 09:45
@mangalaman93 mangalaman93 deleted the aman/limit branch November 27, 2019 09:45
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.

Upsert Block fails when processing multiple nodes
5 participants