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

Create an EQC statem test for the DVV style riak_object #807

Closed
wants to merge 6 commits into from

Conversation

russelldb
Copy link
Member

When making test I had to cut and paste the riak_kv_vnode:put_merge into the test. The same code is also cut and pasted into ec_eqc. Seems an obvious candidate for a refactor. And riak_object seemed like the location since put_merge tinkered with riak_object state.

Since the put_merge code was copied into two tests, both of which
only manipulate riak_object, it seemed a refactor made sense.

Looking at the put merge code, it only manipulates internal state
of riak_object, so I think it is the correct place for the code.

It also smooths the way for a general refactor of riak_object
into a CRDT and riak_kv into riak_crdt (mwah ha ha ha ha!)
@ghost ghost assigned bookshelfdave Jan 17, 2014
FrontierClock = vclock:increment(Actor, Timestamp, MergedClock),
{ok, Dot} = vclock:get_entry(Actor, FrontierClock),
%% Assign an event to the new value
DottedPutObject = assign_dot(NewObject, Dot, dvv_enabled()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't you just remove this assign_dot function from the module?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the exported 'assign_dot/2', kept this private one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but it's missing from the diff above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh n/m disregard

].

%% Like when a user does a put without first fetching (blank vv) @see
%% next_state/3 for the return tuple being prepended to vnode_data
Copy link
Member Author

Choose a reason for hiding this comment

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

note to me, when I address Dave's review comments, fix this too.

@bookshelfdave
Copy link

this test seems fine. It passed against 100k EQC runs.

@bookshelfdave
Copy link

I noticed src/riak_kv_mutator.erl is still in your branch, can you rebase against develop?

@russelldb
Copy link
Member Author

I addressed all comments except that desire to simplify the fake preflist generation. I really should address that, but I want the code this test tests into the build on Friday. Honestly I'll simplify the test in a later PR, since it will need to change to address that weird forwarding siblings issue.

I rebased, and now when I try and push git tells me I need to pull and merge. Do I force push? Some other thing? Please can you help?

@bookshelfdave
Copy link

I'd say create a new rebased branch + PR, close this one but reference it from the new PR.

@russelldb
Copy link
Member Author

Closing and opening new PR for rebased branch.

@russelldb russelldb closed this Feb 12, 2014
@bookshelfdave
Copy link

Link to new PR: #835

@seancribbs seancribbs deleted the rdb/robj-merge-eqc branch April 1, 2015 23:36
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.

None yet

3 participants