----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22151/#review44833 -----------------------------------------------------------
Now I just need to look at the tests next! :) src/common/protobuf_utils.hpp <https://reviews.apache.org/r/22151/#comment79391> // ... for updates created by the master? src/master/master.hpp <https://reviews.apache.org/r/22151/#comment79396> Here's a proposal to try to avoid mixing the meaning the failure conditions (invalid task information vs. authorization denied): // Returns the validation result for the task. // Returns a failure if validation fails. Future<Option<Invalid> > validateTask(...); This exposes the validation result as the type of the future. Since we probably don't want to bother creating an 'Invalid' type for now, we can re-use the 'Error' type as we did in a few other places: // Returns the validation result for the task. // Returns a failure if validation fails. Future<Option<Error> > validateTask(...); This looks familiar to the discussion in: https://reviews.apache.org/r/22162/ src/master/master.hpp <https://reviews.apache.org/r/22151/#comment79446> Should we document the failure case for launchTask? src/master/master.hpp <https://reviews.apache.org/r/22151/#comment79447> Thoughts on using shared_ptr for the TaskInfoVisitors? Might help avoid a memory leak and is a quick way to simplify things a bit. src/master/master.cpp <https://reviews.apache.org/r/22151/#comment79449> The fact that this code has non-local knowledge (that the task is placed inside pendingTasks before any further validation takes place) is really tricky and seems error prone, it seems safer to just keep the 'ids' here for now? FWIW it seems the executor ids were left in the ResourceUsageChecker as well. src/master/master.cpp <https://reviews.apache.org/r/22151/#comment79453> If we don't end up using shared_ptr, can't this cleanup just be done once at the start of _launchTasks? src/master/master.cpp <https://reviews.apache.org/r/22151/#comment79456> Looks like we would want this check even if the framework is null? src/master/master.cpp <https://reviews.apache.org/r/22151/#comment79457> size_t? src/master/master.cpp <https://reviews.apache.org/r/22151/#comment79459> What about post-incrementing here? const TaskInfo& task = tasks[index++]; Or: const TaskInfo& task = tasks[index]; index++; src/master/master.cpp <https://reviews.apache.org/r/22151/#comment79458> Could we add a TODO related to how we can achieve a synchronous launchTask->_launchTask instead of an asynchronous launchTask... ->_launchTasks? Might simplify things a bit and we wouldn't need to worry about this check here. - Ben Mahler On June 3, 2014, 11 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22151/ > ----------------------------------------------------------- > > (Updated June 3, 2014, 11 p.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/common/protobuf_utils.hpp 0f653414bc1bb2b632ec8cd9c8bd7202a53d42e1 > 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 > >
