Add parallel pull mode #256

artem-zinnatullin · 2019-05-01T07:59:18Z

Closes #188.

Finally, here is the implementation of new pull mode: parallel

In this mode Mainframer pulls files from remote machine in parallel to the remote command execution.

This mode dramatically improves total execution time.

With serial pull mode (1.x, 2.x behavior) user had to wait for all files to be pulled after the remote command finished.

Now with parallel pull mode, most of the files will be already pulled by the time remote command finishes and after it finishes last incremental pull will be done (usually ~1 second).

Results from tests/test_pulls_big_files.sh (creates 30 64MB files):

1.6x faster overall execution time!

-------- TEST STARTED (pull mode = 'parallel') test_pulls_big_files.sh --------
:: Mainframer v3.0.0-dev

Pushing...
Push done: took 0 seconds.

Executing command on remote machine (pulling in parallel)...

mkdir build && for ((i=0;i<30;i++)); do dd if=/dev/urandom of=build/buildresult-.txt bs=16M count=4 iflag=fullblock; sleep 1; done;

Execution done: took 41 seconds.
Pulling...
Pull done: took 17 seconds

Success: took 58 seconds.

-------- TEST ENDED (pull mode = 'parallel') test_pulls_big_files.sh --------

vs

-------- TEST STARTED (pull mode = 'serial') test_pulls_big_files.sh --------
:: Mainframer v3.0.0-dev

Pushing...
Push done: took 0 seconds.

Executing command on remote machine...

mkdir build && for ((i=0;i<30;i++)); do dd if=/dev/urandom of=build/buildresult-.txt bs=16M count=4 iflag=fullblock; sleep 1; done;

Execution done: took 40 seconds.
Pulling...
Pull done: took 52 seconds

Success: took 1 minute 33 seconds.

-------- TEST ENDED (pull mode = 'serial') test_pulls_big_files.sh --------

…ontinuous-sync

artem-zinnatullin · 2019-05-01T09:44:56Z

Added results from tests (see PR description)

aneeshusa

A few quick comments from a first glance - will try to find time to read through the actual interesting bits e.g. the parallelized bits later :)

src/intermediate_config.rs

aneeshusa · 2019-05-01T12:28:36Z

src/sync.rs

