feat(snapshot): add support for snapshot recovery from S3 #1839
Conversation
andydunstall
force-pushed
the
feature/add-load-snapshot-from-s3
branch
from
b84c68f to
f3c02c6
Compare
|
Looks good. Do we have any testing strategy for this? |
I haven't added tests as part of this PR though added a ticket to add before we make S3 snapshots widely available or use in control plane (https://github.com/dragonflydb/controlplane/issues/250) |
src/server/detail/snapshot_storage.h
Outdated
| virtual std::string LoadPath(const std::string_view& dir, const std::string_view& dbfilename) = 0; | ||
| virtual io::Result<std::string, GenericError> LoadPath(const std::string_view& dir, | ||
| const std::string_view& dbfilename) = 0; |
There was a problem hiding this comment.
Small side note, its okay to pass string_views by value 😅
There was a problem hiding this comment.
and please also use them instead of const string& below where its possible
There was a problem hiding this comment.
Sure, out of interest why is string_view prefered to const std::string&? (doesn't that risk unintentionally modifying the underlying string?)
There was a problem hiding this comment.
string_view as an argument in function is usually better because it accepts both strings and const char* without doing any allocations. It's like a decorator around an immutable string passed to a function. When you use const string& it accepts strings of course but if you pass "foobar" it will create a temporary string object during the call
src/server/server_family.cc
Outdated
| auto paths_result = snapshot_storage_->LoadPaths(load_path); | ||
| if (!paths_result) { | ||
| LOG(ERROR) << "Failed to load snapshot: " << paths_result.error().Format(); | ||
|
|
||
| Promise<std::error_code> ec_promise; | ||
| ec_promise.set_value(paths_result.error()); | ||
| return ec_promise.get_future(); |
There was a problem hiding this comment.
It's a generic error, right? Unfortunately it can be the case, that if created just with a string, its error_code will be a default-value (not set), so the result of Load will actually return that no error happened. You do so for example with "Invalid S3 Path", so I assume its a possible bug
There was a problem hiding this comment.
Oops - looks like the Load() returned error is currently ignored - though could update that to return a GenericError too?
andydunstall
force-pushed
the
feature/add-load-snapshot-from-s3
branch
from
29c604e to
542c0ff
Compare
Thanks for reviewing - yep kept separate just as debug command sets load path explicitly Had to rebase - mind re-approving if you get a sec :) |
Fixes https://github.com/dragonflydb/controlplane/issues/227.
Adds support for loading snapshots from S3. The Helio S3 client still needs some work to make it more reliable, though this at least gets loading snapshots working.
AwsS3SnapshotStorage::OpenReadFileis the same asAwsS3SnapshotStorage::OpenWriteFile, so the main changes here are addingLoadFileandLoadFilesimplementations for S3To match the correct snapshot, unlike
FileSnapshotStorage::LoadFilewe can't use globs, so this instead uses a regex for the timestamp and shard number.(Note looked at combining
LoadFileandLoadFilesto avoid repeated calls to S3, but since theDEBUGcommand allows explicitly specifying the load path its easier to keep separate. Could cache but doesn't seem worth it for 1 extra call)Also refactors logging and error handling so both
LoadPathandLoadPathsreturn aGenericErrorthen log the error in one place, rather than logging spread out among arbitrary functions which is hard to follow and easy to miss or forget to update. (Its a bit more verbose though I think its cleaner - can revert if prefered)Successful load:
Not found:
Error:
Tasks