You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Paul Brett <pb...@twitter.com.INVALID> on 2015/07/16 21:31:55 UTC
Re: [jira] [Commented] (MESOS-3035) As a Developer I would like a
standard way to run a Subprocess in libprocess
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) <ji...@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
Re: [jira] [Commented] (MESOS-3035) As a Developer I would like a
standard way to run a Subprocess in libprocess
Posted by Marco Massenzio <ma...@mesosphere.io>.
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 <be...@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 <pbrett@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) <ji...@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
> > >
> >
>
Re: [jira] [Commented] (MESOS-3035) As a Developer I would like a
standard way to run a Subprocess in libprocess
Posted by Benjamin Mahler <be...@gmail.com>.
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 <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 <pb...@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) <ji...@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
> >
>
Re: [jira] [Commented] (MESOS-3035) As a Developer I would like a
standard way to run a Subprocess in libprocess
Posted by Marco Massenzio <ma...@mesosphere.io>.
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 <pb...@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) <ji...@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
>