> On June 22, 2015, 10:20 a.m., Alexander Rukletsov wrote: > > Before making a thorough review, let's discuss one high level question. > > > > Here is the problem how I understand it: we reserve for roles, but our code > > works mostly with frameworks (allocator methods, Offer protobuf). To > > workaround this you decided to drop role from the reservation request at > > all. Is it right? Don't we want to update at least the role sorter? > > > > If you decide to add the role, we can preserve validation function for > > reservation and split `updateAllocation()` into two parts: role-specific > > and framework specific and use just role-specific variant for both > > operator-initiated reservations and framework-initiated reservations. > > > > Thoughts?
Thanks for the questions! > Here is the problem how I understand it: we reserve for roles, but our code > works mostly with frameworks (allocator methods, Offer protobuf). To > workaround this you decided to drop role from the reservation request at all. > Is it right? No, the desired roles are specified in the `resources` query parameter. If you're wondering why the validation for `Reserve` now takes an optional role, that's because the master endpoint doesn't have a `role` to which all `resources` need to match. Recall that a framework always has a `role` to which all specified resources need to match. > Don't we want to update at least the role sorter? Yes, you're right about this. I do need to update the role sorter's total. > If you decide to add the role, we can preserve validation function for > reservation Hopefully my reply above clarifies the reason for the changes made to the `validate` function. > ... and split `updateAllocation()` into two parts: role-specific and > framework specific and use just role-specific variant for both > operator-initiated reservations and framework-initiated reservations. There's a fundamental difference between operator-initiated reservations and framework-initiated reservations, in that operator-initiated reservations modify the available (unallocated) resources, whereas framework-initiated reservations modify the allocated resources. `updateAvailble` and `updateAllocation` handle each of the cases. The following are some of the differing characteristics: `updateAvailable` handles the former case: (1) the `available` resources change (2) `frameworkSorter` is unaffected (3) only the total of `roleSorter` is affected, the allocations are not. `updateAllocation` handles the latter case: (1) the `available` resources do not change. (2) both `frameworkSorter` and `roleSorter` are affected. - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35702/#review88760 ----------------------------------------------------------- On June 22, 2015, 5:29 a.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35702/ > ----------------------------------------------------------- > > (Updated June 22, 2015, 5:29 a.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, Joris > Van Remoortere, and Vinod Kone. > > > Bugs: MESOS-2600 > https://issues.apache.org/jira/browse/MESOS-2600 > > > Repository: mesos > > > Description > ------- > > This is still a work in progress, but I'm sharing to gather some feedback > around: > > (1) I've added `updateAvailable` to the allocator API. Is this necessary or > is there maybe a better way? > (2) To determine the amount of available resources, I used: `available = > total - (offered + used)`. > Is this still correct with oversubscription in the picture? > (3) I'm creating an `Offer.Operation` to perform the necessary updates in the > allocator and such. > Feels weird to use an "offer" operation when there's not an actual offer. > Is this fine for now? > > > Diffs > ----- > > include/mesos/master/allocator.hpp 92de1af414321281b00eaa6f129e5e3e2c849448 > src/Makefile.am dfebd2b14c9cb45c437509809fdf5ac3b0c8838c > src/master/allocator/mesos/allocator.hpp > 6cfa04650d91a80211cfbc0809236f9438926c78 > src/master/allocator/mesos/hierarchical.hpp > 7097482fc0adad1c177c15c35edd51c10754f89c > src/master/http.cpp b893013ddd052cb58c520ac0328f4a5f0fed862e > src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064 > src/master/master.cpp 0135c155181546d3cb43e9e05bb874af846d928d > src/master/validation.hpp 469d6f56c3de28a34177124aae81ce24cb4ad160 > src/master/validation.cpp 9d128aa1b349b018b8e4a1916434d848761ca051 > src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 > src/tests/reserve_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/35702/diff/ > > > Testing > ------- > > Added `src/tests/reserve_tests.cpp`. > > > Thanks, > > Michael Park > >
