-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
Make compaction always use the input version with extra ref protection #12992
Conversation
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
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.
LGTM
267b3e5
to
85a8da5
Compare
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
85a8da5
to
14e4102
Compare
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@pdillinger Thank you for the quick review! |
@jowlyzhang merged this pull request in 0c6e9c0. |
Compaction
is already creating its own ref for the input Version:rocksdb/db/compaction/compaction.cc
Line 73 in 4b1d595
And properly Unref it during destruction:
rocksdb/db/compaction/compaction.cc
Line 450 in 4b1d595
This PR redirects compaction's access of
cfd->current()
to this inputVersion
, to prepare for when a column family's data can be replaced all together, andcfd->current()
is not safe to access for a compaction job. Because a newVersion
with just some other external files could be installed ascfd->current()
. The compaction job's expectation of the currentVersion
and the corresponding storage info to always have its input files will no longer be guaranteed.My next follow up is to do a similar thing for flush, also to prepare it for when a column family's data can be replaced. I will make it create its own reference of the current
MemTableListVersion
and use it as input, all flush job's access of memtables will be wired to that inputMemTableListVersion
. Similarly this reference will be unreffed during a flush job's destruction.Test plan:
Existing tests