----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36424/#review92028 -----------------------------------------------------------
>From an email thread >[here](http://www.mail-archive.com/dev@mesos.apache.org/msg32844.html): > (1) Discard semantics: with subprocess the caller has the ability to kill the process through s.pid. 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 shot. > (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? Yes! 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. perf.cpp): ``` // 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. output.push_back(io::read(perf.get().out().get())); output.push_back(io::read(perf.get().err().get())); // Wait for the process to exit. perf.get().status() .onAny(defer(self(), &Self::_sample, lambda::_1)); ``` > vs ``` process::await( perf.get().status(), io::read(perf.get().out().get()), io::read(perf.get().err().get())) .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 advice. 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: > https://reviews.apache.org/r/36424/ > ----------------------------------------------------------- > > (Updated July 14, 2015, 4:21 a.m.) > > > Review request for mesos, Benjamin Hindman and Cody Maloney. > > > Bugs: MESOS-3035 > https://issues.apache.org/jira/browse/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 logic. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/subprocess.hpp > 310cb4f8e4e2faa5545dffd196d7490c868bc5d6 > 3rdparty/libprocess/src/tests/subprocess_tests.cpp > f6acb204582a9e696c3b09d4e4c543bb052e97d4 > > Diff: https://reviews.apache.org/r/36424/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Marco Massenzio > >
