No, no - no frustration at all :) It was just me going down the wrong path too fast, without waiting for advice. (as allegedly a Turkish proverb says: "No matter how far down the wrong path you're gone, it's never too late to turn back" - I'm hoping someone from Turkey on the list can confirm or deny :)
@Paul: please go ahead if you can't wait for this to be committed - by all means, feel free to add a TODO(marco) against your code and assign me a Jira to refactor, once this lands. (please tag it 'mesosphere' and 'tech debt' so we're sure not to drop it. Thanks!) More Comments inline. *Marco Massenzio* *Distributed Systems Engineer* On Thu, Jul 16, 2015 at 6:20 PM, Benjamin Mahler <benjamin.mah...@gmail.com> wrote: > Hey sorry for the frustration, I wrote subprocess originally but it's > evolved over time through a number of folks. Is ben going to shepherd this? > Sweet - I'll be copying your comments into the review (so we keep track of them) and will work my way through them. Yes, benh was supposed to shepherd this - but he's not feeling all that well, as you wrote originally the Subprocess, I'm kinda wondering whether you could shepherd this one? Just a thought. > Just a few higher level things to consider: > > (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. > > Anyway, I will let benh shepherd this, but just wanted to share some > feedback. > Thanks, appreciated! > > On Thu, Jul 16, 2015 at 5:06 PM, Marco Massenzio <ma...@mesosphere.io> > wrote: > > > No update, I'm afraid, still waiting for a review from the shepherd. > > Implemented all suggested changes, but before investing more time on > this, > > I want to be sure I'm on the right track. > > > > Been burned already a few times, having gone through several reviews > cycle, > > done all that was asked of me, and yet, eventually, the whole thing was > > dropped... > > > > BTW - the whole idea of this RR was to avoid having the same code, > slightly > > changed, sprinkled all over the code base (MESOS-2902 needs this too). > > > > *Marco Massenzio* > > *Distributed Systems Engineer* > > > > On Thu, Jul 16, 2015 at 12:31 PM, Paul Brett <pbr...@twitter.com.invalid > > > > wrote: > > > > > Marco > > > > > > Any progress on getting an update to this? It is a blocker on fixing > the > > > perf issues. > > > > > > Thanks > > > > > > -- Paul > > > > > > > > > On Mon, Jul 13, 2015 at 3:24 PM, Paul Brett (JIRA) <j...@apache.org> > > > wrote: > > > > > > > > > > > [ > > > > > > > > > > https://issues.apache.org/jira/browse/MESOS-3035?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14625487#comment-14625487 > > > > ] > > > > > > > > Paul Brett commented on MESOS-3035: > > > > ----------------------------------- > > > > > > > > +1 > > > > > > > > > As a Developer I would like a standard way to run a Subprocess in > > > > libprocess > > > > > > > > > > > > > > > ---------------------------------------------------------------------------- > > > > > > > > > > Key: MESOS-3035 > > > > > URL: > > https://issues.apache.org/jira/browse/MESOS-3035 > > > > > Project: Mesos > > > > > Issue Type: Story > > > > > Components: libprocess > > > > > Reporter: Marco Massenzio > > > > > Assignee: Marco Massenzio > > > > > > > > > > As part of MESOS-2830 and MESOS-2902 I have been researching the > > > ability > > > > to run a {{Subprocess}} and capture the {{stdout / stderr}} along > with > > > the > > > > exit status code. > > > > > {{process::subprocess()}} offers much of the functionality, but in > a > > > way > > > > that still requires a lot of handiwork on the developer's part; we > > would > > > > like to further abstract away the ability to just pass a string, an > > > > optional set of command-line arguments and then collect the output of > > the > > > > command (bonus: without blocking). > > > > > > > > > > > > > > > > -- > > > > This message was sent by Atlassian JIRA > > > > (v6.3.4#6332) > > > > > > > > > > > > > > > > -- > > > -- Paul Brett > > > > > >
