Add an internal API MemTableList::GetEditForDroppingCurrentVersion #13001
Conversation
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| } | ||
| edit_list.push_back(&wal_deletion); | ||
| #else | ||
| edit = GetDBRecoveryEditForObsoletingMemTables( |
There was a problem hiding this comment.
Copy-paste between debug-only and release-only code tempts the code to dangerously diverge, but it is only a couple of lines and temporary.
There was a problem hiding this comment.
Thanks for this note! I was trying to split the work of atomic replacement into a few standalone pieces that can be reviewed separately. This internal API is one such piece but it's a dilemma to get it tested before implementing atomic replacement, so I added this temporary debug code.
Thanks for calling out the danger of diverging release mode vs debug mode. I'm gonna change this to make debug mode random chose between the two.
There was a problem hiding this comment.
There is a small inherent danger in doing something different in release mode vs. debug mode. We prefer to keep it to "extra checks" in debug mode that you can essentially prove don't affect other operations (assuming the checks pass).
Oddly, I would be less concerned if debug mode randomly chose between the two. Both would be covered then. Or you could use the code in one of our build variants like folly or ASSERT_STATUS_CHECKED or COERCE_CONTEXT_SWITCH, etc.
jowlyzhang
force-pushed
the
internal_memtable_apis
branch
from
34d70cb
to
93cc4cd
Compare
|
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
@pdillinger Thank you for the quick review and the suggestion! I have updated the code to only exercise this path in ASSERT_STATUS_CHECKED mode. |
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@jowlyzhang merged this pull request in 43bc71f. |
Prepare this internal API to be used by atomic data replacement. The main purpose of this API is to get a
VersionEditto mark the entire currentMemTableListVersionas dropped. Flush needs the similar functionality when installing results, so that logic is refactored into a util functionGetDBRecoveryEditForObsoletingMemTablesto be shared by flush and this internal API.To test this internal API, flush's result installation is redirected to use this API when it is flushing all the immutable MemTables in debug mode. It should achieve the exact same results, just with a duplicated
VersionEdit::log_numberfield that doesn't upsets the recovery logic.Test plan:
Existing tests