Vclock pruning should probably drop the oldest entry first

[russelldb]

See https://github.com/basho/riak_core/blob/develop/src/vclock.erl#L251

The logic appears to be:

Sort the vclock so that the youngest entry is first.


If the clock is longer than small_vclock and it's youngest entry is older than young_vclock and it's longer than big_vclock and it's youngest entry is older than old_vclock we remove the youngest entry from the vclock, and start over again.



This seems wrong, shouldn't we remove the oldest entry?

[russelldb]

More comments

https://github.com/basho/riak_core/blob/develop/src/vclock.erl#L235 is probably incorrect, since it leaves the youngest entry at the head of the list.

See https://github.com/basho/riak_kv/blob/develop/src/riak_kv_vnode.erl#L1180 and https://github.com/basho/riak_kv/blob/develop/src/riak_kv_vnode.erl#L1338 mean pruning is never called on read repair.

So, pruning is only ever called on a coordinated put. The PruneTime is the StartTime from the PUT fsm. And this time is the time of the youngest entry in the clock, since the coord_put just incremented said clock. All of which leaves a vclock entry with a ts of just about riak_core_util:moment() at the head of the list.

Then this https://github.com/basho/riak_core/blob/develop/src/vclock.erl#L243. Now that Now and that HeadTime…aren't they always going to be the same time? So isn't the result of (Now - HeadTime) always going to be 0? And so we return the clock untouched. If somehow this is not true (how, anyone?) Pruning will proceed to trim the newest entry first.

From talking to @bsparrow435 it seems pruning is never really seen on client sites, as they never have vclocks as long as 50, but I still think it is worth fixing this bug, and better documenting the code too.

[bsparrow435]

I agree with @russelldb. The sort function is not working as it should. Example:

13> V4.
[{<<210,103,72,101,83,39,134,252>>,{1,63573819319}},
 {<<153,84,36,117,83,39,134,197>>,{1,63573819333}},
 {<<238,83,20,227,83,39,134,238>>,{1,63573819372}}]
14> SortV1 = lists:sort(fun({N1,{_,T1}},{N2,{_,T2}}) -> {T1,N1} < {T2,N2} end, V4).
[{<<210,103,72,101,83,39,134,252>>,{1,63573819319}},
 {<<153,84,36,117,83,39,134,197>>,{1,63573819333}},
 {<<238,83,20,227,83,39,134,238>>,{1,63573819372}}]
15> SortV2 = lists:sort(fun({N1,{_,T1}},{N2,{_,T2}}) -> {T1,N1} > {T2,N2} end, V4).
[{<<238,83,20,227,83,39,134,238>>,{1,63573819372}},
 {<<153,84,36,117,83,39,134,197>>,{1,63573819333}},
 {<<210,103,72,101,83,39,134,252>>,{1,63573819319}}]

First sort function result has the newest entry first which will result in the behavior @russelldb described. The function need to be reversed so it returns the oldest entry as the head of the list.

[jrwest]

fwiw, I remember reading through the code about a week ago (after seeing this issue) and drawing the same conclusions. We should either fix it or remove pruning all together if we aren't even regularly hitting the default threshold (iirc pruning was meant to address bloat with client-side vclocks).

[russelldb]

Plan is

1. Add stats counter so we can see if we ever prune IRL
2. Fix code to be correct (as per docs)

But none of it is high priority (like 2.0 blocker) since it may never actually be run IRL.

Sound fair?

[rzezeski]

DISCLAIMER: I'm basing the following comments off vague memories from over 2 years ago.

EDIT: I meant to say that client-side ids caused actor explosion, and thus we needed pruning to cull the actor list. This pruning could thus cause superfluous siblings when combined with things like MDC. Also see my next comment.

