> On Dec. 18, 2014, 2:31 a.m., Ben Mahler wrote: > > src/master/master.cpp, lines 1897-1902 > > <https://reviews.apache.org/r/28720/diff/7/?file=795193#file795193line1897> > > > > 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. > > Michael Park wrote: > +1, also added to > [MESOS-1064](https://issues.apache.org/jira/browse/MESOS-1064)
Added a comment to MESOS-1064 as well. > On Dec. 18, 2014, 2:31 a.m., Ben Mahler wrote: > > src/master/master.cpp, lines 1905-1909 > > <https://reviews.apache.org/r/28720/diff/7/?file=795193#file795193line1905> > > > > (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? Adjusted the comments. > On Dec. 18, 2014, 2:31 a.m., Ben Mahler wrote: > > src/master/master.cpp, line 1910 > > <https://reviews.apache.org/r/28720/diff/7/?file=795193#file795193line1910> > > > > Mind adding newlines when we squash comments, NOTEs, and TODOs together? > > > > ``` > > // Comment ... > > // > > // NOTE: ... > > // > > // TODO(jieyu): ... > > ``` > > > > Does this seem a bit easier on the eyes? Done. > On Dec. 18, 2014, 2:31 a.m., Ben Mahler wrote: > > src/master/master.cpp, line 2076 > > <https://reviews.apache.org/r/28720/diff/7/?file=795193#file795193line2076> > > > > s/resource/disk/ ? Done. > On Dec. 18, 2014, 2:31 a.m., Ben Mahler wrote: > > src/master/master.cpp, line 2078 > > <https://reviews.apache.org/r/28720/diff/7/?file=795193#file795193line2078> > > > > 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? It's still needed because if not, we may end up having two disks with the same id (different size). At the time the validation happens, the this task's resource hasn't been added to the slave's used resources yet. Added a TODO. > On Dec. 18, 2014, 2:31 a.m., Ben Mahler wrote: > > src/tests/resource_offers_tests.cpp, line 995 > > <https://reviews.apache.org/r/28720/diff/7/?file=795194#file795194line995> > > > > 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? Done. > On Dec. 18, 2014, 2:31 a.m., Ben Mahler wrote: > > src/tests/resource_offers_tests.cpp, line 1039 > > <https://reviews.apache.org/r/28720/diff/7/?file=795194#file795194line1039> > > > > Maybe comment on the fact that it's too big? That's why the task fails, > > right? Done. > On Dec. 18, 2014, 2:31 a.m., Ben Mahler wrote: > > src/tests/resource_offers_tests.cpp, lines 1043-1045 > > <https://reviews.apache.org/r/28720/diff/7/?file=795194#file795194line1043> > > > > Is this comment accurate? Where is the non-persistent disk? Fixed here and everywhere else. > On Dec. 18, 2014, 2:31 a.m., Ben Mahler wrote: > > src/master/master.cpp, line 2083 > > <https://reviews.apache.org/r/28720/diff/7/?file=795193#file795193line2083> > > > > Would love to have a test for this case! :) Sure. - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28720/#review65422 ----------------------------------------------------------- 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 > >
