----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21750/#review43578 -----------------------------------------------------------
Higher level comment, why didn't you update removeOffer to make the necessary call on the allocator? src/master/master.cpp <https://reviews.apache.org/r/21750/#comment77835> Ditto about a comment here on why we can CHECK this as it wasn't immediately obvious to me. src/master/master.cpp <https://reviews.apache.org/r/21750/#comment77834> Not yours, but we should probably have a comment here saying that we can CHECK these things because at this point the ValidOfferChecker will have already rejected it. Likewise, we should also have a comment where we instantiate the vector of offer visitors to describe that the order is essential to get right (because of the CHECKs!). Last thing, would be nice if these were just singletons since they have no state (they can't be purely static because we want the virtual aspect of the functions). Allocating them on the heap via 'new' every time a launch task comes in seems fairly wasteful and unnecessarily complicates the code :(. I guess a TODO for this would be nice, same for TaskInfoVisitors :) src/master/master.cpp <https://reviews.apache.org/r/21750/#comment77832> Would love to see a comment as to why we can CHECK this (i.e. because we remove offers for disconnected slaves). src/master/master.cpp <https://reviews.apache.org/r/21750/#comment77839> Some cleanup notes for posterity, the getOffer helper in the master code seems messier in general: Offer* offer = getOffer(offerId); if (offer != NULL) { removeOffer(offer); // Now 'offer' is pointing to deleted memory!!! } vs. if (offers.contains(offerId)) { removeOffer(offerId); } But oh well for now :) src/master/master.cpp <https://reviews.apache.org/r/21750/#comment77841> Is this comment correct? Seems to me that we need this check also because it's possible that the offer id is invalid? src/master/master.cpp <https://reviews.apache.org/r/21750/#comment77838> this isn't indented properly src/master/master.cpp <https://reviews.apache.org/r/21750/#comment77837> Shouldn't "offers" be singular in this sentence? Maybe the "// Remove offers." comment could be amended to reflect the need to recover resources? src/master/master.cpp <https://reviews.apache.org/r/21750/#comment77840> This isn't lined up, do we need the space before the ":"? - Ben Mahler On May 21, 2014, 3 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21750/ > ----------------------------------------------------------- > > (Updated May 21, 2014, 3 a.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, and Niklas Nielsen. > > > Bugs: MESOS-1400 > https://issues.apache.org/jira/browse/MESOS-1400 > > > Repository: mesos-git > > > Description > ------- > > Fixed Master::launchTasks() to inform allocator of unused resources when any > of the offers are invalid. > > > Diffs > ----- > > src/master/master.cpp 075755cad5c50a57c92d7d82f2466b467796f673 > src/tests/master_tests.cpp 1ea1da685420aeee4cbed4765d7cfdf31fc7231f > > Diff: https://reviews.apache.org/r/21750/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
