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

Fix PR/PW #485

Merged
merged 3 commits into from Feb 28, 2013

Conversation

Projects
None yet
3 participants
@Vagabond
Contributor

Vagabond commented Feb 12, 2013

Originally, PR/PW simply checked, in the validate phase of the GET/PUT
FSM that the required number of primaries were online.

However, that was insufficient. Consider PW=2, PR=2, N=3 case; if 2 primaries
are online. You do the write, but one primary and one fallback receive
the write, the third write fails (but you've already returned a result
to the client, because 2 writes succeeded). Now, when you go to read
that key, 2 primaries are still online, but you read from the primary
that just replaced the fallback and the primary that failed the write
before. Congratulations, you just failed to read-your-own-writes.

It gets worse, even. Because the node_watcher can lag behind the actual
up/down state of a node, especially if the node kernel panics or has its
NIC unplugged, primaries can be reported up for some time after they've
stopped responding (until the distributed erlang connection closes).
This means that its even easier to fail PR/PW but still have the FSM
pass the validation stage. And since R and PR are unrelated, even PW=N
could spuriously validate (if R < PR).

The solution to all this is to simply use the preflist to check we get
the required number of responses from a primary before the FSM will
return. However, this required significant modification to the
Quickcheck tests. Additionally, some simple sanity check eunit tests
were added. Most of the actual changes in this branch are just
EQC/Eunit changes and refactoring.

This also means that PR/PW will now block a response from returning
until they've been satisfied. Thusly, PR=2 now will wait for at least 2
replies, possibly all N if necessary, to satisfy the constraint, meaning
you don't need to specify PR and R anymore. The same is true for PW,
except that PW waits for DW responses, not W responses, to satisfy the
constraint.

Additionally, the rules on how DW relates to W have been changed, DW is
always at least 1 (for the coordinating put), but it is no longer
demoted to match W.

@Vagabond

This comment has been minimized.

Show comment
Hide comment
@Vagabond
Contributor

Vagabond commented Feb 12, 2013

@Vagabond

This comment has been minimized.

Show comment
Hide comment
@Vagabond

Vagabond Feb 13, 2013

Contributor

I just re-ran the scenario described here:

http://lists.basho.com/pipermail/riak-users_lists.basho.com/2012-January/007034.html

I'm unable to reproduce the bad behaviour that was reported. Although, note that test/foo on my ring hashed to a preflist that had 2 members on one node, test2/foo hashed better for me, so that is what I used.

I switched back to the master branch, and the bug came back.

Contributor

Vagabond commented Feb 13, 2013

I just re-ran the scenario described here:

http://lists.basho.com/pipermail/riak-users_lists.basho.com/2012-January/007034.html

I'm unable to reproduce the bad behaviour that was reported. Although, note that test/foo on my ring hashed to a preflist that had 2 members on one node, test2/foo hashed better for me, so that is what I used.

I switched back to the master branch, and the bug came back.

@beerriot

View changes

Show outdated Hide outdated src/riak_kv_get_core.erl
@beerriot

View changes

Show outdated Hide outdated src/riak_kv_get_fsm.erl
@beerriot

View changes

Show outdated Hide outdated src/riak_kv_put_core.erl
@beerriot

View changes

