----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53096/#review159578 -----------------------------------------------------------
src/master/allocator/mesos/hierarchical.hpp (lines 477 - 478) <https://reviews.apache.org/r/53096/#comment230630> Update the comment per the comment about the role of the method. ``` // Helper to update the total resources in the allocator and each of the sorters. ``` src/master/allocator/mesos/hierarchical.hpp (line 479) <https://reviews.apache.org/r/53096/#comment230614> 1. No need for the `_` prefix for private methods. We use it for continuations. 2. `oldTotal`? Should be the updated total right? i.e., ``` void updateTotal(const SlaveID& slaveId, const Resources& total); ``` I suggested just naming it `total` because I feel intuitively people would associate the `total` in a method called `updateTotal` to be "the value to update the totals to". What do you think? src/master/allocator/mesos/hierarchical.cpp (lines 731 - 732) <https://reviews.apache.org/r/53096/#comment230624> Move to `updateTotal()`. src/master/allocator/mesos/hierarchical.cpp (line 755) <https://reviews.apache.org/r/53096/#comment230625> Call the this way: ``` updateTotal(slaveId, updatedTotal); ``` And move this to right below `CHECK_SOME(updatedTotal);`. Also we need to shuffle things around a bit so this block of code is cleanly splitted into two sections: ``` // Update the total resources in the allocator and each of the sorters. Try<Resources> updatedTotal = slaves[slaveId].total.apply(operation); CHECK_SOME(updatedTotal); updatedTotal(slaveId, updatedTotal); // Update the allocated resources in the allocator and each of the sorters. Try<Resources> updatedSlaveAllocation = slaves[slaveId].allocated.apply(operation); CHECK_SOME(updatedSlaveAllocation); slaves[slaveId].allocated = updatedSlaveAllocation.get(); Resources frameworkAllocation = frameworkSorter->allocation(frameworkId.value(), slaveId); ... ``` src/master/allocator/mesos/hierarchical.cpp (lines 814 - 815) <https://reviews.apache.org/r/53096/#comment230628> Move over to `updateTotal()`. src/master/allocator/mesos/hierarchical.cpp (line 817) <https://reviews.apache.org/r/53096/#comment230629> Use the same comment: ``` // Update the total resources in the allocator and each of the sorters. ``` src/master/allocator/mesos/hierarchical.cpp (line 1974) <https://reviews.apache.org/r/53096/#comment230626> s/oldTotal/total/ src/master/allocator/mesos/hierarchical.cpp (line 1977) <https://reviews.apache.org/r/53096/#comment230627> As commented above, add ``` const Resources oldTotal = slaves[slaveId].total; slaves[slaveId].total = updatedTotal; ``` src/tests/persistent_volume_tests.cpp (lines 1373 - 1375) <https://reviews.apache.org/r/53096/#comment230633> Move the comments about killing the long-lived task below to where we are actually killing it? Otherwise here you are describing for 4 statuses but are onlying defining 3. src/tests/persistent_volume_tests.cpp (lines 1381 - 1383) <https://reviews.apache.org/r/53096/#comment230631> 2 space indentation. src/tests/persistent_volume_tests.cpp (line 1402) <https://reviews.apache.org/r/53096/#comment230632> Space after `foreach`. src/tests/persistent_volume_tests.cpp (line 1432) <https://reviews.apache.org/r/53096/#comment230634> 2 space indentation. src/tests/persistent_volume_tests.cpp (line 1435) <https://reviews.apache.org/r/53096/#comment230635> 2 space indentation. - Jiang Yan Xu On Dec. 12, 2016, 2:35 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53096/ > ----------------------------------------------------------- > > (Updated Dec. 12, 2016, 2:35 p.m.) > > > Review request for mesos and Jiang Yan Xu. > > > Bugs: MESOS-6444 > https://issues.apache.org/jira/browse/MESOS-6444 > > > Repository: mesos > > > Description > ------- > > We maintain a single copy of shared resource in the role and quota > sorter's total resources. So, when we update these resources, we > remove the previous resources at this agent and add the new resources > at this agent. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.hpp > a6424d624864155e1c87a28a63b784512c5c8722 > src/master/allocator/mesos/hierarchical.cpp > a8cbc8db3d1319718765783827f8be1981b56f04 > src/tests/persistent_volume_tests.cpp > b7c313781f3dd9ab4fd68ce23ccc044d50b734d8 > > Diff: https://reviews.apache.org/r/53096/diff/ > > > Testing > ------- > > Tests passed. > > > Thanks, > > Anindya Sinha > >
