> On June 5, 2014, 10:45 p.m., Ben Mahler wrote: > > src/master/master.hpp, lines 317-327 > > <https://reviews.apache.org/r/22151/diff/3/?file=603279#file603279line317> > > > > 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/
done > On June 5, 2014, 10:45 p.m., Ben Mahler wrote: > > src/master/master.hpp, lines 329-331 > > <https://reviews.apache.org/r/22151/diff/3/?file=603279#file603279line329> > > > > Should we document the failure case for launchTask? done > On June 5, 2014, 10:45 p.m., Ben Mahler wrote: > > src/master/master.cpp, line 2145 > > <https://reviews.apache.org/r/22151/diff/3/?file=603280#file603280line2145> > > > > Looks like we would want this check even if the framework is null? pulled it up. > On June 5, 2014, 10:45 p.m., Ben Mahler wrote: > > src/master/master.cpp, line 2151 > > <https://reviews.apache.org/r/22151/diff/3/?file=603280#file603280line2151> > > > > What about post-incrementing here? > > > > const TaskInfo& task = tasks[index++]; > > > > Or: > > > > const TaskInfo& task = tasks[index]; > > index++; can't do because we need to increment index even if the 'future' failed. > On June 5, 2014, 10:45 p.m., Ben Mahler wrote: > > src/master/master.cpp, lines 2153-2154 > > <https://reviews.apache.org/r/22151/diff/3/?file=603280#file603280line2153> > > > > 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. done > On June 5, 2014, 10:45 p.m., Ben Mahler wrote: > > src/master/master.cpp, line 2146 > > <https://reviews.apache.org/r/22151/diff/3/?file=603280#file603280line2146> > > > > size_t? done > On June 5, 2014, 10:45 p.m., Ben Mahler wrote: > > src/master/master.cpp, line 1476 > > <https://reviews.apache.org/r/22151/diff/3/?file=603280#file603280line1476> > > > > 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. Unfortunately we cannot get away from not depending on non-local knowledge. Example: If launchTasks1(), launchTasks2(), _launchTasks1() happen in this order, local 'ids' is of no use to detect a duplicate id. I left executor ids asis in ResourceUsageChecker as an optimization (to use a hashmap instead of having to loop through pendingTasks to find the executor id). I will remove the optmization. I want to get to a place where the validators don't hold local state (the last remnant is 'usedResources' in ResourceUsageChecker). > On June 5, 2014, 10:45 p.m., Ben Mahler wrote: > > src/master/master.hpp, line 341 > > <https://reviews.apache.org/r/22151/diff/3/?file=603279#file603279line341> > > > > Thoughts on using shared_ptr for the TaskInfoVisitors? Might help avoid > > a memory leak and is a quick way to simplify things a bit. done > On June 5, 2014, 10:45 p.m., Ben Mahler wrote: > > src/common/protobuf_utils.hpp, line 51 > > <https://reviews.apache.org/r/22151/diff/3/?file=603277#file603277line51> > > > > // ... for updates created by the master? don't want a util function to know about its callers. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22151/#review44833 ----------------------------------------------------------- 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 > >
