----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28720/#review65237 -----------------------------------------------------------
First pass, only looked at the disk transformation for now, which would be great to pull out into a separate patch with tests! :) I'm still thinking about the idempotent nature of transformations more generally, but we can punt on it for now until we have more of them to deal with. :) src/master/master.cpp <https://reviews.apache.org/r/28720/#comment108273> Why is a disk with no persistence info and no volume considered valid? What does that mean? src/master/master.cpp <https://reviews.apache.org/r/28720/#comment108274> There is a bit of non-local reasoning needed here, since this assumes that has_volume() implies has_persistence(), otherwise we'd be missing transformations. That is, I need to know about the semantics of the validation code here. It would be nice if has_disk() was the only signal for transformation, because that allows the validation to change (allow volumes w/o persistence ids) while keeping this code correct: ``` if (resource.has_disk()) { transformation.add(DiskTransformation(resource)); } ``` But since we have the very specific `AcquirePersistentDisk` currently, maybe we just need to show our assumptions here with a NOTE and a CHECK: ``` if (resource.has_disk()) { // NOTE: Validation currently ensures that disk metadata // is only provided when persistence is desired. CHECK(resource.disk().has_persistence()); transformation.add(AcquirePersistentDisk(resource)); } ``` Thoughts? src/master/master.cpp <https://reviews.apache.org/r/28720/#comment108277> How about we rename totalResources to offeredResources, then we can call this transformedOfferedResources? All these "adjusted" namings make me think that we should have called it Adjustment instead of Transformation ;) include/mesos/resources.hpp <https://reviews.apache.org/r/28720/#comment108288> Any chance you could split this one out into a separate patch with some tests? We can land it quicker that way :) src/common/resources.cpp <https://reviews.apache.org/r/28720/#comment108291> How about splitting these checks? ``` CHECK(disk.has_disk()); CHECK(disk.disk().has_persistence()); ``` src/common/resources.cpp <https://reviews.apache.org/r/28720/#comment108292> Duplicate persistence IDs are allowed across roles? Is that handled everywhere else in the code? src/common/resources.cpp <https://reviews.apache.org/r/28720/#comment108293> Can we include the ID? src/common/resources.cpp <https://reviews.apache.org/r/28720/#comment108299> How about `stripped` or `strippedDisk`? Might be a bit easier to read. src/common/resources.cpp <https://reviews.apache.org/r/28720/#comment108300> How about: ``` return Error("Insufficient disk resources"); ``` - Ben Mahler On Dec. 16, 2014, 9:11 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28720/ > ----------------------------------------------------------- > > (Updated Dec. 16, 2014, 9:11 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 > ----- > > include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc > src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92 > 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 > >
