mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 48428: Updated Docker::run to return exit status of container.
Date Thu, 09 Jun 2016 22:20:42 GMT


> On June 9, 2016, 7:31 p.m., Joseph Wu wrote:
> > src/docker/docker.cpp, line 705
> > <https://reviews.apache.org/r/48428/diff/2/?file=1411845#file1411845line705>
> >
> >     Is there any reason why this `.onDiscard` isn't chained onto the `return s->status()`
below instead of the `s->status()` above?

Chatted with joseph offline, the main reason to call it out separate from the return type
is for readability. If this was a .then that affected the result it would make more sense
to have it in the return statement, but since it's about setting up discard handling, it seems
more readable to have it separate from what we return to the caller.


> On June 9, 2016, 7:31 p.m., Joseph Wu wrote:
> > src/slave/containerizer/docker.cpp, lines 1246-1249
> > <https://reviews.apache.org/r/48428/diff/2/?file=1411847#file1411847line1246>
> >
> >     Since the `run`'s failure is now propagated manually below, is it still necessary
to associate the `inspect` future within a callback?
> >     
> >     i.e. rather than 
> >     ```
> >     promise->associate(inspect);
> >     ```

Chatted with joseph offline, association binds the promise's future to another future, at
which point the promise can no longer be explicitly transitioned. We should document this
in the promise API :)


- Benjamin


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


On June 9, 2016, 3:57 a.m., Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48428/
> -----------------------------------------------------------
> 
> (Updated June 9, 2016, 3:57 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4279
>     https://issues.apache.org/jira/browse/MESOS-4279
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Previously, Docker::run would return a Future<Nothing> which
> requires the caller to perform an inspect to obtain the exit
> status of the container. This is difficult to get right, and
> so this patch leverages the fact that 'docker run' already
> returns the exit status from the container termination to
> simplify the calling code. Namely, in the docker executor we
> need to use the exit status to determine how to send a
> terminal status update (see the command executor code).
> 
> 
> Diffs
> -----
> 
>   src/docker/docker.hpp d9ffc18496718701fac8182506a8c36e21e9c319 
>   src/docker/docker.cpp ffbd7ef56d02dfe550a60851251e6645d2ca5925 
>   src/docker/executor.cpp 1b3a7795b1db83394d4b884c1041c341f88a7df1 
>   src/slave/containerizer/docker.cpp 63efbe6f45958d44d60fe4a7fea816f5fb0457b2 
>   src/tests/containerizer/docker_containerizer_tests.cpp 5591a6784afd10b4c7535f90c2e6745fa74c318b

>   src/tests/containerizer/docker_tests.cpp 7ef52ade0d3389f9e24e3c5c7dda4f8809b9d83f 
>   src/tests/mesos.hpp 6f2888023c957f0af4f7374c98e406816a8423e3 
> 
> Diff: https://reviews.apache.org/r/48428/diff/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>


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