Generalize work item definition in BackupEngineImpl #13228
Closed
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
…epresenting the intended underlying action(s)
|
@mszeszko-meta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@mszeszko-meta has updated the pull request. You must reimport the pull request before landing. |
|
@mszeszko-meta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
mszeszko-meta
force-pushed
the
generalize_backup_engine_work_item
branch
from
54fb17f to
e7aa0bc
Compare
|
@mszeszko-meta has updated the pull request. You must reimport the pull request before landing. |
|
@mszeszko-meta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
pdillinger
approved these changes
Dec 19, 2024
| @@ -595,8 +595,29 @@ class BackupEngineImpl { | |||
| Temperature file_temp, RateLimiter* rate_limiter, | |||
| std::string* db_id, std::string* db_session_id); | |||
|
|
|||
| struct CopyOrCreateResult { | |||
| ~CopyOrCreateResult() { | |||
| struct WorkItemResult { | |||
There was a problem hiding this comment.
It looks like the current plan is to have WorkItemResult and WorkItem contain all the fields for the various types of work item. This is probably ok for the foreseeable future but std::variant would likely be better if things become more fragmented.
There was a problem hiding this comment.
Good callout!
|
@mszeszko-meta has updated the pull request. You must reimport the pull request before landing. |
mszeszko-meta
force-pushed
the
generalize_backup_engine_work_item
branch
from
76cbdcd to
a774bfb
Compare
|
@mszeszko-meta has updated the pull request. You must reimport the pull request before landing. |
|
@mszeszko-meta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
pdillinger
reviewed
Dec 20, 2024
| } | ||
| work_item.result.set_value(std::move(result)); | ||
| } else { | ||
| result.io_status = IOStatus::InvalidArgument( |
There was a problem hiding this comment.
I believe this result gets dropped. Maybe move the work_item.result.set_value(...) above to after the else block.
There was a problem hiding this comment.
Fix coming up in #13238. Thanks for catching it, Peter!
|
@mszeszko-meta merged this pull request in f7b4216. |
This was referenced Dec 20, 2024
facebook-github-bot
pushed a commit
that referenced
this pull request
Dec 20, 2024
Summary: Followup to #13228. This fix is not a critical one in a sense that `else`-branch is only supposed to act as a guard just in case when new work item type is being introduced, scheduled but not handled. However, we're in control of the work item types and currently we only support a single one (which has appropriate handling logic to it). Pull Request resolved: #13238 Reviewed By: pdillinger Differential Revision: D67512001 Pulled By: mszeszko-meta fbshipit-source-id: 71e74b3dac388882dd3757871f500c334667fbd1
facebook-github-bot
pushed a commit
that referenced
this pull request
Jan 18, 2025
Summary: With this change we are adding native library support for incremental restores. When designing the solution we decided to follow 'tiered' approach where users can pick one of the three predefined, and for now, mutually exclusive restore modes (`kKeepLatestDbSessionIdFiles`, `kVerifyChecksum` and `kPurgeAllFiles` [default]) - trading write IO / CPU for the degree of certainty that the existing destination db files match selected backup files contents. New mode option is exposed via existing `RestoreOptions` configuration, which by this time has been already well-baked into our APIs. Restore engine will consume this configuration and infer which of the existing destination db files are 'in policy' to be retained during restore. ### Motivation This work is motivated by internal customer who is running write-heavy, 1M+ QPS service and is using RocksDB restore functionality to scale up their fleet. Given already high QPS on their end, additional write IO from restores as-is today is contributing to prolonged spikes which lead the service to hit BLOB storage write quotas, which finally results in slowing down the pace of their scaling. See [T206217267](https://www.internalfb.com/intern/tasks/?t=206217267) for more. ### Impact Enable faster service scaling by reducing write IO footprint on BLOB storage (coming from restore) to the absolute minimum. ### Key technical nuances 1. According to prior investigations, the risk of collisions on [file #, db session id, file size] metadata triplets is low enough to the point that we can confidently use it to uniquely describe the file and its' *perceived* contents, which is the rationale behind the `kKeepLatestDbSessionIdFiles` mode. To find more about the risks / tradeoffs for using this mode, please check the related comment in `backup_engine.cc`. This mode is only supported for SSTs where we persist the `db_session_id` information in the metadata footer. 2. `kVerifyChecksum` mode requires a full blob / SST file scan (assuming backup file has its' `checksum_hex` metadata set appropriately, if not additional file scan for backup file). While it saves us on write IOs (if checksums match), it's still fairly complex and _potentially_ CPU intensive operation. 3. We're extending the `WorkItemType` enum introduced in #13228 to accommodate a new simple request to `ComputeChecksum`, which will enable us to run 2) in parallel. This will become increasingly more important as we're moving towards disaggregated storage and holding up the sequence of checksum evaluations on a single lagging remote file scan would not be acceptable. 4. Note that it's necessary to compute the checksum on the restored file if corresponding backup file and existing destination db file checksums didn't match. ### Test plan ✅ 1. Manual testing using debugger: ✅ 2. Automated tests: * `./backup_engine_test --gtest_filter=*IncrementalRestore*` covering the following scenarios: ✅ * Full clean restore * Integration with `exclude files` feature (with proper writes counting) * User workflow simulation: happy path with mix of added new files and deleted original backup files, * Existing db files corruptions and the difference in handling between `kVerifyChecksum` and `kKeepLatestDbSessionIdFiles` modes. * `./backup_engine_test --gtest_filter=*ExcludedFiles*` ✅ * Integrate existing test collateral with newly introduced restore modes Pull Request resolved: #13239 Reviewed By: pdillinger Differential Revision: D67513875 Pulled By: mszeszko-meta fbshipit-source-id: 273642accd7c97ea52e42f9dc1cc1479f86cf30e
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.
Summary
This change refactors existing
CopyOrCreateWorkItemasync task definition to a more generic one (WorkItem) with an assignedtypeindicative of intended action. This would allow us to reuse existing, battle-tested async tasks initialization code to handle wider range of incoming use cases in B/R space.Motivation
Historically, the two main use cases for
BackupEngineImpl's async work items were either creating a file in backup workflow or copying files in restore workflow. However, as we're now exploring opportunities in incremental restore (and potentially speeding up backup verification), we need the work item abstraction to be capable of processing different workflow types concurrently (computing checksum comes to mind).Test plan
Since this is purely cosmetic change where behavior remains intact, existing test collateral will suffice.