fix race conditions in bulk loader and export #3697
Conversation
… is called concurrently, and adding locks to protect the counter would be too expensive
…e validation should be run on individual files
There was a problem hiding this comment.
I am confused by the diff (specifically the export.go file). It looks like it includes changes from https://github.com/dgraph-io/dgraph/pull/3682/files. Am I missing something?
Reviewed 2 of 3 files at r1.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @gitlw and @manishrjain)
dgraph/cmd/bulk/reduce.go, line 138 at r1 (raw file):
send
sends
dgraph/cmd/bulk/reduce.go, line 139 at r1 (raw file):
iterating
iterating over
There was a problem hiding this comment.
Nevermind, I noticed you merged the other changes from master.
Now other than the couple of minor fixes I noticed, it
Reviewed 1 of 3 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gitlw and @manishrjain)
worker/export.go, line 529 at r2 (raw file):
case pk.IsData(): // defining a local error variable to avoid concurrent write
minor: uppercase and period.
|
Hey @gitlw , this looks like a significant change (a good change, tbh). Definitely not a NOOP, so should have been reviewed by me. Did you do benchmarks with your changes? It might have some impact. |
|
@manishrjain Will run the benchmarks, and will refrain from merging without your LGTM (sorry about that). |
fixes #3693 which is about concurrent write to an err variable by multiple go routines that are triggered via the StreamWriter.KeyToList function
also fixes reported race conditions in the bulk loader by avoiding concurrent calls to StreamWriter.Write
This change is