Add BackupEngine feature to exclude files #11030
Conversation
Summary: Previously, the "latest" valid backup would not be updated on delete. Test Plan: unit test included (added to an existing test for efficiency)
Summary: We have a request for RocksDB to essentially support disconnected incremental backup. In other words, if the primary backup dir is currently unavailable or limited , we should still be able to take an incremental backup relative to that primary backup dir (anticipating it will come back online) assuming some To support that, this feature allows the API user to Also in this PR: * Stacked on facebook#11029 Test Plan: unit tests added / updated
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
|
How does this approach compare to the user's Env presenting a "virtual" shared directory containing all files pooled across the various alternate_dirs? The approach in this PR appears to give flexibility to the application in deciding what files to exclude, and hides a detail that is not appropriate for the Env abstraction layer (which directory is the shared directory). A downside might be API complexity, probably for a single user feature. Are there other tradeoffs / what is the most compelling reason for this approach? |
To the BackupEngine, files in the recognized directories that are not owned by any backup are garbage files, which are not usable as is and often deleted. To present such remote files in a way that would allow them to "work" for incremental backups, without similar changes to what is done here, you would also need to create fake or unavailable backups referencing those unavailable files. Also discussing in private channels. |
utilities/backup/backup_engine.cc
Outdated
| try { | ||
| progress_callback(); | ||
| } catch (const std::exception& exn) { | ||
| io_s = IOStatus::Aborted("Exception in exclude_files_callback: " + |
There was a problem hiding this comment.
s/exclude_files_callback/progress_callback/
utilities/backup/backup_engine.cc
Outdated
| break; | ||
| } catch (...) { | ||
| io_s = | ||
| IOStatus::Aborted("Unknown exception in exclude_files_callback"); |
utilities/backup/backup_engine.cc
Outdated
| if (excludable_items != nullptr && shared && shared_checksum && | ||
| need_to_copy) { | ||
| ROCKS_LOG_INFO(options_.info_log, "Copying (if not excluded) %s to %s", | ||
| fname.c_str(), copy_dest_path->c_str()); | ||
| excludable_items->emplace_back(std::move(copy_or_create_work_item), | ||
| std::move(after_copy_or_create_work_item)); | ||
| } else { | ||
| ROCKS_LOG_INFO(options_.info_log, "Copying %s to %s", fname.c_str(), | ||
| copy_dest_path->c_str()); | ||
| files_to_copy_or_create_.write(std::move(copy_or_create_work_item)); | ||
| backup_items_to_finish.push_back( | ||
| std::move(after_copy_or_create_work_item)); | ||
| } |
There was a problem hiding this comment.
Adding work items and processing them feels a bit more scattered. What if you queue the work items no matter excludable or not (still keeping track of excludable filenames and still invoking the callback in CreateNewBackup*()), then make the worker threads wait for the exclusion list to be ready and let them use it to skip file copies, and then call AddExcludedFile for excluded files in the finishing loop?
There was a problem hiding this comment.
Right now the file copying can begin while the checkpoint is being taken. I was worried about regressing that by forcing all file copying to wait for the checkpoint to finish, even though it's probably a minor difference.
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@pdillinger merged this pull request in 02f2b20. |
Summary: We have a request for RocksDB to essentially support
disconnected incremental backup. In other words, if there is limited
or no connectivity to the primary backup dir, we should still be able to
take an incremental backup relative to that primary backup dir,
assuming some metadata about that primary dir is available (and
obviously anticipating primary backup dir will be fully available if
restore is needed).
To support that, this feature allows the API user to "exclude" DB
files from backup. This only applies to files that can be shared
between backups (sst and blob files), and excluded files are
tracked in the backup metadata sufficiently to ensure they are
restored at restore time. At restore time, the user provides
a set of alternate backup directories (as open BackupEngines, which
can be read-only), and excluded files must be found in one of the
backup directories ("included" in some backup).
This feature depends on backup schema version 2 features, though
schema version 2.0 support is not sufficient to read / restore a
backup with exclusions. This change updates the schema version to
2.1 because of this feature, so that it's easy to recognize whether
a RocksDB release supports this feature, while backups not using the
feature are fully compatible with 2.0.
Also in this PR:
recover from exceptions thrown by BackupEngine callbacks.
AsBackupEngine()function is working around thediamond hierarchy of
BackupEngineImplThreadSafeto get to theinternals, without using confusing features like virtual inheritance.
Test Plan: unit tests added / updated