----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22151/#review44574 -----------------------------------------------------------
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. src/master/master.hpp <https://reviews.apache.org/r/22151/#comment78998> Why are they pending? Maybe add a bit to this comment explaining? src/master/master.cpp <https://reviews.apache.org/r/22151/#comment79080> 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). src/master/master.cpp <https://reviews.apache.org/r/22151/#comment78999> Might be nice to remove the indentation: if (authorizer.isNone()) { return true; } ... src/master/master.cpp <https://reviews.apache.org/r/22151/#comment79075> How about calling this 'futures' plural? src/master/master.cpp <https://reviews.apache.org/r/22151/#comment79078> 'await' will never fail the Future (we should just CHECK that), so the only case that's important here is discarded, but since we don't discard(), that's also not possible. How about a CHECK_READY(future) rather than all the logic to deal with a case that cannot occur? src/master/master.cpp <https://reviews.apache.org/r/22151/#comment79073> Was protobuf::createStatusUpdate insufficient here? src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/22151/#comment79061> In the interest of minimizing the diff, do you need this and the other logging level changes below? Maybe a dependent review? src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/22151/#comment79063> 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? - Ben Mahler 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 > >
