feat(server): Support dynamic SAVE path #4729
Conversation
mkaruza
force-pushed
the
mkaruza/github#4660
branch
2 times, most recently
from
127fa72 to
759b39c
Compare
|
I think there is a misunderstanding about the feature, so I would like to clarify it. We need to support functionality that when dragonfly is configured with The challenge here is that the file system controller is created upon server initialization based on |
Add functionality to use SAVE and BGSAVE commands with dynamic path. Third argument to this two functions was used to change basename for snapshot, now we should also support writing to different locations (directories).~ For example, command `SAVE DF snapshots/my-snapshot` will write to directory `snapshots` with basename `my-snapshot`. If that directory doesn't exists it will be created relative to provided working directory. When cloud storage is used same logic is applied. Executing previous command snapshots will be written to `s3://bucket/my-snapshot` folder with basename `my-snapshot`. Resolves #4660 Signed-off-by: mkaruza <[email protected]>
mkaruza
force-pushed
the
mkaruza/github#4660
branch
from
759b39c to
efb4351
Compare
src/server/server_family.cc
Outdated
| } | ||
| snapshot_storage_ = std::move(gcs); | ||
|
|
||
| if (IsS3Path(flag_dir) || IsGCSPath(flag_dir)) { |
There was a problem hiding this comment.
save_stages_controller defines IsCloudPath internal function. Consider expose it and use here
src/server/server_family.h
Outdated
| // if new_version is true, saves DF specific, non redis compatible snapshot. | ||
| // if basename is not empty it will override dbfilename flag. | ||
| GenericError DoSave(bool new_version, std::string_view basename, Transaction* transaction, | ||
| GenericError DoSave(SaveCmdOptions save_cmd_opts, Transaction* transaction, |
There was a problem hiding this comment.
It's trivially copyable, IMO by value is fine. I guess the problem is that if we add a member that makes the struct non trivially copyable and then we need to remember to change that (or enforce it via static_assert so if it ever changes we are aware). Sooooo const & it is 🤣
src/server/server_family.h
Outdated
|
|
||
| void BgSaveFb(boost::intrusive_ptr<Transaction> trans); | ||
|
|
||
| GenericError DoSaveCheckAndStart(bool new_version, string_view basename, Transaction* trans, | ||
| GenericError DoSaveCheckAndStart(SaveCmdOptions save_cmd_opts, Transaction* trans, |
src/server/server_family.cc
Outdated
| } else if (IsGCSPath(ArgS(args, 1))) { | ||
| save_cmd_opts.cloud_uri = ArgS(args, 1); | ||
| } else { | ||
| // Probably basename than |
There was a problem hiding this comment.
| // Probably basename than | |
| // Probably basename then. |
src/server/server_family.cc
Outdated
| } | ||
| } | ||
|
|
||
| if (save_cmd_opts.basename.empty() && args.size() == 3) { |
There was a problem hiding this comment.
consider moving into if (args.size() >= 2) { block.
Add functionality to use SAVE and BGSAVE commands with dynamic CLOUD storage path.
New syntax should be (as discussed with @romange):
SAVE [RDB|DF] [CLOUD_URI] [BASENAME]whereCLOUD_URIshould start with S3 or GCS prefix.For example, now it should work to have working directory pointing to some local folder and executing
SAVE DF s3://bucket/snapshots my_snapshotwould save snapshots tos3://bucket/snapshotswith basenamemy_snapshot.Resolves #4660