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