Flush vlog buffer if it grows beyond threshold #1067
Conversation
jarifibrahim
requested review from
ashish-goswami,
manishrjain and
a team
as code owners
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ A review job has been created and sent to the PullRequest network.
@jarifibrahim you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
Reviewed the ticket details (#1062 (comment)) to the test and implementation. I added an inline comment regarding readability as well.
Reviewed with ❤️ by PullRequest
| @@ -1315,6 +1315,18 @@ func (vlog *valueLog) write(reqs []*request) error { | |||
| p.Len = uint32(plen) | |||
| b.Ptrs = append(b.Ptrs, p) | |||
| written++ | |||
|
|
|||
There was a problem hiding this comment.
Can the buffer length be stored since it is retrieved multiple times?
bl := buf.Len()
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @ashish-goswami, @jarifibrahim, and @manishrjain)
value.go, line 1319 at r2 (raw file):
written++ if int64(buf.Len()) > vlog.db.opt.ValueLogFileSize {
Please add a comment explaining, why we are doing this change.
Looks like new code written has something common with toDisk method. Can we somehow avoid the repetition?
Also if possible please add a test for this(may be in db2_test.go which can be run manually).
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @ashish-goswami, @manishrjain, and @pullrequest[bot])
value.go, line 1318 at r1 (raw file):
Previously, pullrequest[bot] wrote…
Can the buffer length be stored since it is retrieved multiple times?
bl := buf.Len()
I've changed the code and this comment is no longer valid.
value.go, line 1319 at r2 (raw file):
Previously, ashish-goswami (Ashish Goswami) wrote…
Please add a comment explaining, why we are doing this change.
Looks like new code written has something common withtoDiskmethod. Can we somehow avoid the repetition?
Also if possible please add a test for this(may be in db2_test.go which can be run manually).
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @manishrjain and @pullrequest[bot])
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jarifibrahim and @pullrequest[bot])
value.go, line 1263 at r3 (raw file):
var buf bytes.Buffer toDisk := func() error {
flushBuffer
value.go, line 1279 at r3 (raw file):
return nil } flushWrites := func() error {
toDisk
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @manishrjain and @pullrequest[bot])
value.go, line 1263 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
flushBuffer
Done.
value.go, line 1279 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
toDisk
Done.
The OOM happens when we trying to flush write batch. When we insert data into a transaction, we calculate the size of the transaction by using the following function https://github.com/dgraph-io/badger/blob/b20e8b6e8b742236f4a88f7c0cead2d64118a416/txn.go#L295-L300 which uses `estimateSize` function https://github.com/dgraph-io/badger/blob/b20e8b6e8b742236f4a88f7c0cead2d64118a416/structs.go#L134-L139 For a key size of 4 and a value size of 1 MB, the `estimateSize` function would return 4 + 12 + 2 = 18 byte which is valid in case of SSTs but not for a vlog file. The entry size for vlog would be around 1 MB and when all such entries are packed into a single request for vlog, the bytes.buffer in vlog crashes with OOM error. This commit fixes it. (cherry picked from commit bcf8b97)
See #1062 (comment) for more details.
Fixes #1062
This change is