Change the way we handle removes with a context #761
Conversation
ghost
assigned 
| split_ops([{update, {_Name, ?MAP_TYPE}=Key, Op} | Rest], Pre0, Post0) -> | ||
| {Pre, Post} = split_ops(Op), | ||
| split_ops(Rest, [{update, Key, Pre} | Pre0], [{update, Key, Post} | Post0]); | ||
| split_ops([{update, Key, {remove, _}}=Op | Rest], Pre, Post) -> |
There was a problem hiding this comment.
So, could this be solved by ensuring that any nested update (whether add or remove) in the map results in an increment to the clock and dot? I guess I'm just concerned that this is a larger issue with riak_dt_map.
There was a problem hiding this comment.
Can you explain the issue?
There was a problem hiding this comment.
Doesn't riak_dt_map already increment its clock and apply the dot to fields that contain nested removes? I guess that's my question.
There was a problem hiding this comment.
Yes, but the remove is applied to the context, so the increment is applied to the context. If the replica local copy has already incremented past that point for the field in question the update loses. That is the purpose of the patch, to ensure that the field update is applied in the adds list.
There was a problem hiding this comment.
So to answer your question: "Yes" and this is how we ensure that.
There was a problem hiding this comment.
Aha, I think I see now.
|
👍 Thanks for sorting this out, could have bit us big time. |
Change the way we handle removes with a context
As @lenary pointed out, in some situations a field update concurrent with a field remove, could result in the update losing.
We applied removes to the context. But removes of a nested map field, or nested set element are also a Add for the containing field. If the local replica has since modified the field, and then removed it, the clock for the Map will dominate the dot for the field in the context. The field remove wins over the concurrent field update.
By making sure that all field updates (even removes) end up on the Add list of operations this PR fixes that bug.
There is an eunit test that fails without this fix, and passes with it, in riak_kv_crdt.
Another change is that the remove ops are applied to the context, and the add operations are applied to the local merged state and only then are the context and local state merged. Previously the updated context was merged with the local state before the add operations were performed.