mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ian Downes" <ian.dow...@gmail.com>
Subject Re: Review Request 18403: Added support for launching tasks by TaskInfo.
Date Tue, 18 Mar 2014 16:23:57 GMT

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

Ship it!


Some minor points for consideration.


src/slave/slave.hpp
<https://reviews.apache.org/r/18403/#comment69069>

    Do we really need both of these constructors for Executor?



src/slave/slave.hpp
<https://reviews.apache.org/r/18403/#comment69070>

    Executor::finalize doesn't seem to be a very appropriate name for part of the transition
from LAUNCHING to REGISTERING?
    
    Slave::finalize is for clean up when the slave is exiting.
    
    What about launchCompleted() or something?



src/slave/slave.hpp
<https://reviews.apache.org/r/18403/#comment69065>

    Could this not be Future<ExecutorInfo> and then remove the Future<bool> future
below? This better reflects this variable - it's not available until launch is complete, rather
than being optional.



src/slave/slave.cpp
<https://reviews.apache.org/r/18403/#comment69066>

    Is it worth pulling this removal out into a preceding review?



src/slave/slave.cpp
<https://reviews.apache.org/r/18403/#comment69067>

    Move comments 2) and 3) to below the if (executor == NULL)?



src/slave/slave.cpp
<https://reviews.apache.org/r/18403/#comment69068>

    see previous comment about using Future<ExecutorInfo>


- Ian Downes


On March 17, 2014, 5:37 p.m., Niklas Nielsen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18403/
> -----------------------------------------------------------
> 
> (Updated March 17, 2014, 5:37 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Ian Downes, Till Toenshoff,
and Vinod Kone.
> 
> 
> Bugs: MESOS-922
>     https://issues.apache.org/jira/browse/MESOS-922
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> This patch delegates the choice of executor to the containerizer by removing executorInfo
dependencies up until Containerizer::launch().
> Containerizer::launch() now returns a future to the executor info that is being run and
the slave creates the corresponding executor structure when launch completes.
> This means message handling from the running executor to the slave in the interim where
the executor structure has not created, need to be enqueued until executor is ready. So far,
registerExecutor() and reregisterExecutor() has been split into two continuations to deal
with this issue.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/containerizer.hpp d9ae326 
>   src/slave/containerizer/mesos_containerizer.hpp ee1fd30 
>   src/slave/containerizer/mesos_containerizer.cpp c819c97 
>   src/slave/http.cpp 594032d 
>   src/slave/slave.hpp 01b80df 
>   src/slave/slave.cpp d8d3e0f 
>   src/tests/containerizer.hpp 5686398 
>   src/tests/containerizer.cpp bfb9341 
> 
> Diff: https://reviews.apache.org/r/18403/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Niklas Nielsen
> 
>


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