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, 08 Sep 2015 23:26:15 GMT


> On Sept. 7, 2015, 11:14 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 103-110
> > <https://reviews.apache.org/r/37336/diff/6/?file=1044858#file1044858line103>
> >
> >     Does it make sense to aggregate these into a `Result<std::string>`? The
`Some` case would be stdout, the `Error` case would be stederr, and the `None` case would
represent not known yet?
> >     If you don't need the `None` case then you can use a `Try<std::string>`.

I like the idea and it would be a very elegant solution; unfortunately, I suspect it may not
work in the more general case.

In my experience, Linux processes emit to both stdout and stderr (sometimes, even though there
may be no error -- eg `rsync` and `tar` do, and, for non-fatal errors, they happily carry
on; and can even be "forced" to continue in the presence of errors) - so, sometimes, you need
both.

Also, bearing in mind there may be a bunch of output on `stdout` *before* the error, which
then manifests itself in `stderr`: but one really needs to look at both to really do discovery.

Does it make sense?


> On Sept. 7, 2015, 11:14 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 350-354
> > <https://reviews.apache.org/r/37336/diff/6/?file=1044858#file1044858line350>
> >
> >     The indentation is a little different from your comment in `subprocess.hpp`.
Let's be consistent between them, ideally also with the rest of the codebase if you can find
some good examples.

so, this is `subprocess.hpp` and having looked at all of them, I notice that the *NOTE* pattern
is not used; so I've changed it to `NOTE:`
However, they all seem aligned the same way (no indent).

I'll keep an eye for other places in my diff that may have different indentantion and will
fix them too.


> On Sept. 7, 2015, 11:14 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 197
> > <https://reviews.apache.org/r/37336/diff/6/?file=1044859#file1044859line197>
> >
> >     I don't think it hurts to take timeout by const ref here. It helps the reader
understand you don't intend to copy and modify it.
> >     `const Duration& timeout`.
> >     I acknowledge your have knowledge of the internal layout of the datastructure
and know it's equally cheap to copy it. If someone came along in the future and added to the
internal state of `Duration` then they wouldn't have to refactor your code :-)

You're being generous here :)
(but, really, can't think of a good reason why I didn't use a const & in the first place
- it's my go-to choice...)
Fixed!


> On Sept. 7, 2015, 11:14 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 199
> > <https://reviews.apache.org/r/37336/diff/6/?file=1044859#file1044859line199>
> >
> >     I believe the issues you are running into regarding `FIXME(marco)` are rooted
in your promise not having a future associated with it.
> >     
> >     Usually the patter we follow is:
> >     ```
> >     // Create a promise.
> >     Promise<Nothing>* promise = new promise();
> >     
> >     // Get the future from the promise.
> >     Future<Nothing> future = promise->future();
> >     
> >     // Attach any mandatory chained functions to the future.
> >     future.then([](){ ...; });
> >     
> >     // Schedule our async action, and fulfill the promise inside that action.
> >     io::read(fd).then([=]() {
> >       promise->set(...);
> >     }).onFailed([=](const std::string& err) {
> >       promise->fail(err);
> >     });
> >     
> >     // Return the future to the user for them to attach any of their own callbacks.
> >     return future.
> >     ```
> >     
> >     Feel free to sync with MPark or me to understand this further offline.

right - that's the pattern I had actually used in my preliminary tests; and it worked.
What throws a spanner in the works are a couple of things:

1) the Future returned from `await()` - not sure what to do with it:
```
  // We need to wait on the process to complete, as well as for
  // stdout and stderr to be available.
  Future<tuple<Future<Option<int>>, Future<string>, Future<string>>>
waitRes =
      await(status(), stdout(), stderr());
```

2) returning a `Future` to the caller, instead of waiting inside the `wait()` method makes
the caller API ugly:
```
Future<CommandResult> future = subprocess.wait();
await(future, Seconds(5));
```
or something along those lines - I wanted to actually "compress" the two calls into one.

The question I had was more along the lines: given the current code (and the fact that it
seems to "work as intended") would you be happy as to where it currently stands? or am I missing
something that will come back and bite us?

Thanks!


- Marco


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


On Aug. 20, 2015, 8:03 a.m., Marco Massenzio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37336/
> -----------------------------------------------------------
> 
> (Updated Aug. 20, 2015, 8:03 a.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> 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 a `wait(Duration timeout)` method
> that returns a `CommandResult` (also introduced with this patch) which
> contains useful information about the command invocation; the exit code;
> stdout and, optionally, stderr too.
> 
> The `wait()` method will wait for both the process to terminate, and
> stdout/stderr to be available to read from; it also "unpacks" them into
> ready-to-use `string`s.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/subprocess.hpp d2341a53aac7c779668ee80983f73158fd44bff5

>   3rdparty/libprocess/src/subprocess.cpp d6ea62ed1c914d34e0e189395831c86fff8aac22 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp ab7515325e5db0a4fd222bb982f51243d7b7e39d

> 
> 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