+use remote_command::RemoteCommandResult;
+
+#[derive(Debug, PartialEq, Clone)]
+pub enum PushResult {


Any reason not to make this a type synonym instead so you can reuse all the functionality of Result directly?

Or actually, how about something like:

Suggested change

pub enum PushResult {

pub struct PushResult {

pub duration: Duration,

pub result: Result<(), String>,

}

I think this would reflect the intent more accurately, looking at fn push() below.

The only reason to not make a synonym was to better support extension with additional fields, but I think best version would be to combine custom structs with raw Result

pub struct PushOk { pub duration: Duration } pub struct PushErr { pub duration: Duration } Result<PushOk, PushErr>

I def feel weird that enums can't have named fields, structs seem to provide more protection from accidental value mismatch

I guess you meant... PushEr...

IMO it would still be better to have a product type at the top since you'll always want the duration, either via a named struct as I suggested above or an (anonymous) tuple. This also lets you punt on custom structs for the Ok/Err cases until you have extra values there - the compiler will enable safe refactoring later. This lets you avoid adding too much complexity; e.g., the execute_rsync(&mut command) on master which is currently in your PR:

match execute_rsync(&mut command) { Err(reason) => Err(PushErr { duration: start_time.elapsed(), message: reason, }), Ok(_) => Ok(PushOk { duration: start_time.elapsed() }), }

could be just

(start_time.elapsed(), execute_rsync(&mut command))

Similarly, code like

let remote_command_duration = match remote_command_result { Err(err) => err.duration, Ok(ok) => ok.duration }; ``` goes away.

I'd like a product top-level type but I'd also like to immediately be able to distinguish its subtype (if needed)

In Kotlin (best example I have on mind) I'd do this:

sealed class PushResult(val duration: Duration) { data class Ok(override val duration: Duration): PushResult(duration) data class Err(override val duration: Duration): PushResult(duration) }

This way it's still an enum (sealed class in Kotlin) while also containing always-present data in each case.

In Rust I guess I could do something like this:

pub enum PushResult { Ok(Duration), Err(Duration) } pub trait _PushResult { fn duration(&self) -> &Duration; } pub impl _PushResult for PushResult { fn duration(&self) -> &Duration { match self { PushResult::Err(duration) => duration, PushResult::Ok(duration) => duration } } }

wdyt?

artem-zinnatullin · 2019-05-04T06:47:21Z

@aneeshusa thanks for review, that was really helpful 👍

I've addressed all your comments, don't want to waste more of your time but of course you're welcome to review again :)

aneeshusa · 2019-05-04T18:52:44Z

src/main.rs

+        args.command.clone(),
+        config.clone(),
+        sync::project_dir_on_remote_machine(&local_dir_absolute_path.clone()),
+        2


This 2 is bugging me (perhaps more than it should). Any reason execute_remote_command doesn't return the bus directly? This will also let you get rid of the pops and their accompanying unwraps.

yeah… I can't return Bus (that's what I've tried first until I got compiler yelling at me)

It's owned by closure that I pass to thread::spawn in execute_remote_command, there is an upstream issue about similar problem jonhoo/bus#19

aneeshusa · 2019-05-04T18:56:40Z

src/main.rs

-        Err(_) => exit_with_error(&format!("\nFailure: took {}.", format_duration(duration)), 1),
-        _ => println!("\nSuccess: took {}.", format_duration(duration))
+        Err(_) => {
+            match pull_result {


The branches are currently identical, can you get rid of the match?

Done, lost some compile-time safety though, but not too bad

aneeshusa · 2019-05-04T19:10:20Z

src/main.rs

+        .unwrap();
+
+    match remote_command_result {
+        Err(ref err) => eprintln!("\nExecution failed: took {}.\nPulling...", format_duration(err.duration)),


The Pulling... part of this message seems a bit misleading in the parallel case, as I'd expect one last incremental sync. Consider having separate messaging for the serial vs parallel cases.

While it's incremental, it's nontheless potentially long

In many cases remote command produces file(s) at the very end, thus parallel pulling might not be parallel

src/main.rs

src/sync.rs

aneeshusa · 2019-05-04T19:23:39Z

Cargo.toml

@@ -11,3 +11,5 @@ authors = ["Artem Zinnatullin <artem.zinnatullin@gmail.com>", "Artur Dryomov <ar
 [dependencies]
 yaml-rust = "0.4.2"
 linked-hash-map = "0.5.1"
+crossbeam-channel = "0.3"


I haven't used it in a while, but any reason not to use std::sync::mpsc?

Initially I swapped to crossbeam because I needed multi-receiver support, but then even that wasn't enough so I've also added bus

Decided to keep crossbeam for performance lol (nanoseconds matter hey!), we'll be stripping final binary #242 so it shouldn't matter which one we're using from the binary size point

aneeshusa · 2019-05-04T19:24:08Z

src/sync.rs

+use remote_command::RemoteCommandResult;
+
+#[derive(Debug, PartialEq, Clone)]
+pub enum PushResult {


IMO it would still be better to have a product type at the top since you'll always want the duration, either via a named struct as I suggested above or an (anonymous) tuple. This also lets you punt on custom structs for the Ok/Err cases until you have extra values there - the compiler will enable safe refactoring later. This lets you avoid adding too much complexity; e.g., the execute_rsync(&mut command) on master which is currently in your PR:

match execute_rsync(&mut command) { Err(reason) => Err(PushErr { duration: start_time.elapsed(), message: reason, }), Ok(_) => Ok(PushOk { duration: start_time.elapsed() }), }

could be just

(start_time.elapsed(), execute_rsync(&mut command))

Similarly, code like

let remote_command_duration = match remote_command_result { Err(err) => err.duration, Ok(ok) => ok.duration }; ``` goes away.

aneeshusa

Overall seems fine, but per #256 (comment) I see a lot of types being added with an unclear benefit - would love to this feature implemented with fewer lines of code :)

…uration

…ontinuous-sync

arturdryomov · 2019-06-25T20:13:29Z

@artem-zinnatullin, rebase?

Also expecting a lot of issues with this mode in practice 🤔

artem-zinnatullin · 2019-06-27T00:25:55Z

Rebased, all issues that we will encounter should be solvable

arturdryomov · 2019-06-27T05:06:03Z

clippy.toml

@@ -0,0 +1 @@
+type-complexity-threshold = 450


Why do you need it?

error: very complex type used. Consider factoring parts into `type` definitions --> src/sync.rs:93:47 | 93 | let (pull_finished_tx, pull_finished_rx): (Sender<Result<PullOk, PullErr>>, Receiver<Result<PullOk, PullErr>>) = unbounded(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::type-complexity` implied by `-D warnings` = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity error: very complex type used. Consider factoring parts into `type` definitions --> src/sync.rs:110:47 | 110 | let (pull_finished_tx, pull_finished_rx): (Sender<Result<PullOk, PullErr>>, Receiver<Result<PullOk, PullErr>>) = unbounded(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity error: aborting due to 2 previous errors

😭

artem-zinnatullin and others added 15 commits April 7, 2018 19:13

WIP continuous sync.

71ff23d

Sleep between syncs.

bad4749

500 millis between syncs.

69b1b52

Merge branch 'development' into az/continuous-sync

806a9f1

Fixes

56e3f7a

More progress on it

474c263

merge master

b5d5610

Switch to crossbeam channels

2ed4aeb

Add PullMode to config

0aa3a78

Handle warnings

4622234

Fix clippy

06902f8

Add separate type for remote command result

a5758a6

my god it finally compiles yooo

f85ca20

Rename sync_ stuff to push and pull

7c655da

Run all tests in two pull modes

6572462

artem-zinnatullin requested a review from arturdryomov May 1, 2019 07:59

artem-zinnatullin added 2 commits May 1, 2019 01:01

Merge branch '3.x' of github.com:buildfoundation/mainframer into az/c…

fe31d87

…ontinuous-sync

Add docs

be3b6a9

artem-zinnatullin added this to the v3.0.0 milestone May 1, 2019

Add tests with lots of big files, improve output

419b36c

artem-zinnatullin mentioned this pull request May 1, 2019

Add millis to format_duration #259

Merged

aneeshusa reviewed May 1, 2019

View reviewed changes

artem-zinnatullin added 2 commits May 3, 2019 23:30

Improve result types

3d39438

Remove RemoteCommandResult

69bdf25

artem-zinnatullin mentioned this pull request May 4, 2019

Add content verification to tests #260

Merged

aneeshusa reviewed May 4, 2019

View reviewed changes

aneeshusa approved these changes May 4, 2019

View reviewed changes

Simplify exit

8149b55

artem-zinnatullin added 4 commits May 5, 2019 15:33

Remove sleep, fix wrong duration passed to calculate_perceived_pull_d…

2e972d9

…uration

Replace while … with loop

3340f3e

Merge branch '3.x' of github.com:buildfoundation/mainframer into az/c…

8722dd1

…ontinuous-sync

ls files in test/test_pulls_big_files.sh

2129174

merge 3.x

e8ad1e2

arturdryomov approved these changes Jun 27, 2019

View reviewed changes

arturdryomov reviewed Jun 27, 2019

View reviewed changes

Update test after merge w/ upstream

b905d73

artem-zinnatullin merged commit 8454660 into 3.x Jun 27, 2019

artem-zinnatullin deleted the az/continuous-sync branch June 27, 2019 06:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parallel pull mode #256

Add parallel pull mode #256

artem-zinnatullin commented May 1, 2019 •

edited

Loading

artem-zinnatullin commented May 1, 2019

aneeshusa left a comment

aneeshusa May 1, 2019

aneeshusa May 1, 2019

artem-zinnatullin May 4, 2019

arturdryomov May 4, 2019

aneeshusa May 4, 2019

artem-zinnatullin May 5, 2019

artem-zinnatullin commented May 4, 2019

aneeshusa May 4, 2019

artem-zinnatullin May 5, 2019

aneeshusa May 4, 2019

artem-zinnatullin May 5, 2019

aneeshusa May 4, 2019

artem-zinnatullin May 5, 2019

aneeshusa May 4, 2019

artem-zinnatullin May 5, 2019

aneeshusa May 4, 2019

aneeshusa left a comment

arturdryomov commented Jun 25, 2019

artem-zinnatullin commented Jun 27, 2019

arturdryomov Jun 27, 2019

artem-zinnatullin Jun 27, 2019

-pub enum PushResult {
+pub struct PushResult {
+    pub duration: Duration,
+    pub result: Result<(), String>,
+}

Add parallel pull mode #256

Add parallel pull mode #256

Conversation

artem-zinnatullin commented May 1, 2019 • edited Loading

artem-zinnatullin commented May 1, 2019

aneeshusa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-zinnatullin commented May 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aneeshusa left a comment

Choose a reason for hiding this comment

arturdryomov commented Jun 25, 2019

artem-zinnatullin commented Jun 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-zinnatullin commented May 1, 2019 •

edited

Loading