Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug(snapshot): Fix unwriten entries in multiple snapshotting Fixes #823 #825

Merged
merged 2 commits into from
Feb 20, 2023

Conversation

adiholden
Copy link
Collaborator

@adiholden adiholden commented Feb 20, 2023

When traversing over buckets for snapshoting we deside to serialize bucket if its version is lower than snapshot version,
after we serialize the bucket we update the snapshot version.
When we have 2 snapshots S1 and S2 running at the same time with versions v1 , v2 where v1 < v2 and
S2 reaches bucket B before S1, then S1 will skip serializing the bucket .

My fix:
In BucketSaveCb called from the snapshot main loop, we first trigger calls of OnDbChange for all snapshots with version less than the current flow version. This way we make sure that the snapshot with higher version will not overide the bucket version before the snapshot with lower version will serialize it

Fixes #823

…some entries isuee #823

Signed-off-by: adi_holden <adi@dragonflydb.io>
@romange
Copy link
Collaborator

romange commented Feb 20, 2023

Please shorten your commit title and move the details to the body. Also add Fixes #... to reference the issue

@@ -278,6 +278,8 @@ class DbSlice {
//! at a time of the call.
uint64_t RegisterOnChange(ChangeCallback cb);

void CallChangeOnAllLessThanVersion(DbIndex db_ind, PrimeIterator it, uint64_t version);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the name could be improved :)
void ApplyChangesToPreviousSnapshots(..., uint64_t upper_bound_ver);

void DbSlice::CallChangeOnAllLessThanVersion(DbIndex db_ind, PrimeIterator it, uint64_t version) {
uint64_t bucket_version = it.GetVersion();
// change_cb_ is ordered by version.
for (const auto& ccb : change_cb_) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a bug here if ccb.second preempts and some other snapshot (un)registers to change_cb_.
changing change_cb_ type to map might fix the issue.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vesion -> version

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our callbacks do not preempt. We should not allow for preemptable callbacks as they are getting iterator and it could be invalidated if there is a preempt.

Copy link
Contributor

@dranikpg dranikpg Feb 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

our callbacks can preempt when writing to the channel, when calling PushBytesToChannel

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dranikpg you are right. They can preempt when pushing to channel. In this case we should check if we dont have a bug in the flow, as we are using iterator..

Signed-off-by: adi_holden <adi@dragonflydb.io>
@adiholden adiholden changed the title bug(snapshot): On bucket save call dbslice registered cb with version less than runing snapshot version #823 bug(snapshot): Fix unwriten entries in multiple snapshotting Fixes #823 Feb 20, 2023
@adiholden adiholden requested a review from romange February 20, 2023 12:34
@romange romange merged commit d738d8d into main Feb 20, 2023
@romange romange deleted the snapshot_version_bug branch February 20, 2023 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple snapshots at the same time skips serializing some entries
3 participants