Show outdated Hide outdated src/riak_kv_put_core.erl
[
{"Checking W",
fun() ->
%% you can never fail W directly...

This comment has been minimized.

@beerriot

beerriot Feb 21, 2013

Contributor

There should be a test with num_fail > 0, if they are to prove that "you can never fail W directly".

@beerriot

beerriot Feb 21, 2013

Contributor

There should be a test with num_fail > 0, if they are to prove that "you can never fail W directly".

This comment has been minimized.

@Vagabond

Vagabond Feb 22, 2013

Contributor

W failures don't increment num_fail, only DW failures do.

W results are always either 'good' or are not sent at all:

https://github.com/basho/riak_kv/blob/master/src/riak_kv_vnode.erl#L367

@Vagabond

Vagabond Feb 22, 2013

Contributor

W failures don't increment num_fail, only DW failures do.

W results are always either 'good' or are not sent at all:

https://github.com/basho/riak_kv/blob/master/src/riak_kv_vnode.erl#L367

@beerriot

View changes

Show outdated Hide outdated src/riak_kv_put_fsm.erl
@beerriot

View changes

Show outdated Hide outdated src/riak_kv_put_core.erl
@beerriot

View changes

Show outdated Hide outdated src/riak_kv_get_core.erl

russelldb and others added some commits Jan 15, 2013

Fix PR/PW
Originally, PR/PW simply checked, in the validate phase of the GET/PUT
FSM that the required number of primaries were online.

However, that was insufficient. Consider PW=2, PR=2, N=3 case; if 2 primaries
are online. You do the write, but one primary and one fallback receive
the write, the third write fails (but you've already returned a result
to the client, because 2 writes succeeded). Now, when you go to read
that key, 2 primaries are still online, but you read from the primary
that just replaced the fallback and the primary that failed the write
before. Congratulations, you just failed to read-your-own-writes.

It gets worse, even. Because the node_watcher can lag behind the actual
up/down state of a node, especially if the node kernel panics or has its
NIC unplugged, primaries can be reported up for some time after they've
stopped responding (until the distributed erlang connection closes).
This means that its even easier to fail PR/PW but still have the FSM
pass the validation stage. And since R and PR are unrelated, even PW=N
could spuriously validate (if R < PR).

The solution to all this is to simply use the preflist to check we get
the required number of responses from a primary before the FSM will
return. However, this required significant modification to the
Quickcheck tests. Additionally, some simple sanity check eunit tests
were added. Most of the actual changes in this branch are just
EQC/Eunit changes and refactoring.

This also means that PR/PW will now *block* a response from returning
until they've been satisfied. Thusly, PR=2 now will wait for at least 2
replies, possibly all N if necessary, to satisfy the constraint, meaning
you don't need to specify PR *and* R anymore. The same is true for PW,
except that PW waits for DW responses, not W responses, to satisfy the
constraint.

Additionally, the rules on how DW relates to W have been changed, DW is
always at least 1 (for the coordinating put), but it is no longer
demoted to match W.
Address concerns raised during review:
* Fix test setup to be more valid
* Fix some specs
* Move idx_type to get core
* Remove w_fail_threshold
* erlang:max() -> lists:max()
* Switch error tuples to always be 2-tuples
* Update comment about meeting PR
@Vagabond

This comment has been minimized.

Show comment
Hide comment
@Vagabond

Vagabond Feb 23, 2013

Contributor

Riak test here: basho/riak_test#218

Contributor

Vagabond commented Feb 23, 2013

Riak test here: basho/riak_test#218

Fix wm_object to handle dw_val_unsatisfied
dw_val_unsatisfied is the replacement for w_val_unsatisfied, which
turned out to be useless and was removed.
@Vagabond

This comment has been minimized.

Show comment
Hide comment
@Vagabond

Vagabond Feb 27, 2013

Contributor

Rebased and pushed a fix for webmachine error handling.

Contributor

Vagabond commented Feb 27, 2013

Rebased and pushed a fix for webmachine error handling.

@beerriot

This comment has been minimized.

Show comment
Hide comment
@beerriot

beerriot Feb 28, 2013

Contributor

The given test makes sense, fails pre-patch, passes post-patch. I like it. +1!

Contributor

beerriot commented Feb 28, 2013

The given test makes sense, fails pre-patch, passes post-patch. I like it. +1!

@ghost ghost assigned Vagabond Feb 28, 2013

Vagabond added a commit that referenced this pull request Feb 28, 2013

@Vagabond Vagabond merged commit 2cecfff into 1.3 Feb 28, 2013

1 check failed

default The Travis build failed
Details

@Vagabond Vagabond deleted the fix-pr-pw branch Feb 28, 2013

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