mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Niklas Nielsen" <...@qni.dk>
Subject Re: Review Request 18403: Added support for launching tasks by TaskInfo.
Date Thu, 27 Feb 2014 18:08:21 GMT


> On Feb. 26, 2014, 10:29 a.m., Ian Downes wrote:
> > I think we need to work on this a little. It seems that making the launching state
explicit and then cleaning up behavior (killTask) when in this state is necessary.

Thanks for the prompt review! :)


> On Feb. 26, 2014, 10:29 a.m., Ian Downes wrote:
> > src/slave/containerizer/mesos_containerizer.cpp, line 670
> > <https://reviews.apache.org/r/18403/diff/2/?file=503852#file503852line670>
> >
> >     const ExecutorInfo& ?

Good point! Thanks :)


> On Feb. 26, 2014, 10:29 a.m., Ian Downes wrote:
> > src/slave/slave.cpp, line 1021
> > <https://reviews.apache.org/r/18403/diff/2/?file=503854#file503854line1021>
> >
> >     What about introducing a new executor state - Executor::LAUNCHING?

I like that idea - I will take that for a spin.


> On Feb. 26, 2014, 10:29 a.m., Ian Downes wrote:
> > src/slave/slave.cpp, line 1432
> > <https://reviews.apache.org/r/18403/diff/2/?file=503854#file503854line1432>
> >
> >     s/We might be in a situation where the executor is launching but not yet ready.
The second half is scheduled until executor is ready, or called directly if executor is running./If
the executor is still launching we'll defer registering until it is ready./?

Much better - thanks!


> On Feb. 26, 2014, 10:29 a.m., Ian Downes wrote:
> > src/slave/slave.cpp, line 3505
> > <https://reviews.apache.org/r/18403/diff/2/?file=503854#file503854line3505>
> >
> >     The determination of executor resources should be done by each containerizer
implementation if they choose an executor.

Cool - scratched the comment.


> On Feb. 26, 2014, 10:29 a.m., Ian Downes wrote:
> > src/slave/containerizer/containerizer.hpp, line 125
> > <https://reviews.apache.org/r/18403/diff/2/?file=503849#file503849line125>
> >
> >     Should this be in mesos_containerizer? I was thinking if the task didn't have
task.executor() then the determination of the executor would be containerizer implementation
dependent.

I wanted to make it available for other containerizers in case they wanted a default behavior.
Moved it into mesos_containerizer.


> On Feb. 26, 2014, 10:29 a.m., Ian Downes wrote:
> > src/slave/containerizer/mesos_containerizer.cpp, line 402
> > <https://reviews.apache.org/r/18403/diff/2/?file=503852#file503852line402>
> >
> >     Why is this a Future<>? Can you not just pass the ExecutorInfo to exec()
and have it return?

Ah - that is a mistake. Thanks.


> On Feb. 26, 2014, 10:29 a.m., Ian Downes wrote:
> > src/slave/slave.cpp, line 824
> > <https://reviews.apache.org/r/18403/diff/2/?file=503854#file503854line824>
> >
> >     Could you swap this logic? i.e.,
> >     if (contains) {
> >       launch = framework->launchingExecutors[];
> >     } else {
> >       launch = framework->launch();
> >     }

Sure :)


> On Feb. 26, 2014, 10:29 a.m., Ian Downes wrote:
> > src/slave/slave.cpp, line 853
> > <https://reviews.apache.org/r/18403/diff/2/?file=503854#file503854line853>
> >
> >     What happens to Executor* if the future is ready?

Not sure I follow; we can't do much with if the framework can't be found?


> On Feb. 26, 2014, 10:29 a.m., Ian Downes wrote:
> > src/slave/slave.cpp, line 1006
> > <https://reviews.apache.org/r/18403/diff/2/?file=503854#file503854line1006>
> >
> >     Previously executor == NULL was unexpected but now it seems weird to have NULL
as a 'normal' value. Can we capture this is a better way?

Should be fixed with the new _killTask()


> On Feb. 26, 2014, 10:29 a.m., Ian Downes wrote:
> > src/slave/slave.cpp, line 1759
> > <https://reviews.apache.org/r/18403/diff/2/?file=503854#file503854line1759>
> >
> >     okay :-p

With the new executor state, we should be able to clean up. So I scratched the comment.


- Niklas


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


On Feb. 27, 2014, 10:07 a.m., Niklas Nielsen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18403/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2014, 10:07 a.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 6d990cb 
>   src/slave/http.cpp 594032d 
>   src/slave/slave.hpp 01b80df 
>   src/slave/slave.cpp 4f5349b 
>   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