-
Notifications
You must be signed in to change notification settings - Fork 1
Add continuous transfer mode #17
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
base: master
Are you sure you want to change the base?
Conversation
77988c7
to
8d1505c
Compare
085add4
to
589871c
Compare
Ooh, nice! I didn’t review in depth yet, but one thing that comes to mind, our goal for this is to sync a directory, where the sending side may still be producing new files, right? So we sync once, but by the time it’s complete, there may be new files, so we do it again, etc., until the difference is very small, then we stop the process that’s writing files, we do a final sync, and at that point we know the copy is complete. With the current implementation, would we need to execute |
In my use-case it's 2 times.
The transfer is usually much faster than the speed of producing files, so I have not observed this being very useful for the workloads I have worked with so far. There is are some potential risks:
Yes, considering we would not keep the Edit: this is now done and tested. |
84969ff
to
67b7bbe
Compare
d9a1c4e
to
737e2db
Compare
1ede88c
to
7c49b26
Compare
src/main.rs
Outdated
// Check for auto-accept environment variable (useful for testing) | ||
if std::env::var("FASTSYNC_AUTO_ACCEPT").is_ok() { | ||
println!("Receiving will overwrite existing files with those names. Continue? [y/N] y"); | ||
return Ok(()); | ||
} |
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.
Should this be a command line option or documented env variable? I think option would be better choice and proper documentation. I would make it separate commit though.
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.
Let’s not mix env vars and CLI flags for configuration, I would put it in the CLI flags then.
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.
Thank you for highlighting.
I've removed this option now and using #[cfg(not(test))]
for the part of the code where the tests would be blocked by expecting user input.
This reduces the size of the current diff.
With my use-case with fastsync
, I don't use it in non-interactive environments (as it requires synchronized action on 2 hosts).
If you think it would be a benefit to have this CLI flag now, I can add it back here or in another PR.
fn set_file_mtime(path: &str, mtime: u64) -> Result<()> { | ||
let file = OpenOptions::new().write(true).open(path)?; | ||
|
||
let system_time = std::time::UNIX_EPOCH + std::time::Duration::from_secs(mtime); | ||
let times = FileTimes::new() | ||
.set_accessed(system_time) | ||
.set_modified(system_time); | ||
|
||
file.set_times(times)?; | ||
Ok(()) | ||
} |
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.
Right, I wrote above that ctime should be checked too, but ctime cannot be set directly, so I guess check should be if src ctime is newer than dest ctime.
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.
However I think as-is with mtime and no ctime it's fine too. I don't have strong feelings. I would just write it that way myself as I know some (I don't remember which) backup software checks ctime and not mtime, but backup software has different constrains, requirements and the nature of files and directory sets.
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.
Thank you, I agree that the fact that ctime
can't be changed is useful for some archival / security perspective.
With fastsync continuous mode I was mostly concerned with keeping the 2 hosts "in sync continously".
The main reason to use mtime
was that it can be set on the file (matching source and destination).
In combination with file size (and filename) this gives an acceptable signal that the files will be the same (without having to rery on extra metadata).
9e87bd6
to
99334f2
Compare
Allows the transfer to happen continuously. Files can change on the source and will be picked up and transferred as long as fastsync is running. This is also means fastsync now supports incremental transfers i.e. stopping the operation and resuming it later. Closes: #16
99334f2
to
dc345e0
Compare
Closes: #16