Clone pending updates in buffered storages#7801
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
timvisee
left a comment
There was a problem hiding this comment.
(review ongoing, here are my remarks thus far)
| let mut delete_up_to = 0; | ||
| for (pending, persisted) in pending.iter().zip(changes) { | ||
| if pending != persisted { | ||
| // This should not happen, since flushers are supposed to be requested->executed | ||
| // one at a time, but if it does, it should be fine. | ||
| // | ||
| // The only consequence is to persist some operations again. | ||
| break; | ||
| } | ||
| delete_up_to += 1; | ||
| } | ||
|
|
||
| *pending = pending.split_off(delete_up_to); |
There was a problem hiding this comment.
We can probably use retain here too. As I remember it is very efficient on diffs like this. And it prevents us having to do this index magic.
Also, this indexed approach will break if we have concurrent flushers. We shouldn't have that, but you never know what happens in the future.
There was a problem hiding this comment.
I am not sure about this one, since they are vecs representing a log.
If we have a sequence like:
changes: [1, 2, 3]
pending_updates: [1, 2, 3, 4, 5, 2]
and we do something like .retain(|pending| !changes.contain(pending)), we will end up with
pending_updates: [4, 5]
which will lose the latest mapping.
There was a problem hiding this comment.
You're right. It is a log. We should keep the same ordering and retain breaks that.
The good thing is that these are idempotent. It means that if a flush didn't clear anything from the pending updates list, it's fine if we apply it again in the next flush iteration.
I do have something in mind that makes it more resilient against concurrent flushers though. I might implement that in the future.
My idea is that the beginning of the lists don't have to match.
- pending_updates:
[1, 2, 3] - flusher 1 created:
[1, 2, 3] - update comes in
- pending_updates:
[1, 2, 3, 4] - flusher 2 created:
[1, 2, 3, 4] - flusher 1 flushes:
[1, 2, 3]
pending_updates:[4] - flusher 2 flushes:
[1, 2, 3, 4]
pending_updates:[](first three were missing)
Or a slightly more complicated example:
- pending_updates:
[1, 2, 3] - flusher 1 created:
[1, 2, 3] - update comes in
- pending_updates:
[1, 2, 3, 4] - flusher 2 created:
[1, 2, 3, 4] - update comes in:
pending_updates:[1, 2, 3, 4, 5] - flusher 1 flushes:
[1, 2, 3]
pending_updates:[4, 5] - flusher 2 flushes:
[1, 2, 3, 4]
pending_updates:[5](first three were missing)
And maybe we have to enforce flushers are also applied in the same order because:
- pending_updates:
[1, 2, 3] - flusher 1 created:
[1, 2, 3] - update comes in
- pending_updates:
[1, 2, 3, 4] - flusher 2 created:
[1, 2, 3, 4] - flusher 2 flushes:
[1, 2, 3, 4]
pending_updates:[] - flusher 1 flushes:
[1, 2, 3](⚠️ we now don't write 4 at the end!)
pending_updates:[]
timvisee
left a comment
There was a problem hiding this comment.
Pre-approval for when all review remarks are resolved.
Then we can merge and test over night.
bbe2952 to
b62d3ed
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
I'm afraid this still gets missing points when running crasher locally. Let's see the effect on chaos... Merging |
* in MmapSliceBufferedUpdateWrapper * in MmapBitsliceBufferedUpdateWrapper * in MutableIdTracker's versions updates * in MutableIdTracker's mapping updates * clone updates only when non-empty * only lock for reconciling pending changes * simpler reconciling * use Mutex as argument to ensure we only lock within reconciliation
|
Noticed there were two more components that needed this treatment. I've implemented that in #7805. |
Audits and implements better delayed flushing.
This fixes the case where a flusher could lose its pending updates if the flusher closure fails or is never executed.
Instead of taking the current pending updates, and leaving them initialized, we should clone them, and reconcile them after persisting them (often times asyncronously).
This PR applies this fix to
MutableIdTracker,MmapSliceBufferedUpdateWrapper, andMmapBitsliceBufferedUpdateWrapper, which were the only storages missing this behavior in my audit.