-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Expose Badger Compression Level option in Bulk Loader #4669
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jarifibrahim and @manishrjain)
dgraph/cmd/bulk/reduce.go, line 152 at r2 (raw file):
opt.ZSTDCompressionLevel = r.state.opt.BadgerCompressionLevel if r.state.opt.BadgerCompressionLevel < 1 { opt.ZSTDCompressionLevel = 1
We should throw an error if the compression level is invalid.
0 might be a valid value (to disable compression).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @danielmai and @manishrjain)
dgraph/cmd/bulk/reduce.go, line 152 at r2 (raw file):
Previously, danielmai (Daniel Mai) wrote…
We should throw an error if the compression level is invalid.
0 might be a valid value (to disable compression).
0
should not be used to disable the compression. We have a separate option to set the compression algorithm and disable it
https://github.com/dgraph-io/badger/blob/master/options.go#L126
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this PR so it's just for the compression flag? There's actually no need for the table/vlog options in bulk loader.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @danielmai and @manishrjain)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I've removed the table and vlog loading mode option. 👍
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @danielmai and @manishrjain)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment, and then . Do update the PR title and description. This closes #4439.
Reviewed 3 of 3 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @danielmai, @jarifibrahim, and @manishrjain)
dgraph/cmd/bulk/loader.go, line 73 at r3 (raw file):
// BadgerKeyFile is the file containing the key used for encryption. Enterprise only feature. BadgerKeyFile string // Badger is the compression level to use while writing to badger.
BadgerCompressionLevel is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @danielmai, @jarifibrahim, and @manishrjain)
…h-io/dgraph into ibrahim/badger-option-bulk
This PR exposes badger's
CompressionLevel
option in bulk loader.Fixes #4439
This change is