Fix needs_flush assertion in file ingestion
#13045
Conversation
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
jowlyzhang
force-pushed
the
fix_overflow
branch
from
27f61e2
to
f306b73
Compare
|
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
jowlyzhang
force-pushed
the
fix_overflow
branch
from
f306b73
to
17bed99
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. |
|
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
| // Check if history trim is needed first, so that we can avoid installing a | ||
| // new MemTableListVersion without installing a SuperVersion (installed based | ||
| // on return value of this function). | ||
| if (!current_->HistoryShouldBeTrimmed(usage)) { |
There was a problem hiding this comment.
This change relies on the current SuperVersion to always quickly catch up and point to the most recent MemTableListVersion. I did a code audit and found this case where this is not guaranteed, so made this change to make it do the same. This also addressed a comment in the original change: https://github.com/facebook/rocksdb/pull/7296/files#r474976458
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This PR adds a flush option
wait_for_results_readableto make flush wait a bit further until the flushed files are available for reading. And use this option in file ingestion's flush. Manual flush can use this option too if this is what they need.File ingestion job's second
NeedsFlushcall could have been invoked when the memtables are flushed but the SuperVersion hasn't been updated yet, triggering the assertion.Test plan:
Existing tests
Manually stress tested