-
-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make transform application transactional #2465
Conversation
da1edec
to
d169fbe
Compare
d169fbe
to
3a7c4f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We paired on this and tested it extensively by crashing intentionally in various places.
@@ -23,6 +23,26 @@ | |||
|
|||
namespace vast::system { | |||
|
|||
caf::expected<atom::done> | |||
posix_filesystem_state::rename_single_file(const std::filesystem::path& from, | |||
const std::filesystem::path& to) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't rename this more than a single file? Just curious why not go with a plain filesystem rename, which should work for directories as well, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the question. Are you proposing to change the name OR the semantics of this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name only, because it actually renames directories as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Checklist
🎯 Review Instructions