Skip to content

Vendor latest badger #4007

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

Merged
merged 1 commit into from
Sep 18, 2019
Merged

Vendor latest badger #4007

merged 1 commit into from
Sep 18, 2019

Conversation

jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Sep 17, 2019

This change pulls the following commits from badger

http://github.com/dgraph-io/badger/commit/cbdef65 Fix windows build for badger
http://github.com/dgraph-io/badger/commit/ee70ff2 Store entire L0 in memory
http://github.com/dgraph-io/badger/commit/86a77bb Implement PageBuffer
http://github.com/dgraph-io/badger/commit/f88aa0c Add FAQ about manifest version mismatch
http://github.com/dgraph-io/badger/commits/a1ff348 Minor optimizations
http://github.com/dgraph-io/badger/commits/7e99a81 fix small typo ("duing" to "during")


This change is Reviewable

@jarifibrahim jarifibrahim requested a review from a team September 17, 2019 14:20
Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

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

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

I provided a few typo fix suggestions inline for further review.


Reviewed with ❤️ by PullRequest

read += n
r.startIdx += n

// Instead of len(cp.buf), we comparing with cap(cp.buf). This ensures that we move to next
Copy link

Choose a reason for hiding this comment

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

// Instead of len(cp.buf), we compare with cap(cp.buf)

// Truncate truncates PageBuffer to length n.
func (b *PageBuffer) Truncate(n int) {
pageIdx, startIdx := b.pageForOffset(n)
// For simplicity of the code reject extra pages. These pages can be kept.
Copy link

Choose a reason for hiding this comment

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

Did you mean something like this?

// For simplicity, reject extra pages. The existing pages can be kept

}

// PageBuffer consists of many pages. A page is a wrapper over []byte. PageBuffer can act as a
// replacement of bytes.Buffer. Instead of having single underlying buffer, it has multiple
Copy link

Choose a reason for hiding this comment

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

// Instead of having single underlying buffer to Instead of having a single underlying buffer


// NewPageBuffer returns a new PageBuffer with first page having size pageSize.
func NewPageBuffer(pageSize int) *PageBuffer {
b := &PageBuffer{}
Copy link

Choose a reason for hiding this comment

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

I understand what is happening here but it is really two steps the way it was written. You are mixing value and pointer semantics together.

Given how you are using the b variable, prefer

b := new(PageBuffer)

continue
}

// When last page in not full to its capacity and we have read all data up to its
Copy link

Choose a reason for hiding this comment

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

// When the last page is not ...

Copy link
Member

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

Address the comments from @_pullrequestbot in a separate PR on badger @jarifibrahim

Copy link
Contributor

@danielmai danielmai left a comment

Choose a reason for hiding this comment

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

Great to see the Windows build issue is fixed. :lgtm:

Reviewable status: 0 of 17 files reviewed, 5 unresolved discussions (waiting on @jarifibrahim and @manishrjain)

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