> On June 3, 2014, 5:52 p.m., Ben Mahler wrote: > > First pass, didn't look at all of the code just yet. Higher level comments: > > > > (1) 'validateTask' is a bit confusing in terms of return value because it's > > actually doing two things (validation and authorization). If it doesn't > > pass validation, we return a failure. If it doesn't pass authorization, we > > return false. Can you add documentation related to this? > > > > (2) The task visitor ownership is really tricky now! Any plan there? At > > least, is it possible to use 'Shared' visitors to avoid needing to pass > > them in to _launchTasks? > > > > I will help with these two: > > > > (3) MESOS-1451: To clean things up a bit, we should remove the singular > > 'offer_id' field from LaunchTasksMessage and the corresponding logic inside > > launchTasks to deal with it, since the scheduler driver has been setting > > 'offer_ids' since 0.18.0 and we're now in 0.20.0. > > > > (4) MESOS-1452: I think we should reduce the room for mistakes around > > removeOffer (per previous conversations) so that it takes an enum that > > decides whether to USE, DISCARD, RESCIND.
(1) added a comment. (2) let me think about this. > On June 3, 2014, 5:52 p.m., Ben Mahler wrote: > > src/master/master.cpp, line 2115 > > <https://reviews.apache.org/r/22151/diff/1/?file=601791#file601791line2115> > > > > How about calling this 'futures' plural? thats how it was originally named but changed it to singular because its a future of a list not a list of futures. reverted. > On June 3, 2014, 5:52 p.m., Ben Mahler wrote: > > src/master/master.cpp, lines 1992-2015 > > <https://reviews.apache.org/r/22151/diff/1/?file=601791#file601791line1992> > > > > Might be nice to remove the indentation: > > > > if (authorizer.isNone()) { > > return true; > > } > > > > ... done. > On June 3, 2014, 5:52 p.m., Ben Mahler wrote: > > src/master/master.hpp, line 917 > > <https://reviews.apache.org/r/22151/diff/1/?file=601790#file601790line917> > > > > Why are they pending? Maybe add a bit to this comment explaining? added comment. > On June 3, 2014, 5:52 p.m., Ben Mahler wrote: > > src/master/master.cpp, line 1535 > > <https://reviews.apache.org/r/22151/diff/1/?file=601791#file601791line1535> > > > > Seems like we should avoid using the phrase "if we are here" as it > > seems redundant to me (there are two other cases in the diff). done > On June 3, 2014, 5:52 p.m., Ben Mahler wrote: > > src/master/master.cpp, lines 2275-2284 > > <https://reviews.apache.org/r/22151/diff/1/?file=601791#file601791line2275> > > > > Was protobuf::createStatusUpdate insufficient here? yea. because createStatusUpdate() expects a slave id. added a todo for createStatusUpdate() to take an optional slave id. > On June 3, 2014, 5:52 p.m., Ben Mahler wrote: > > src/master/hierarchical_allocator_process.hpp, line 546 > > <https://reviews.apache.org/r/22151/diff/2/?file=602448#file602448line546> > > > > In the interest of minimizing the diff, do you need this and the other > > logging level changes below? Maybe a dependent review? ok. > On June 3, 2014, 5:52 p.m., Ben Mahler wrote: > > src/master/hierarchical_allocator_process.hpp, lines 550-555 > > <https://reviews.apache.org/r/22151/diff/2/?file=602448#file602448line550> > > > > I don't understand the subtlety here, it looks like > > Master::_launchTasks doesn't call resourcesUnused if the framework was > > removed, something I'm missing that the comment should say? you are right. i was just being overzealous here. reverted the check for framework. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22151/#review44574 ----------------------------------------------------------- On June 3, 2014, 5:25 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22151/ > ----------------------------------------------------------- > > (Updated June 3, 2014, 5:25 a.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Bugs: MESOS-1114 > https://issues.apache.org/jira/browse/MESOS-1114 > > > Repository: mesos-git > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/Makefile.am ffde59be8683dd40cc5bc7cb88cd88c5bc91cf96 > src/master/hierarchical_allocator_process.hpp > 0c5e2e050cad96fafaf136232bd255b0ae3038cd > src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1 > src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719 > src/tests/master_authorization_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/22151/diff/ > > > Testing > ------- > > make check. > > Will write new tests and update this review or will create a new one. > > > Thanks, > > Vinod Kone > >
