mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marco Massenzio" <ma...@mesosphere.io>
Subject Re: Review Request 37336: Simplified the caller interface to process::Subprocess
Date Fri, 06 Nov 2015 02:58:54 GMT


> On Nov. 3, 2015, 12:55 a.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 56
> > <https://reviews.apache.org/r/37336/diff/9/?file=1113944#file1113944line56>
> >
> >     Could you explain in what situations and why the list of arguments could/would
be modified...?

I think I should remove this comment (in fact, I just did): originally, I was hoping to be
able to somehow fix the awkward situation in which `arg[0]` is just the `progname` and not
actually used as a command argument: while this is "obvious" for any C/C++ developer, it is
also undocumented in `os::shell` and leads to unexpected behavior for unaware users of `CommandInfo`
(for example).

It turns out that that's not possible without breaking a lot of existing code/frameworks,
so I guess it's here to stay.


> On Nov. 3, 2015, 12:55 a.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 120
> > <https://reviews.apache.org/r/37336/diff/9/?file=1113944#file1113944line120>
> >
> >     This suggests to me either that `Command` should be named `Invocation`, or that
this variable should be named `command`. Given that `Command` is storing both the command
and the arguments, it seems more fitting to name it `Invocation`.
> >     
> >     Additionally, I think it would make a lot of sense to simply name it `Result`
and move it into the `Subprocess` class. Similar to `Subprocess::IO`, we would have `Subprocess::Result`
which captures the result of a subprocess call.
> >     
> >     What do you think?

Done.
Renamed `Command` -> `Invocation`
Renamed `CommandResult` -> `Subprocess::Result`

Thanks for the suggestion!
(I was unhappy about naming too)


> On Nov. 3, 2015, 12:55 a.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 135-152
> > <https://reviews.apache.org/r/37336/diff/9/?file=1113944#file1113944line135>
> >
> >     Could you explain why these aren't symmetric? That is, why is `stdout_` not
a `Option<std::string>` or why is `stderr_` not a `std::string`?

It is safe to assume that every shell command will *always* emit to `stdout` (possibly an
empty string) - while, not necessarily so to `stderr`.
It is probably a "distinction without a difference" but I believe it makes the caller's code
easier to write and removes unnecessary (and tedious) `.isSome()` and `.get()` for stdout;
while allowing for the possibility that there is no `stderr` available.

Obviously, we could make `stderr_` a `string` too - but making it an `Option` seemed to me
to be more in line with Mesos' practices.


> On Nov. 3, 2015, 12:55 a.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 400
> > <https://reviews.apache.org/r/37336/diff/9/?file=1113944#file1113944line400>
> >
> >     Is there a reason why this is wrapped in a `std::shared_ptr`?

yes, because I need to pass it to the lambda and not doing so seemed to cause issues (if memory
serves).
Wrapping it in a `shared_ptr` (instead of using a `raw` pointer) is also, in my understanding,
"best practice" now in C++11?


> On Nov. 3, 2015, 12:55 a.m., Michael Park wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, lines 499-503
> > <https://reviews.apache.org/r/37336/diff/9/?file=1113945#file1113945line499>
> >
> >     Rather than exposing the `invokedWith_` member variable like this, can we introduce
a `Subprocess` constructor that takes a `Command` instead? At which point we can do something
like:
> >     
> >     `Subprocess process(path == "sh" ? Command(argv[2]) : Command(path, argv));`

well, `invokedWith_` is already "exposed" to `subprocess` - as it's a 'friend' anyway.
But I have simplified the invocation, so that it reflects closely what you suggested.


- Marco


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37336/#review104816
-----------------------------------------------------------


On Nov. 2, 2015, 7:22 a.m., Marco Massenzio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37336/
> -----------------------------------------------------------
> 
> (Updated Nov. 2, 2015, 7:22 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-3035
>     https://issues.apache.org/jira/browse/MESOS-3035
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Jira: MESOS-3035
> 
>     The original API for `process::Subprocess` still left a lot of legwork
>     to do for the caller; we have now added an `execute()` method
>     that returns a `Future<CommandResult>` (also introduced with this patch) which
>     contains useful information about the command invocation; the exit code;
>     stdout and, optionally, stderr too.
>     
>     Once the Future completes, if successful, the caller will be able to retrieve
>     stdout/stderr; whether the command was successful; and whether it received a signal
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/subprocess.hpp f17816e813d5efce1d3bb1ff1e1111850eeda3ba

>   3rdparty/libprocess/src/subprocess.cpp 459825c188d56d25f6e2832e5a170d806e257d6b 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp ac600a551fb1a7782ff33cce204b7819497ef54a

> 
> Diff: https://reviews.apache.org/r/37336/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>


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