mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marco Massenzio" <>
Subject Re: Review Request 36424: Created a command executor helper method.
Date Fri, 17 Jul 2015 05:49:58 GMT

This is an automatically generated e-mail. To reply, visit:

>From an email thread [here](

> (1) Discard semantics: with subprocess the caller has the ability to kill
 the process through However, with 'execute' that you've introduced,
 if the caller discards the future, we should be killing the process tree
 we've forked to clean up.

Nice catch - I may need some guidance on how to do that, but I'll give it a

> (2) As a general note, `Future<Try<string>>` as a type tends to be a bit
 confusing for folks as one has to resort to reading comments to figure out
 the difference between f.isFailed() and f.get().isError(). When we talk
 about "readability" we're generally including things like this, where it
 isn't obvious to the "reader" of the code. Similar to paul's approach, I'd
 suggest having a small struct to capture the data of a completed command
 (i.e. status, out, err) to make this very clear. e.g. `Future<Output>`
 Make sense?

I was actually thinking about even creating a Protobuf for this (some kind
of CommandResultInfo) and use that.

> (3) Subprocess has two overloads, path+args (exec-style), and command (sh
> -c "command" style). Your documentation says "sh -c" but the code is just
> directly passing through to path+args subprocess, so it's not going through
> sh -c at all. Something I'm missing?

Bad comment (I'd originally started down the 'sh -c' path, then figured out
it was not strictly necessary).
I'll fix that.

> Lastly, it would be great to see the caller code related to this, spot
 checking I see a number of places that are unnecessarily tedious (e.g.
     // Start reading from stdout and stderr now. We don't use stderr
     // but must read from it to avoid the subprocess blocking on the
     // pipe.

     // Wait for the process to exit.
       .onAny(defer(self(), &Self::_sample, lambda::_1));
> vs
       .then(defer(self(), &Self::_sample, lambda::_1));
> There is no need to wait for the command first before continuing, can just
 wait for everything. Having something like what you're proposing seems even
 cleaner, but it would be helpful to first start with the above improvement
 to get a better sense of all the use cases, and whether just adding a
 `.join()` that returns `Future<Output>` gets us all the way there. I propose
 this first because the structural simplifications needed will be very
 similar to those from `process::executeCommand`.

Uhm - this is assuming a deeper understanding of libprocess than I
currently have: let me ponder this a bit and I may reach out for more
And, yes, my code was largely based on `perf.cpp` blueprint.

- Marco Massenzio

On July 14, 2015, 4:21 a.m., Marco Massenzio wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> -----------------------------------------------------------
> (Updated July 14, 2015, 4:21 a.m.)
> Review request for mesos, Benjamin Hindman and Cody Maloney.
> Bugs: MESOS-3035
> Repository: mesos
> Description
> -------
> Jira: MESOS-2902
> While researching how to execute an arbitrary script
> to detect the Master IP address, it emerged clearly that
> a helper method to execute an arbitrary command/script on
> a node and obtain either stdout or stderr would have been
> useful and avoided a lot of code repetition.
> This could not be ultimately used for the purpose at hand,
> but I believe it to be useful enough (particularly, to avoid
> people doing "coding by copy&paste" and/or waste time
> researching the same functionality).
> This would also be beneficial in MESOS-2830, factoring out the remote command execution
> Diffs
> -----
>   3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6

>   3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4

> Diff:
> Testing
> -------
> make check
> Thanks,
> Marco Massenzio

  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message