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

Add a Jepsen style bank test for Badger #577

Merged
merged 11 commits into from Sep 25, 2018

Conversation

Projects
None yet
2 participants
@manishrjain
Copy link
Member

manishrjain commented Sep 21, 2018

Breaking Change

This change breaks Value API. Instead of Value returning a byte slice, we now expose the byte slice from within a function. This is because when we read a value, we acquire a read lock on the value log. With the previous API it was possible for a transaction to deadlock, if multiple reads are done on the same log file, and those read locks get interleaved with a write lock caused by log file rotation. The new API fixes that by releasing the lock before another value is read; and also makes it obvious to the end user that they must copy the value to be able to use it outside the function.

This change also adds a Jepsen style bank test for Badger, which is currently showing some transaction violations, needing further investigation.

This change is Reviewable

manishrjain added some commits Sep 21, 2018

@manishrjain manishrjain merged commit c10276c into master Sep 25, 2018

5 of 8 checks passed

License Compliance 5 issues found
Details
continuous-integration/appveyor/branch AppVeyor build failed
Details
code-review/reviewable 13 files left
Details
Unit Tests (badger) TeamCity build finished
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls First build on mrjn/bank at 80.432%
Details
license/cla Contributor License Agreement is signed.
Details

@manishrjain manishrjain deleted the mrjn/bank branch Sep 25, 2018

@@ -95,16 +95,20 @@ func (item *Item) Version() uint64 {
// If you need to use a value outside a transaction, please use Item.ValueCopy
// instead, or copy it yourself. Value might change once discard or commit is called.
// Use ValueCopy if you want to do a Set after Get.
func (item *Item) Value() ([]byte, error) {
func (item *Item) Value(fn func(val []byte)) error {

This comment has been minimized.

@shazow

shazow Oct 2, 2018

Hey, sorry for the unsolicited feedback. Not sure if you're mimicking another reference API, but in this case I wonder if this function signature would be more convenient:

- func (item *Item) Value(fn func(val []byte)) error {
+ func (item *Item) Value(fn func(val []byte) error) error {

Where the returned error would be forwarded into Value's error. This way it could be used as:

	return item.Value(func(val []byte) error {
		return NewDecoder(bytes.NewReader(val)).Decode(into)
	})

Instead of

    var err error
	err = item.Value(func(val []byte) {
		if decodeErr := NewDecoder(bytes.NewReader(val)).Decode(into); decodeErr != nil {
			err = decodeErr
		}
	})
    return err

Feel free to disregard this feedback if it's not relevant. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment