----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51412/#review146789 -----------------------------------------------------------
Fix it, then Ship it! > This is being added to allow for handling of ACCEPT call within the allocator in the future. For now, we expose the offered resources in this API to account for additional copies of shared resources in the allocator when there are more tasks being launched in a single ACCEPT call for a single copy of shared resource being offered. I feel the summary too specific and it sounds like we are changing a generic API for only one purpose. One of the main reasons we chose to add this is because it can be used for more than shared resources. So how about this as the summary: ``` Operations are applied to specific offered resources which are a subset of all resources allocated to the framework. For RESERVE/UNRESERVE/CREATE/DESTROY it's not necessary to make such distinction but we need it for future work such as supporting shared resoruces. ``` include/mesos/allocator/allocator.hpp (lines 255 - 257) <https://reviews.apache.org/r/51412/#comment213461> > The offeredResources is passed to this function to allow the allocator handle ACCEPT calls. The same to true for every other argument? I think it suffices to simply state: ``` The `offeredResources` are the resources that these operations are applied to. ``` include/mesos/allocator/allocator.hpp (lines 262 - 263) <https://reviews.apache.org/r/51412/#comment213464> Can we swap the order of the two arguments? Offered resources (or offer ID) go before the list of operations in the protobuf `Accept` and master method `_accept()`. It's probably more important when the method is changed to `Allocator::accept()` but if we do it now we don't need to swap them later. src/master/allocator/mesos/hierarchical.cpp (lines 616 - 617) <https://reviews.apache.org/r/51412/#comment213521> It's not clear to me what "handle ACCEPT related operations" implies. If we want to use a TODO to justify the `offeredResources` I think we can just say: ``` // TODO(anindya_sinha): `offeredResources` will be useful for supporting LAUNCH operations involving shared resources. ``` src/master/allocator/mesos/hierarchical.cpp (line 617) <https://reviews.apache.org/r/51412/#comment213508> > ACCEPT You meant LAUNCH? src/master/master.hpp <https://reviews.apache.org/r/51412/#comment213470> Move the comments to `_apply()`? ``` // Updates the slave's resources by // applying the given operation. It also sends a // 'CheckpointResourcesMessage' to the slave with the updated // checkpointed resources. ``` I think it's worth documenting `_apply()` because of the "side effect": sending CheckpointResourcesMessage. src/master/master.cpp (line 3556) <https://reviews.apache.org/r/51412/#comment213465> Add the following? ``` // Note that this list could be different than `accept.operations()` as we drop invalid operations. However the order should remain unchanged. ``` src/master/master.cpp (line 4095) <https://reviews.apache.org/r/51412/#comment213471> s/the operations/the valid operations/ - Jiang Yan Xu On Aug. 25, 2016, 8:51 a.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51412/ > ----------------------------------------------------------- > > (Updated Aug. 25, 2016, 8:51 a.m.) > > > Review request for mesos, Benjamin Mahler and Jiang Yan Xu. > > > Bugs: MESOS-4431 > https://issues.apache.org/jira/browse/MESOS-4431 > > > Repository: mesos > > > Description > ------- > > This is being added to allow for handling of ACCEPT call within the > allocator in the future. For now, we expose the offered resources > in this API to account for additional copies of shared resources in > the allocator when there are more tasks being launched in a single > ACCEPT call for a single copy of shared resource being offered. > > > Diffs > ----- > > include/mesos/allocator/allocator.hpp > 41a9d457286e30431490ca626e680d85684b48d6 > src/master/allocator/mesos/allocator.hpp > 69639be9c14b83157df29f767e819db719588053 > src/master/allocator/mesos/hierarchical.hpp > bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 > src/master/allocator/mesos/hierarchical.cpp > 234ef98529964a0b6d3f132426a6c8ccbb1263ee > src/master/master.hpp 6decff6f4b9c3434de030fd5c06df4c683a7abad > src/master/master.cpp 789fb9760a5ea60ce3b7b08f17e36cec69349c60 > src/tests/allocator.hpp 5ca3057265eb2c00c965132ac7922019e1cc77b8 > src/tests/hierarchical_allocator_tests.cpp > cbed333f497016fe2811f755028796012b41db77 > src/tests/reservation_tests.cpp 6181c6f8abbf254b45ea77f33c48dee918106ef0 > > Diff: https://reviews.apache.org/r/51412/diff/ > > > Testing > ------- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
