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

fix(server): Fix bug in flushing in snapshot #654

Merged
merged 4 commits into from
Jan 8, 2023

Conversation

dranikpg
Copy link
Contributor

@dranikpg dranikpg commented Jan 8, 2023

The bug was here:

  VLOG(2) << "FlushDefaultBuffer " << bytes.size() << " bytes";
  PushBytesToChannel(current_db_, bytes);
  default_serializer_->Clear();

We might block on the channel when pushing. If the snapshot continues serializing, it will write to the default serializer. And once the channel unblocked, the clear will remove everything - not only the part it has written, but also the new one.

The solution would be to make Clear consume only a fixed part - but instead I decided to revert some changes I made previously as well because of the changes in async streamer.

I initially abstracted flushing in snapshot to pushing just bytes taken from the rdb serializer. But since in the async streamer we're gonna make the streamer base to act as a sink as well, we can just use FlushToSink for everyhing, so I removed Flush and make it just PrepareFlush.

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
@dranikpg dranikpg requested a review from romange January 8, 2023 10:53
Copy link
Collaborator

@romange romange left a comment

Choose a reason for hiding this comment

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

Do I understand correctly that now we copy everything twice?

src/server/rdb_save.cc Outdated Show resolved Hide resolved
@dranikpg
Copy link
Contributor Author

dranikpg commented Jan 8, 2023

Do I understand correctly that now we copy everything twice?

No, it works just like before - internal buffer -> string file -> moved out to channel. We had a embedded string file, but it doesn't matter if its owned and moved out or created from scratch, because its filled with one flush.

@romange romange merged commit 3da6bd9 into dragonflydb:main Jan 8, 2023
@dranikpg dranikpg deleted the fix-snapshot-buf branch February 27, 2023 16:39
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.

2 participants