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?
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.
(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?
(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?
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.
Anyway, I will let benh shepherd this, but just wanted to share some
feedback.
On Thu, Jul 16, 2015 at 5:06 PM, Marco Massenzio <[email protected]>
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 <[email protected]>
> 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) <[email protected]>
> > 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
> >
>
