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

Update badger2 dependency to v2.2007.2 #31

Merged
merged 6 commits into from
Oct 5, 2020
Merged

Update badger2 dependency to v2.2007.2 #31

merged 6 commits into from
Oct 5, 2020

Conversation

gammazero
Copy link
Contributor

This is needed to fix a protobuf conflict between badger and badger2 when both are linked into the same binary.

Issue described here:
dgraph-io/badger#1208

Fix described here:
dgraph-io/badger#1293

This is needed to fix a protobuf conflict between badger and badger2 when both are linked into the same binary.
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
This resolves a protobuf namespace conflict that would happen when both badger1 and badger2 are linked into the same binary.  For more information on this issue see: dgraph-io/badger#1519
… Lotus uses, and prevents transaction failures when writing larger batches with FileIO option used for the value log.
Copy link
Contributor

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me @Kubuxu is this OK by you?

@gammazero
Copy link
Contributor Author

gammazero commented Oct 5, 2020

@Kubuxu Here is an explanation of what was done to get badger2 working:

Depend on a newer version of badger2 that has two build fixes:

  • Fixes integer overflow on 32bit builds
  • Resolves protobuf namespace conflict with badger1

Use configuration options that includes disabling mmap for the value log and using FileIO instead. This was apparently supposed to have been used previously, but was missed or was was not done because it resulted in transaction failures with large batch writes. That issues is fixed by the following change:

Stop using LSMOnly options and use DefualtOptions instead. This changes the ValueThreshold to a much smaller size, and is the better choice because it fixes transaction failures for large batch writes, when used in conjunction with FileIO for the value log. It is also better to use the default value as this is what Lotus uses.

In a future PR, batch writes will be handled by a BatchWriter instead of a Transaction, which should allow large batch writes to work with either IO configuration above. Doing that will take care of ipfs/go-ds-badger#92

@aschmahmann aschmahmann requested a review from Kubuxu October 5, 2020 17:11
Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

SGWM.

In Lotus, we are also using mmap IO. IDK how we feel about changing the default for that.

@Kubuxu
Copy link
Member

Kubuxu commented Oct 5, 2020

Also, we have ncreased LSM to the badger default, the 16MB was causing us to hit FD limits.

@Kubuxu Kubuxu merged commit 8fe207b into master Oct 5, 2020
@Kubuxu Kubuxu deleted the update-badger2 branch October 5, 2020 17:34
@gammazero
Copy link
Contributor Author

@Kubuxu If Lotus is using mmap IO, then I would not want to change that. Using FileIO avoids mmap allocation failures during some 32bit tests, but I don't know that is something that lotus is concerned with.

@Kubuxu
Copy link
Member

Kubuxu commented Oct 5, 2020

We for sure use more than 3.5GiB memory, so no 32bit without pointer extensions for us.

mvdan added a commit that referenced this pull request Jul 22, 2021
Back in October 2020, we merged #31 to update to a commit from master,
in order to pull in a fix for protobuf warnings/panics.

Luckily, the upstream fix has now been backported and released as
v2.2007.3. Use it, instead of continuing to rely on a stray master
commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants