mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jie Yu" <yujie....@gmail.com>
Subject Re: Review Request 37336: Added `execute()` method to process::Subprocess
Date Fri, 20 Nov 2015 23:28:25 GMT


> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 119-130
> > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line119>
> >
> >     Why not just use a single `int status` field here. The users can use WEXITSTATUS
... to derive those information themself. This is also consistent with other places in the
code place.
> 
> Marco Massenzio wrote:
>     `Subprocess` does exactly that (actually, it uses an `Option<int>`) - this
leaves the caller with the burned to do all the legwork: again and again and again - it's
all over the code base.
>     
>     The point of this patch is to encapsulate that work, having it done (hopefully, properly)
in **one** place and avoid to have the same code sprinkled all over the codebase (and you
can tell, most places, by copy & paste).

What if there's no returnCode (due to signal)? Should you use Option<int> returnCode
here? Similarly, should you use Option<int> for signal as well. What will the client
code be look like? Do you still need to check if (returnCode.isSome()) ... if (signal.isSome())
...

Also
1) what if I want to know if WCOREDUMP is true or not, do you need to add another boolean?
2) what if the reaper cannot reap the subprocess (i.e., we cannot get the exit status)?


- Jie


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


On Nov. 10, 2015, 8:51 p.m., Marco Massenzio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37336/
> -----------------------------------------------------------
> 
> (Updated Nov. 10, 2015, 8:51 p.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