----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28720/#review65537 -----------------------------------------------------------
Ship it! src/master/master.cpp <https://reviews.apache.org/r/28720/#comment108754> Per our chat: In ResourceValidator here, you assume that all used resources are either * or a single role because of 1) and 2). But this doesn't hold because we also need 3) from ResourceUsageValidator. Where 3) is that we disallow the framework from changing the role of resources. As you suggested, could we just store a hashmap<role, hashset<ids>> and remove this giant note? The reasoning will be straightforward :) src/master/master.cpp <https://reviews.apache.org/r/28720/#comment108755> Ok, this tripped me up a bit, given there are a few invariants here we might want to write down explicitly: ``` // We allow frameworks to implicitly acquire persistent disks // through task and executor resources. This means that we need // to infer these implicit disk acquisition transformations on the // offered resources, so that we can validate resource usage. // // NOTE: ResourceValidator ensures that there are no duplicate // persistence IDs per role in 'resources'. // // NOTE: 'offeredResources' will not contain duplicate persistence // IDs per role, given we do not construct such offers. Resources::CompositeTransformation transformation; foreach (const Resource& disk, resources.persistentDisks()) { if (!offeredResources.contains(disk)) { // This is an implicit acquisition, the framework is not allowed // to mutate an offered persistent disk, so we need to check the // offered resources for this persistence ID within the role. // // TODO(jieyu): We need to ensure this persistence ID within the // role does not clash with any in-use persistent disks on the // slave. ... } } ``` src/master/master.cpp <https://reviews.apache.org/r/28720/#comment108756> How about we move the comment below up here: ``` // Validate that the offered resources are sufficient for launching this task/executor. // First we must apply the implicit transformations. ``` src/master/master.cpp <https://reviews.apache.org/r/28720/#comment108758> Mind wrapping it at the parenthesis? Seems a bit less "jagged" given it's a short line, but up to you. src/master/master.cpp <https://reviews.apache.org/r/28720/#comment108757> Could you remove this comment once you move it up, per my other comment? src/master/master.cpp <https://reviews.apache.org/r/28720/#comment108759> Can you remove the NOTE on this one? Doesn't seem like a "gotcha" IMHO. - Ben Mahler On Dec. 18, 2014, 7:59 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28720/ > ----------------------------------------------------------- > > (Updated Dec. 18, 2014, 7:59 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-2030 > https://issues.apache.org/jira/browse/MESOS-2030 > > > Repository: mesos-git > > > Description > ------- > > Introduced an abstraction for mutating resources. Acquiring persistent disk > is one type of resources mutation. > > Infer persistent disk acquisitions from resources and check resource usage > against adjusted total resources. > > Adjusted the calculation of unused resources in _launchTasks by considering > persistent disk acquisition. > > > Diffs > ----- > > src/master/master.cpp 0c98b5184dec4c6db87f7a6ae7993b83f9b9b4c9 > src/tests/resource_offers_tests.cpp > e13b6c5460d9e6729843c40bed9e4d4e3f76d5d3 > > Diff: https://reviews.apache.org/r/28720/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
