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

don't pre allocate mutation map #4343

Merged
merged 3 commits into from
Dec 6, 2019
Merged

don't pre allocate mutation map #4343

merged 3 commits into from
Dec 6, 2019

Conversation

poonai
Copy link
Contributor

@poonai poonai commented Dec 2, 2019

Signed-off-by: balaji jinnah rbalajis25@gmail.com


This change is Reviewable

Signed-off-by: balaji jinnah <rbalajis25@gmail.com>
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.

Not sure if the change would have a lot of impact but it should be fine as long as you fix the breaking tests. The stacktrace below seems directly related to the change:

------- Stdout: -------
=== RUN   TestExportRdf
--- FAIL: TestExportRdf (0.01s)
panic: assignment to entry in nil map [recovered]
	panic: assignment to entry in nil map

goroutine 46 [running]:
testing.tRunner.func1(0xc0002a0900)
	/usr/local/go/src/testing/testing.go:830 +0x392
panic(0x14fed00, 0x17c14d0)
	/usr/local/go/src/runtime/panic.go:522 +0x1b5
github.com/dgraph-io/dgraph/posting.(*List).updateMutationLayer(0xc000093800, 0xc0000ef860)
	/home/pawan0201/go/src/github.com/dgraph-io/dgraph/posting/list.go:323 +0x232
github.com/dgraph-io/dgraph/posting.(*List).addMutationInternal(0xc000093800, 0x17fa340, 0xc0000c8010, 0xc0002aafc0, 0xc0000ef680, 0xc0004bebf8, 0xc0004bebf8)
	/home/pawan0201/go/src/github.com/dgraph-io/dgraph/posting/list.go:444 +0xb1
github.com/dgraph-io/dgraph/posting.(*Txn).addMutationHelper(0xc0002aafc0, 0x17fa340, 0xc0000c8010, 0xc000093800, 0x0, 0xc0000ef680, 0x0, 0x0, 0x0, 0x8db500, ...)
	/home/pawan0201/go/src/github.com/dgraph-io/dgraph/posting/index.go:361 +0x205
github.com/dgraph-io/dgraph/posting.(*List).AddMutationWithIndex(0xc000093800, 0x17fa340, 0xc0000c8010, 0xc0000ef680, 0xc0002aafc0, 0x13, 0x0)
	/home/pawan0201/go/src/github.com/dgraph-io/dgraph/posting/index.go:394 +0x11e
github.com/dgraph-io/dgraph/worker.commitTransaction(0xc0002a0900, 0xc0000ef680, 0xc000093800)
	/home/pawan0201/go/src/github.com/dgraph-io/dgraph/worker/predicate_test.go:71 +0x102
github.com/dgraph-io/dgraph/worker.addEdge(...)
	/home/pawan0201/go/src/github.com/dgraph-io/dgraph/worker/worker_test.go:50
github.com/dgraph-io/dgraph/worker.populateGraphExport(0xc0002a0900)
	/home/pawan0201/go/src/github.com/dgraph-io/dgraph/worker/export_test.go:98 +0x4d0
github.com/dgraph-io/dgraph/worker.initTestExport(0xc0002a0900, 0x1650e53, 0x14)
	/home/pawan0201/go/src/github.com/dgraph-io/dgraph/worker/export_test.go:134 +0x94c
github.com/dgraph-io/dgraph/worker.TestExportRdf(0xc0002a0900)
	/home/pawan0201/go/src/github.com/dgraph-io/dgraph/worker/export_test.go:184 +0x58
testing.tRunner(0xc0002a0900, 0x1696008)
	/usr/local/go/src/testing/testing.go:865 +0xc0
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:916 +0x35a
exit status 2
FAIL	github.com/dgraph-io/dgraph/worker	0.098s

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @manishrjain)

Signed-off-by: balaji jinnah <rbalajis25@gmail.com>
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.

Looks good from my side. Got one comment. Please address @martinmr and get his approval.

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @balajijinnah)


posting/list.go, line 82 at r2 (raw file):

// initialize map if it's nil. caller has to take care of
// lock.
func (l *List) initializeMutationMap() {

I doubt there's a need for this func. Maybe remove this func and just inline the code.

Signed-off-by: பாலாஜி <balaji@dgraph.io>
Copy link
Contributor Author

@poonai poonai 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 3 files reviewed, 1 unresolved discussion (waiting on @manishrjain)


posting/list.go, line 82 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

I doubt there's a need for this func. Maybe remove this func and just inline the code.

Done.

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:

Reviewed 3 of 3 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manishrjain)

@poonai poonai merged commit bbf50c5 into master Dec 6, 2019
@joshua-goldstein joshua-goldstein deleted the alaji/map branch August 11, 2022 20:24
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