mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Charles Reiss" <woggl...@gmail.com>
Subject Re: Review Request: Account for queued task resources when making post-registration executor resource adjustment
Date Mon, 14 Nov 2011 21:34:23 GMT


> On 2011-11-14 19:21:12, Benjamin Hindman wrote:
> > src/slave/slave.cpp, line 750
> > <https://reviews.apache.org/r/2669/diff/1/?file=55784#file55784line750>
> >
> >     s/start/start.

Done.


> On 2011-11-14 19:21:12, Benjamin Hindman wrote:
> > src/slave/slave.cpp, line 755
> > <https://reviews.apache.org/r/2669/diff/1/?file=55784#file55784line755>
> >
> >     Let's keep this down by the actual RunTaskMessage for consistency with the other
RunTaskMessage usage.

Done.


> On 2011-11-14 19:21:12, Benjamin Hindman wrote:
> > src/slave/slave.cpp, lines 758-759
> > <https://reviews.apache.org/r/2669/diff/1/?file=55784#file55784line758>
> >
> >     This is great. But three things: (1) let's tweak this comment to include the
fact that we are changing the resources _after_ we have accounted for the tasks; (2) possibly
include a TODO or NOTE that says that given the asynchronous nature of the dispatch this still
might not be sufficient for guaranteeing that the resource limits have been changed before
the executor starts running the task; and (3) also change the other place in the code where
we send a RunTaskMessage to a slave to dispatch to the isolation module before we send the
message.

Done.


> On 2011-11-14 19:21:12, Benjamin Hindman wrote:
> > src/tests/master_tests.cpp, lines 137-138
> > <https://reviews.apache.org/r/2669/diff/1/?file=55785#file55785line137>
> >
> >     Unless I'm mistaken, because of the async nature of the isolation module there
is no happens-before relationship in the code that guarantees that the isolation module will
have gotten a "resourcesChanged" before the slave gets a status update from the executor that
it sends back to the scheduler. I'd rather you add a TODO in the code to try and capture the
fact that resources have been updated then doing it this way for now.

I agree. Changed the test to wait for an resourcesChanged() call, so we have a test that resourcesChanged()
is eventually called with the correct value.

Added TODOs to both the dispatch(...resourcesChanged) in the slave. I'll also file a separate
JIRA about this; it probably needs a IsolationModule API change.


- Charles


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


On 2011-11-14 21:34:18, Charles Reiss wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2669/
> -----------------------------------------------------------
> 
> (Updated 2011-11-14 21:34:18)
> 
> 
> Review request for mesos.
> 
> 
> Summary
> -------
> 
> This calls Executor::addTask to accumulate the usage of queued tasks before calling resourcesChanged()
to ensure the executor has enough resources to run those queued tasks.
> 
> 
> This addresses bug MESOS-56.
>     https://issues.apache.org/jira/browse/MESOS-56
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.cpp de5716c 
>   src/tests/master_tests.cpp 2438114 
>   src/tests/utils.hpp 02772f7 
> 
> Diff: https://reviews.apache.org/r/2669/diff
> 
> 
> Testing
> -------
> 
> Modification to MasterTest included.
> 
> 
> Thanks,
> 
> Charles
> 
>


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