Make flush create its own reference of MemTableListVersion #12996
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jowlyzhang
changed the title
jowlyzhang
force-pushed
the
prepare_flush_mlv_ref
branch
from
23a166f
to
80ae49a
Compare
jowlyzhang
changed the title
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
This PR is paused because we are re-evaluating the need for the atomic replacement feature. I think updating a FlushJob's contract for cleanup is still worth doing. I will get back to this PR for that part. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This extra ref is redundant now but we need it for when data replacement can happen. This PR also changes the contract for cancelling a FlushJob and adds some TODOs for the flush code path for implementing atomic replacement.
FlushJob's cancellation is for properly releasing the resources it is holding. In this PR one more resource namely the
MemTableListVersionis added into FlushJob, so I want to make sure it's properly handled. Currently, the caller of FlushJob's public APIs use a side by side boolean to track the need for cancellation on their side. That is a contract that cannot be enforced. For example, flush_job_test.cc is not doing that at all. Besides, this is also a complicated contract. There is setting that boolean to true / false in some conditions, checking it in combination with returned Status etc. That makes the contract error prone. There is also no enforcement that resources are guaranteed to be cleaned up.In this PR, we added a required output variable
need_cleanupto FlushJob's public APIs. The contract is: the user always need to callFlushJob::Cleanupas long as that boolean is set to true. Although this is yet one more output argument,FlushJob::Runalready have a few other boolean output arguments of the same pattern so it's not too much extra cognitive burden. FlushJob maintains its own internal state about resource management, and a few extra guards are added in debug mode.Test plan:
Existing tests