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

feat(live): added upsert in live loader #6057

Merged
merged 21 commits into from
Aug 17, 2020
Merged

Conversation

harshil-goel
Copy link
Contributor

@harshil-goel harshil-goel commented Jul 22, 2020


This change is Reviewable

Docs Preview: Dgraph Preview

@github-actions github-actions bot added the area/live-loader Issues related to live loading. label Jul 22, 2020
@harshil-goel harshil-goel marked this pull request as ready for review July 22, 2020 16:20
@harshil-goel harshil-goel changed the title feat(live): added upsert in live loader (alternative) feat(live): added upsert in live loader Jul 22, 2020
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.

Let's first narrow down why Dgraph was going OOM for upserts run via it. Think the debugging showed that the cache wasn't properly set up. So, do that first. And then see if you still need this change.

Overall, upserts in Dgraph should work and we should make them more robust.

Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @harshil-goel, @manishrjain, and @vvbalaji-dgraph)


dgraph/cmd/live/run.go, line 156 at r1 (raw file):

	flag.Bool("ludicrous_mode", false, "Run live loader in ludicrous mode (Should "+
		"only be done when alpha is under ludicrous mode)")
	flag.StringP("upsertPredicate", "U", "", "run in upsertPredicate mode. the value would be used to "+

100 chars.


dgraph/cmd/live/run.go, line 255 at r1 (raw file):

	for _, i := range nqs {
		// taking hash as the value might contain invalid symbols
		ids[i.Subject] = fmt.Sprintf("u_%d", farm.Fingerprint64([]byte(i.Subject)))

Avoid fmt.Sprintf wherever possible. Perhaps strconv.Itoa.

Copy link
Contributor

@parasssh parasssh left a comment

Choose a reason for hiding this comment

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

:lgtm: minor comments.

Reviewable status: 0 of 7 files reviewed, 5 unresolved discussions (waiting on @harshil-goel and @vvbalaji-dgraph)


dgraph/cmd/live/run.go, line 245 at r2 (raw file):

}

func generateUidFunc(val string) string {

Put an example of the generated UidFunc in comments


dgraph/cmd/live/run.go, line 253 at r2 (raw file):

}

func generateQuery(node, predicate, xid string) string {

Same here. Write an example in comments.


dgraph/cmd/live/run.go, line 269 at r2 (raw file):

	// We form upsertPredicate query for each of the ids we saw in the request, along with
	// adding the corresponding xid to that uid. The mutation we added is only useful if the
	// uid doesn't exists.

if m.1234 exists, then the mutation still happens on the same m.1234? Is there a way to make it no-op so we dont incur the cost of additional mutation.

@parasssh parasssh merged commit e33bb1d into master Aug 17, 2020
@bucanero bucanero deleted the harshil-goel/upsert-live-temp branch November 11, 2020 22:50
@jarifibrahim
Copy link
Contributor

Related to DGRAPH-1181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/live-loader Issues related to live loading.
Development

Successfully merging this pull request may close these issues.

4 participants