-
Notifications
You must be signed in to change notification settings - Fork 120
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
Introduce SuperSnapshot #292
Conversation
Just realized I also need to add support for MultiGet, adding that now. |
3664001
to
113f820
Compare
Added MultiGet support together with the unit test. |
read_options.background_purge_on_iterator_cleanup || | ||
immutable_db_options_.avoid_unnecessary_blocking_io); | ||
internal_iter->RegisterCleanup(CleanupSuperVersionHandle, cleanup, nullptr); | ||
// Do not clean up the super version if super snapshot owns it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we assuming that the super_version passed to NewInternalIterator is always the same as read_options.snapshot.sv_ here?
If so, how is that guarantee maintained?
If not, why can't we cleanup super_version here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asking because NewInternalIterator
is called in multiple places, but seems we only maintain the assumption in DBImpl::NewIteratorImpl
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is the assumption here, and you're right that it's not respected in some scenarios (like iterator refresh, although iterator refresh is also not supported if snapshot is non-nullptr). I wll add a check to only avoid cleaning up if super_version
is not the same as read_options.snapshot.sv_
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed this comment in latest version of the code.
Summary: SuperSnapshot is a combination of a snapshot and a super-version. They fulfill the same goal as snapshots, i.e. provide a consistent view of the database. However, unlike snapshots, they also pin the current super-version (memtable, immutable memtable list and file LSM tree). In that way they are similar to iterators, which also pin the current super-version for the duration of their lifetime. Test Plan: Added a basic unit test. Reviewers:
113f820
to
febf35a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
Summary:
SuperSnapshot is a combination of a snapshot and a super-version. They
fulfill the same goal as snapshots, i.e. provide a consistent view of
the database. However, unlike snapshots, they also pin the current
super-version (memtable, immutable memtable list and file LSM tree). In
that way they are similar to iterators, which also pin the current
super-version for the duration of their lifetime.
Test Plan:
Added a basic unit test.
Reviewers: