Implement obsolete file deletion (GC) in follower #12657
Conversation
|
@anand1976 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
db/db_impl/db_impl_follower.cc
Outdated
| JobContext purge_files_job_context(0); | ||
| { | ||
| InstrumentedMutexLock lock_guard(&mutex_); | ||
| // Currently, secondary instance does not own the database files, thus it |
There was a problem hiding this comment.
nit: it seems like this comment needs update.
| } | ||
| follower_filenums.insert(test::GetFileNumber(name)); | ||
| } | ||
| db_filenums.merge(follower_filenums); |
There was a problem hiding this comment.
IIUC, this is checking that the files on the follower is a superset of the files on the leader. Should the relationship be reversed? Since some intermediate files that may still exist on leader are not gonna be linked (or strictly speaking, linked and immediately removed) when follower jump forward and just installs the newest Version. In other cases where either the follower fails to install a Version or installs as many Versions as the leader do. Since follower can only link a file that's already present on the leader, so leader's files should be also a superset of the follower's files too. Is that right?
In order to check follower correctly cleans up obsolete files, maybe checking this relationship holds is not sufficient. Would it make sense for this verification method to take the current storage info of the follower and verify the directory contains no more and no less file?
There was a problem hiding this comment.
This is checking that the leader and follower files are identical. std::set::merge will move any element in follower_filenums that's not in db_filenums. Let's say either or both directories have some unique files, after the merge db_filenums will be a superset of follower_filenums. If they're equal in size, then the files in both are identical, since otherwise merge would have moved the different to db_filenums.
When CheckDirs() is called, both leader and follower are expected to be in their respective final states, so there shouldn't be any intermediate or obsolete files on the leader. Let's say the follower doesn't delete an intermediate file due to a bug, then it wouldn't be present in the leader directory and this check would fail.
There was a problem hiding this comment.
Ah, I see. Thanks for the detailed explanation. I didn't notice how merge is also updating follower_filenums. Checking them to be exactly identical makes sense.
| // This test creates 4 L0 files, immediately followed by a compaction to L1. | ||
| // The follower replays the 4 flush records from the MANIFEST unsuccessfully, | ||
| // and then successfully recovers a Version from the compaction record | ||
| TEST_F(DBFollowerTest, RetryCatchup) { |
There was a problem hiding this comment.
This test and a few other tests currently is not CheckDirs, should it be added to all of them?
db/db_follower_test.cc
Outdated
| ASSERT_OK(Put("k1", "v4")); | ||
| ASSERT_OK(Flush()); | ||
|
|
||
| // FlushOptions fo; |
There was a problem hiding this comment.
nit:Are these commented out test lines outdated?
| Reopen(opts); | ||
| ASSERT_OK(OpenAsFollower()); | ||
|
|
||
| follower_fs()->BarrierInit(2); |
There was a problem hiding this comment.
I didn't quite get what these Barrier* functions helped to achieve, would you mind adding a bit clarification?
There was a problem hiding this comment.
Sure. I assume you're asking about this particular test rather than the Barrier* functions in general. So in this test, they ensure that the follower sees the next 4 L0 files and is able to link them, and then sees the compaction that obsoletes those L0 files (so those L0 files are intermediates that it has to explicitly delete). Suppose we don't have any barriers, its possible the follower reads the L0 records and compaction records from the MANIFEST in one read, which means those L0 files would have already been deleted by the leader and the follower cannot link to them.
|
@anand1976 has updated the pull request. You must reimport the pull request before landing. |
|
@anand1976 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@anand1976 merged this pull request in 0ed9355. |
This PR implements deletion of obsolete files in a follower RocksDB instance. The follower tails the leader's MANIFEST and creates links to newly added SST files. These links need to be deleted once those files become obsolete in order to reclaim space. There are three cases to be considered -
VersionSettopending_outputs_to prevent their deletion.Version. These are deleted as usual inPurgeObsoleteFiles.VersionEditand deleted by a subsequentVersionEdit, both processed in the same catchup attempt. Links will be created for the new files when verifying a candidateVersion. Those need to be deleted explicitly as they're never added toVersionStorageInfo, and thus not deleted byPurgeObsoleteFiles.Test plan -
New unit tests in
db_follower_test.