> 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? > > Michael Park wrote: > 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.
> 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. If I understand you correctly, you mean that an operator may reserve resources for multiple roles in one request (while the framework can't). First, you don't have a test for this case : ). Second, if we group resources by roles and call `applyOfferOperation()` per role, can we avoid modifying validation function? I think you'll need to do the grouping at some point in order to update `roleSorter`, so why not doing it earlier and therefore keep framework-initiated and operator-initiated requests closer to each other? You already group them implicitly by `SlaveID`. > 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. Let's capture this in the user guide! It also means that if all frameworks in the role are beyond their fare share, they are not able to dynamically reserve (because they are not getting offers), but an operator can always reserve, right? I also try to understand, whether we can refactor the code in a way that we do not introduce one more method on the allocator. Remember, it's a public interface and folks are supposed to write their own implementations, so we should be careful when changing the API. - Alexander ----------------------------------------------------------- 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 > >