Pruning probably doesn't matter as much as it did back in the 0.14.x days. Client-side ids would cause massive sibling explosion for some use cases and thus we needed some way to cull the list. But I don't need to tell either of you that :). I made a change to set the small/large size to be the same (50 IIRC) because pruning can cause bugs with MDC. Something like tombstone resurrection and I think perhaps data loss. This goes back to the days when Ian Plosker, who found the bug, was a DA! My change was an attempt to avoid pruning as much as possible without actually removing the feature because we still had rare cases of sibling explosion (e.g. the internal tech talk where the DVV creators showed us sibling explosion even with vnode vclocks). AFAIK, pruning, even if implemented correctly, can still cause issues with MDC. So if there are ways to completely avoid it that would be great. Anyways, just wanted to add some vague historical context in case it helps.

[russelldb]

Interesting. I wasn't aware that pruning and sibling explosion were related. Pruning would actually make siblings more likely. I thought pruning came from actor explosion from client side ids. Brian, above, suggested that it rarely if ever occurs now we have vnode ids. Some CSEs, in some situation, have set young_vclock to -1 to ensure no pruning ever. I imagine this is for some MDC issue, and I'll do some more research on that.

Thanks very much for adding some context. I think this is why adding stats around pruning is essential, so CSEs/Customers can be aware if it even happens.

[rzezeski]

I wasn't aware that pruning and sibling explosion were related. Pruning would actually make siblings more likely. I thought pruning came from actor explosion from client side ids.

Crap, I meant actor explosion, not sibling, sigh.

Here is my small_vclock commit. You can tell how old it is by the fact that I reference bugzilla, haha. In that commit I state that this is to prevent superfluous siblings in MDC, which is true, but I also could swear that Ian found even more insidious behavior than that in some cases. But, I'm working with very fuzzy memory here. Happy hunting :).

basho/riak_kv@cdd6448

[russelldb]

heh, thanks again @rzezeski. We're hiring you know

[bsparrow435]

This hit a CS customer over the past weekend and resulted in handoff creating siblings in an infinite loop. Firs time i've seen it but not surprising with CS as the manifest objects are often updated simultaneously and the customer was restarting nodes across the cluster repeatedly during a leave operation.

[krestenkrab]

Is pruning really needed at all since riak dropped client-side id's? Or is this only a recent change, can't remember exactly.

[jonmeredith]

It is code from the age of client-side ids, but to hit it, I suspect they
had at least 50 actor ids in there, so something is going on.

On Thu, Oct 23, 2014 at 1:27 PM, Kresten Krab Thorup <
notifications@github.com> wrote:

Is pruning really needed at all since riak dropped client-side id's? Or is
this only a recent change, can't remember exactly.

—
Reply to this email directly or view it on GitHub
#613 (comment).

Jon Meredith
Chief Architect
Basho Technologies, Inc.
jmeredith@basho.com

[krestenkrab]

My experience is that once you hit pruning, the system is likely to go berserk in siblings; most likely because you end up activating pruning on a write-write conflict.

So to avoid the problem, we'd have to mark VCs as pruned - with a "prune stone" (some magic actor id), or recognise them as such because of their length - and in that case do a special filtering on both VCs before comparison by removing entries that are in the (wallclock-wise) old end of the VCs which are not occurring in both VCs being compared - or some such.

Without such special case (heuristic / scientifically incorrect) recognition and logic for pruned VCs, you'd always end up with conflicting writes on all following PUTs. If the given object is a particularly write-contented object, then you drive head-first right into an explosion.

[russelldb]

@krestenkrab this is your experience even with DVV? I thought with DVV we had a handle on sibling explosion.

I think I see what your are advocating, having a pruned vclock prune other clocks (like an epidemic) before it compares with them…but how do you unset the "prune stone" (nice term!) How do you know when you've seen all vclocks for key X and they've all been pruned? Or do you just prune clocks to the same length all the time? Who's length? The longest? The shortest? Some heuristic (max 25?) Oh, I think also, never prune an entry for which you have a dotted value.

If the pruning algorithm wasn't broken (actually removed the oldest, not newest entry), and DVV was in use, I don't think you'd see the issues you describe. But that is just my intuition. I also think we can leverage more than timestamps, with the new key epoch stuff coming as fixes for kv679, we could prune old actor epochs, maybe.

I think a customer issue does bump this up in priority though.