----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45961/#review147218 -----------------------------------------------------------
Updated the review with the following changes. I have already committed all 'resources*.cpp's so they are removed from the review. I moved the tests to /r/45962/ so the remainder of the current set of tests are in one place. Please take a look. src/master/allocator/mesos/hierarchical.cpp (lines 620 - 621) <https://reviews.apache.org/r/45961/#comment214733> src/master/allocator/mesos/hierarchical.cpp (lines 622 - 630) <https://reviews.apache.org/r/45961/#comment214767> I am a bit uncomfortable with processing LAUNCHes after all other operations are applied. This may or may not be problematic but it's not the same as the original order. Implementation cleanliness aside, I think it's most intuitive if we structure it as "loop through the list of operations and handle them individually, we just added some logic for handling the LAUNCH operation". In fact in my change I kept most of the method unchanged. src/master/allocator/mesos/hierarchical.cpp (lines 651 - 653) <https://reviews.apache.org/r/45961/#comment214390> src/master/allocator/mesos/hierarchical.cpp (line 693) <https://reviews.apache.org/r/45961/#comment214578> `additional` could have more instances than `updatedFrameworkAllocation`. src/master/allocator/sorter/drf/sorter.cpp (lines 163 - 178) <https://reviews.apache.org/r/45961/#comment214763> I changed it to the following which I think is cleaner and more direct translation of the comment. ``` // Add shared resources to the total quantities when the same // resources don't already exist in the allocation. const Resources newShared = resources.shared().filter([this, name, slaveId]( const Resource& resource) { return !allocations[name].resources[slaveId].contains(resource); }); allocations[name].resources[slaveId] += resources; allocations[name].scalarQuantities += (resources.nonShared() + newShared).createStrippedScalarQuantity(); ``` src/master/allocator/sorter/drf/sorter.cpp (lines 285 - 296) <https://reviews.apache.org/r/45961/#comment214764> Similarly: ``` // Remove shared resources from the allocated quantities when there // are no instances of same resources left in the allocation. const Resources absentShared = resources.shared().filter( [this, name, slaveId](const Resource& resource) { return !allocations[name].resources[slaveId].contains(resource); }); const Resources resourcesQuantity = (resources.nonShared() + absentShared).createStrippedScalarQuantity(); ``` src/master/allocator/sorter/drf/sorter.cpp (lines 314 - 328) <https://reviews.apache.org/r/45961/#comment214765> Similarly, ``` // Add shared resources to the total quantities when the same // resources don't already exist in the total. const Resources newShared = resources.shared().filter([this, slaveId]( const Resource& resource) { return !total_.resources[slaveId].contains(resource); }); total_.resources[slaveId] += resources; total_.scalarQuantities += (resources.nonShared() + newShared).createStrippedScalarQuantity(); ``` src/master/allocator/sorter/drf/sorter.cpp (lines 347 - 359) <https://reviews.apache.org/r/45961/#comment214766> Similarly, ``` // Remove shared resources from the total quantities when there // are no instances of same resources left in the total. const Resources absentShared = resources.shared().filter( [this, slaveId](const Resource& resource) { return !total_.resources[slaveId].contains(resource); }); const Resources resourcesQuantity = (resources.nonShared() + absentShared).createStrippedScalarQuantity(); ``` src/master/master.cpp (lines 3807 - 3808) <https://reviews.apache.org/r/45961/#comment214338> Why the change from `offeredSharedResources = _offeredResources.shared();`? This won't work if `offeredSharedResources` has multiple copies. I also changed the CREATE case to be more consistent. src/master/master.cpp (lines 3820 - 3821) <https://reviews.apache.org/r/45961/#comment214339> This is far away from its first usage. Plus the comments about why we are doing it is below. I moved it down. src/master/master.cpp (line 3821) <https://reviews.apache.org/r/45961/#comment214329> Indentation. src/master/master.cpp (line 6636) <https://reviews.apache.org/r/45961/#comment214735> Kill this redundant line. src/master/validation.cpp (line 904) <https://reviews.apache.org/r/45961/#comment214769> I moved the comment to the allocator where the context can help people understand this TODO. ``` // TODO(anindya_sinha): For simplicity we currently don't // allow shared resources in ExecutorInfo. See comments in // `HierarchicalAllocatorProcess::updateAllocation()` for more // details. Remove this check once we start supporting it. ``` src/master/validation.cpp (lines 1558 - 1574) <https://reviews.apache.org/r/45961/#comment214737> This worked. `,` in `foreachvalue` was the culprit. ``` typedef hashmap<TaskID, TaskInfo> TaskMap; foreachvalue(const TaskMap& tasks, pendingTasks) { foreachvalue (const TaskInfo& task, tasks) { Resources resources = task.resources(); if (task.has_executor()) { resources += task.executor().resources(); } foreach (const Resource& volume, destroy.volumes()) { if (resources.contains(volume)) { return Error("Persistent volume in pending tasks"); } } } } ``` - Jiang Yan Xu On Aug. 31, 2016, 2:17 a.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45961/ > ----------------------------------------------------------- > > (Updated Aug. 31, 2016, 2:17 a.m.) > > > Review request for mesos, Benjamin Mahler, Joris Van Remoortere, Michael > Park, and Jiang Yan Xu. > > > Bugs: MESOS-4431 > https://issues.apache.org/jira/browse/MESOS-4431 > > > Repository: mesos > > > Description > ------- > > o Each shared resource is accouted via its share count. This count is > updated based on the resource operations (such as add and subtract) > in various scenarios such as task launch and terminate at multiple > modules such as master, allocator, sorter, etc. > o Only allow DESTROY if there are no running or pending tasks using > the volume. However, if the volume is in a pending offer to one or > more frameworks, rescind that offer before processing the DESTROY. > o To allow multiple tasks to be launched in the same ACCEPT call > using the same shared resource, we update the allocator and > sorter with additional copies of shared resources to reflect the > true shared count of allocated shared resources. > > > Diffs > ----- > > src/common/resources.cpp a5f5902d8f7f2757e3aee35619bff5cc3a52f29b > src/common/resources_utils.cpp 8e881a09500b0966b577e514077460f724868a8d > src/master/allocator/mesos/hierarchical.hpp > dd07ed221d2c1755d2478369641ffdc46ecc4471 > src/master/allocator/mesos/hierarchical.cpp > 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 > src/master/allocator/sorter/drf/sorter.hpp > 09aa685f2bd7197385959d7d70d5411d0fd72f06 > src/master/allocator/sorter/drf/sorter.cpp > 1f2954c564eea5a3c649a5cc7181cb69329f9e35 > src/master/http.cpp 525ef6cd7545d25f3ac89a6325ace6e6d768262a > src/master/master.hpp c32c7e9d859ef73216354e2c03ecc07d0009b12f > src/master/master.cpp 192b2ce5eb24deb6e843f2f1a8c915114614b268 > src/master/validation.hpp 3205ee1e699d0fb7ca16ced96a07be3a07cf8956 > src/master/validation.cpp 15e2ecc2de99d6bed522f45f855ba686bf19c008 > src/tests/master_validation_tests.cpp > e440ff3f6d04f797e65874b9f610ed63d9f28e0e > src/tests/sorter_tests.cpp 821e30d6574b045d25d4de4f7c3b8ac5346d3002 > src/v1/resources.cpp 172217505d80d66cb7e10b3635dc273229313601 > > Diff: https://reviews.apache.org/r/45961/diff/ > > > Testing > ------- > > Tests successful. > > > Thanks, > > Anindya Sinha > >
