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: Added `execute()` method to process::Subprocess
Date Fri, 20 Nov 2015 23:08:57 GMT


> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 53-75
> > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line53>
> >
> >     What's the motivation of storing this? Should the caller already have those
information?

not necessarily - please note that this is executed asynchronously, so the caller may have
several of them running concurrently and having the invocation returned together with the
invocation will greatly simplifly the caller's code.

The whole point of this patch is to make the caller's API less "low-level" and simplify the
life of those using this facility.

I have, for example, a module that executes commands upon an HTTP request - having the `invocation`
returned to me with the result, greatly simplifies my code and enables me to return a Response
without waiting for execution.


> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 114-117
> > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line114>
> >
> >     Ditto. I am not convinced that this is strictly necessary.

Of course it's not *necessary* - but it makes the caller's life a lot easier.
This was the whole point of "wrapping" the calls to `subprocess()` with something that avoided
the need to do all the legwork


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

`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).


> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 328
> > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line328>
> >
> >     I don't like the name 'execute'. When you create the Subprocess instance, the
subprocss is already launched and exec'ed. This is rather waiting for the subprocess to terminate.

This method most definitely does **not** wait.

This is how one can use it as a caller (code simplified):
```
  auto s = process::subprocess(commandInfo.command(), args);

  if (s.isError()) {
    LOG(ERROR) << "Could not spawn subprocess: " << s.error();
    return http::ServiceUnavailable(s.error());
  }

  store(s.get().pid());  // <-- needed to reconcile with GETs

  Future<CommandResult> result_ = s->execute();
  result_.then([](const Future<CommandResult> &future) {
        if (future.isFailed()) {
          // mark the command as failed
          return Nothing();
        }
        auto result = future.get();
        // update status of job - use pid(); something equivalent to:
        LOG(INFO) << "Result of '" << result.invocation.command << "'was:
"
                  << result.stdout();
        return Nothing();
      }).after(Seconds(30), [s](const Future<Nothing> &future) {
        // update status of job to timed out; use `invocation` and `stdout`.
        s.get().cleanup();
        return Nothing();
      });

  http::Response response = http::OK("{\"result\": \"OK\", \"pid\": \"" +
                                     stringify(s.get().pid()) + "\"}");
  response.headers["Content-Type"] = "application/json";
  return response;
```


> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 336-343
> > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line336>
> >
> >     Why introduce this method? I think the caller should be responsible for killing
the subprocess if needed.
> >     
> >     Also, os::killtree is in general not reliable and shouldn't be used in library
code.

again, this is all about hiding implementation details from the caller and make their life
easier.
thanks for heads-up about `killtree()` - what would you suggest we use instead?

when you say "unreliable" what do you mean, exactly?
that it may fail to actually kill the process or that it risks blowing up the app or segfaulting
or whatever?


> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, lines 185-190
> > <https://reviews.apache.org/r/37336/diff/11/?file=1122403#file1122403line185>
> >
> >     What if outData is called more than once?

Great question!
I would guess they'll get an empty string (as I assume that `io::read()` is sitting on an
empty buffer) but worth looking into (and testing!)

Thanks.


- Marco


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