Add per key actor epochs #1040
Add per key actor epochs #1040
Conversation
use the counter to create a per epoch key when local not found.
When it hits that limit create a new vnodeid.
Start a new epoch if the local object has a lower epoch for the vnode id than the incoming object (no epoch is lower, no epich is zero)
Bad match for re-refactord put_merge return. Correct return type for highest_actor
russelldb
force-pushed
the
bug/rdb/gh679-actor-epochs
branch
from
194e222 to
dc581bd
Compare
use the bare vnodeid as long as possible, only start a new epoch when needed. In a way the bare vnodeid is just epoch zero, so use it. Also, consider the incoming counter when deciding about new epochs. An incoming clock with the same actor+epoch but greater counter is a hint that a byzantine failure occurred and a new epoch is needed
NOTE: make dialyzer exits with an error, but no information. Could do with some help on that.
| State; | ||
| maybe_lease_counter(State) -> | ||
| #state{status_mgr_pid=MgrPid, counter=CS=#counter_state{cnt=Cnt, lease=Lease, lease_size=LeaseSize}} = State, | ||
| %% @TODO configurable?? |
There was a problem hiding this comment.
Yes, this should probably be configurable/tunable.
There was a problem hiding this comment.
Yeah. It is hard to decide if it should be. Trying to figure out a scenario where you'd want to change this. And adding it adds more chance to shoot yourself in the foot. And more complex validation of the config item. And can it change in the vnode's lifetime? Do we need to add it to state, or check the env var every iteration?
I'm not against making it configurable, I'd just like a better understanding of the benefit vs. risk/complexity.
| vnode_pid :: undefined | pid() | ||
| }). | ||
|
|
||
| -type status() :: [proplists:property()] | []. |
There was a problem hiding this comment.
The empty list is not strictly necessary in this type. Also, is there any case where you will have bare atoms in the list? If not, orddict:orddict() might be better all around.
There was a problem hiding this comment.
I don't know, it is inherited.
|
I rebased against 2.0 before it moves too far away. I addressed your comments. Sadly I can't push to this branch now. Instead I opened a new PR and ref'd this one from riak_kv#1053 |
See riak_kv#679 and the associated platform_task RFC and summary.
Requires basho/riak_core#646
This PR addresses the "doomstone", backup-restore, and some byzantine flavours of the kv679 bug.
The RFC explains the mechanism in detail but briefly:
This ensures that a first time write for a key gets a new actor, this is the epoch for the key. It means we don't mix up deleted+re-created keys {a,1} event with the original {a, 1} event for some key, by ensuring an actor per epoch, without causing a keyspace wide actor explosion.
I hope that is enough details for the review to start. I'll start work on EQC for the status_mgr and the new vnode functions in the mean time.