basho / riak_dt

Convergent replicated datatypes in Erlang
Apache License 2.0
353 stars 70 forks source link

Add preconditions failures to orset / map #53

Closed russelldb closed 11 years ago

russelldb commented 11 years ago

Update all modules update/3 to return {ok, CRDT} | {error, Fail}

Also discovered broken EQC tests, which this PR has fixes for, and then assorted fixes for bugs found.

These tests then found errors in the precondition code, which I fixed (multi-ops were not failing atomically.)

There is also a change to the Map to keep tombstones (a bug was dropping them when to replicas that agreed on removal merged, even if a third had a concurrent update.) This leads to some surprising behaviour if two concurrent removes occur concurrent with an update, the reset occurs twice, and for a counter that has partitioned since a merge of all increments, that will result in a double decrement. I imagine it is a very rare edge case, but I note it here as we don't have a n answer yet.

Also changed the reset strategy on LWW to reset the timestamp to zero. This is non-monotonic. It deals with the case where a reset / remove | update causally, but happens temporally after the update, resulting in a present register field with an undefined value.

Also, removed zorset and rwmap, no time to get them correct for 2.0.

Added bulk operations to Set types (add_all, remove_all, {update, [{add, _} | {remove, _} | etc}

lenary commented 11 years ago

Maybe it's not supposed to be part of this PR, but riak_dt_orset has no reset function.

russelldb commented 11 years ago

No, it needs one if we allow people to store it in Maps. We haven't made the call yet, but right now we are using vvorset.

lenary commented 11 years ago

Right, my only note is that https://github.com/basho/riak_dt/pull/53/files#L5R91 suggests it does, however, I get that everything should be going via the encoders where you limit the possibilities further.

lenary commented 11 years ago

Looks good to me!

seancribbs commented 11 years ago

Some dialyzer warnings:

riak_dt_gcounter.erl:80: Invalid type specification for function riak_dt_gcounter:update/3. The success typing is ('increment' | {'increment',pos_integer()},_,riak_dt_gcounter:gcounter()) -> {'ok',riak_dt_gcounter:gcounter()}
riak_dt_multi.erl:289: Function precondition_context/3 will never be called
riak_dt_multi.erl:296: Function precondition_context/2 will never be called
riak_dt_orset.erl:90: Invalid type specification for function riak_dt_orset:update/3. The success typing is ({'add',_} | {'add_all',[any()]} | {'remove',_} | {'remove_all',maybe_improper_list()} | {'update',[any()]},_,_) -> {'error',{'precondition',{_,_}}} | {'ok',_}
riak_dt_orset.erl:90: The return type riak_dt_orset:orset() in the specification of update/3 is not a subtype of {'error',_} | {'ok',_}, which is the expected return type for the callback of riak_dt behaviour
riak_dt_orset.erl:191: The call riak_dt_orset:to_binary({_,[]}) does not have an opaque term of type riak_dt_orset:orset() as 1st argument
riak_dt_orset.erl:196: The return type riak_dt_orset:binary_orset() in the specification of to_binary/1 is not a subtype of binary(), which is the expected return type for the callback of riak_dt behaviour
riak_dt_pncounter.erl:96: Invalid type specification for function riak_dt_pncounter:update/3. The success typing is ('decrement' | 'increment' | {_,integer()},_,_) -> {'ok',_}
riak_dt_vvorset.erl:123: Invalid type specification for function riak_dt_vvorset:update/3. The success typing is ({'add',_} | {'add_all',[any()]} | {'remove',_} | {'remove_all',maybe_improper_list()} | {'update',[any()]},_,_) -> {'error',{'precondition',{_,_}}} | {'ok',_}
riak_dt_vvorset.erl:123: The return type riak_dt_vvorset:vvorset() in the specification of update/3 is not a subtype of {'error',_} | {'ok',_}, which is the expected return type for the callback of riak_dt behaviour
riak_dt_vvorset.erl:267: The call riak_dt_vvorset:to_binary({_,[{_,{_,_}}]}) does not have an opaque term of type riak_dt_vvorset:vvorset() as 1st argument
Unknown types:
  ordsets:ordset/0
  riak_dt_multi:multi_op/0
  riak_dt_orset:orset_op/0
seancribbs commented 11 years ago
seancribbs commented 11 years ago

@russelldb This doesn't seem to add the partitioning of operations by whether they have preconditions. Is that only in KV or is it here?

seancribbs commented 11 years ago

This is really well-tested and clear, even with the many commits. There are only a few documentation typos that need fixing, then :+1:.

russelldb commented 11 years ago

@seancribbs yeah, the precondition before / after application is in riak_kv. Reason being that it only makes sense in a hybrid op/state/"action at a distance" context, and not in a pure state based one.

(And I didn't want to complicate the pure CRDTs further.)