> On May 21, 2014, 3:33 a.m., Ben Mahler wrote: > > Higher level comment, why didn't you update removeOffer to make the > > necessary call on the allocator? > > Ben Mahler wrote: > To be more clear, why not have a 'useOffer' and a 'discardOffer' for the > two cases we care about?
added a TODO for now. > On May 21, 2014, 3:33 a.m., Ben Mahler wrote: > > src/master/master.cpp, line 1633 > > <https://reviews.apache.org/r/21750/diff/1/?file=585964#file585964line1633> > > > > Ditto about a comment here on why we can CHECK this as it wasn't > > immediately obvious to me. see below. > On May 21, 2014, 3:33 a.m., Ben Mahler wrote: > > src/master/master.cpp, lines 1654-1658 > > <https://reviews.apache.org/r/21750/diff/1/?file=585964#file585964line1654> > > > > 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 :) i removed the CHECK because it depends on the order of validators which i don't like. > On May 21, 2014, 3:33 a.m., Ben Mahler wrote: > > src/master/master.cpp, lines 1661-1663 > > <https://reviews.apache.org/r/21750/diff/1/?file=585964#file585964line1661> > > > > Would love to see a comment as to why we can CHECK this (i.e. because > > we remove offers for disconnected slaves). this is actually a bug. https://issues.apache.org/jira/browse/MESOS-1418 i will fix this in another (dependent) patch. > On May 21, 2014, 3:33 a.m., Ben Mahler wrote: > > src/master/master.cpp, lines 1812-1818 > > <https://reviews.apache.org/r/21750/diff/1/?file=585964#file585964line1812> > > > > this isn't indented properly the outer "if" wasn't indented properly afaict. fixed. > On May 21, 2014, 3:33 a.m., Ben Mahler wrote: > > src/master/master.cpp, line 1814 > > <https://reviews.apache.org/r/21750/diff/1/?file=585964#file585964line1814> > > > > Shouldn't "offers" be singular in this sentence? Maybe the "// Remove > > offers." comment could be amended to reflect the need to recover resources? fixed. > On May 21, 2014, 3:33 a.m., Ben Mahler wrote: > > src/master/master.cpp, line 1824 > > <https://reviews.apache.org/r/21750/diff/1/?file=585964#file585964line1824> > > > > This isn't lined up, do we need the space before the ":"? done > On May 21, 2014, 3:33 a.m., Ben Mahler wrote: > > src/master/master.cpp, lines 1810-1811 > > <https://reviews.apache.org/r/21750/diff/1/?file=585964#file585964line1810> > > > > Is this comment correct? Seems to me that we need this check also > > because it's possible that the offer id is invalid? hmm. doesn't look like it. killed it. > On May 21, 2014, 3:33 a.m., Ben Mahler wrote: > > src/master/master.cpp, lines 1809-1818 > > <https://reviews.apache.org/r/21750/diff/1/?file=585964#file585964line1809> > > > > 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 :) there is an existing TODO to kill getOffer, so I'll punt on it for now. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21750/#review43578 ----------------------------------------------------------- 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 > >
