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 Tue, 10 Nov 2015 20:13:15 GMT


> On Nov. 7, 2015, 1:06 a.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 91
> > <https://reviews.apache.org/r/37336/diff/10/?file=1117802#file1117802line91>
> >
> >     How come `Invocation` is within `Subprocess::Result`? Wouldn't it make more
sense for it to be at the `Subprocess` level to be `Subprocess::Invocation`?

I probably misunderstood your earlier comment.
Moved one level up, into `Subprocess`.


> On Nov. 7, 2015, 1:06 a.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 278
> > <https://reviews.apache.org/r/37336/diff/10/?file=1117802#file1117802line278>
> >
> >     We tend to not use `getX()` style naming for accessors. How about `outData()`?

great point (wasn't sure, as it's not spelled out in our style guide - and it's not truly
a "getter" in the Google sytle sense: `foo_` -> `foo()`)
also renamed `stderr_` to `errData` and `getStderr()` to `errData()` - please let me know
if that was "overreach"!


> On Nov. 7, 2015, 1:06 a.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 340
> > <https://reviews.apache.org/r/37336/diff/10/?file=1117802#file1117802line340>
> >
> >     I think this violates our use of `auto` rule. Could you please spell out the
type?

I would disagree here... the type is `Try<std::list<os::ProcessTree>>` and adding
that, I don't think makes the code any more readable (on the contrary).
In fact, we don't really make use of any of that - all we care is that no error is returned:
it could very well be a `Try<Pizza>` for all we care :)

This seems to me a poster child of the use-case for `auto`... anyways, no biggie, I've added
the type.


> On Nov. 7, 2015, 1:06 a.m., Michael Park wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, lines 202-203
> > <https://reviews.apache.org/r/37336/diff/10/?file=1117803#file1117803line202>
> >
> >     Why do we need the `promise` stuff here?
> >     
> >     Does something like the following not work?
> >     
> >     ```cpp
> >     Future<tuple<Future<Option<int>>, Future<string>, Future<string>>>
result =
> >       await(status(), getStdout(), getStderr());
> >     
> >     return result.then([=](...) { ... })
> >                  .onFailed([=](...) { ... })
> >                  .onDiscard([this]() { cleanup(); });
> >     ```

I don't quite remember what it was that I came across originally when I tried the above (sans
Promise) then Joris suggested the pattern that you see here now, and this works. IIRC I could
never get the `onFailed` to get invoked for failed commands, so the pending process never
got cleaned up.

If you feel strongly this should not be here (and/or are confident the pattern above works)
I'm happy to have another go at it.


- Marco


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


On Nov. 6, 2015, 6:24 a.m., Marco Massenzio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37336/
> -----------------------------------------------------------
> 
> (Updated Nov. 6, 2015, 6:24 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
> -------
> 
> 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<Subprocess::Result>`.
>  
> `Subprocess::Result`, also introduced with this patch, contains useful information
> about the command invocation (an `Invocation` struct); 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 efe0018d0414c4137fd833c153eb262232e712bc 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp ac600a551fb1a7782ff33cce204b7819497ef54a

> 
> Diff: https://reviews.apache.org/r/37336/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> (also tested functionality with an anonymous module that exposes an `/execute` endpoint
and runs arbitrary commands, asynchronously,
> on an Agent)
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>


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