----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28720/#review65422 -----------------------------------------------------------
Could you drop a TODO in the location where you'll want to call `allocator->transformAllocation(...)`? Just some minor comments, then we can get this shipped! src/master/master.cpp <https://reviews.apache.org/r/28720/#comment108559> Ah, we should consider how to make these unit testable, since ResourceValidator doesn't actually need Framework and Slave. Let's punt for now, but this seems like a pretty dangerous area since we're not going to write big integration tests to catch all the cases. I can think about this. Commented on [MESOS-1064](https://issues.apache.org/jira/browse/MESOS-1064) One idea was to pull out validators as a series of functions, when we compose validators we bind in the appropriate arugments. This means not all validators need to take the same arguments, but we can invoke them through a no-argument function operator, since they have everything bound in already. src/master/master.cpp <https://reviews.apache.org/r/28720/#comment108549> (1) is enforced via validateDiskInfo, right? Let's add that to the comment. The part that I'm a bit confused about is, if the framework changes the role (e.g. "disk(foo):1<ID1>; disk(bar):1<ID1>") where do we catch that? The comment here seems to ignore the fact that the framework could change the role. Is there something I'm missing that should be documented here? src/master/master.cpp <https://reviews.apache.org/r/28720/#comment108551> Mind adding newlines when we squash comments, NOTEs, and TODOs together? ``` // Comment ... // // NOTE: ... // // TODO(jieyu): ... ``` Does this seem a bit easier on the eyes? src/master/master.cpp <https://reviews.apache.org/r/28720/#comment108563> s/resource/disk/ ? src/master/master.cpp <https://reviews.apache.org/r/28720/#comment108579> This comment looks inaccurate, you're only checking the offered resources here? ``` // Ensure the persistence ID for this role does not already exist in the offered resources. ``` Could you add a TODO here for checking the slave's used resources? Once we are checking the slave's used resources, (in https://reviews.apache.org/r/28886/), is it still necessary to look at the offered resources? Won't the task's resources get added in the slave's used resources after the validation passes? src/master/master.cpp <https://reviews.apache.org/r/28720/#comment108582> Would love to have a test for this case! :) src/tests/resource_offers_tests.cpp <https://reviews.apache.org/r/28720/#comment108543> Mind including a summary comment here? ``` // This test ensures that a persistent disk that is larger than // the offered disk resources results in a failed task. ``` Maybe a more precise name? AcquirePersistentDiskTooBig? src/tests/resource_offers_tests.cpp <https://reviews.apache.org/r/28720/#comment108539> Maybe comment on the fact that it's too big? That's why the task fails, right? src/tests/resource_offers_tests.cpp <https://reviews.apache.org/r/28720/#comment108537> Is this comment accurate? Where is the non-persistent disk? - Ben Mahler On Dec. 17, 2014, 11:09 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28720/ > ----------------------------------------------------------- > > (Updated Dec. 17, 2014, 11:09 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 0f55a5cc2d6845cbaace718a48f771d80aad0e6e > src/tests/resource_offers_tests.cpp > e13b6c5460d9e6729843c40bed9e4d4e3f76d5d3 > > Diff: https://reviews.apache.org/r/28720/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
