----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64303/#review192962 -----------------------------------------------------------
Looks good! Just some minor suggestions below src/master/allocator/mesos/hierarchical.hpp Lines 453-457 (patched) <https://reviews.apache.org/r/64303/#comment271372> Can you comment on the lifecycle of the entries in the map? My intuition would guess that we only store entries when they have reservations, if a role doesn't have any reservations we don't bother putting it in this map? i.e. there will never be an empty resources, in that case the key will be absent? src/master/allocator/mesos/hierarchical.hpp Lines 546-555 (patched) <https://reviews.apache.org/r/64303/#comment271373> Perhaps a TODO here that reflects your plan based on the ticket for fixing the fairness issues when there are unallocated reservations? Would be helpful for others. src/master/allocator/mesos/hierarchical.hpp Lines 552-555 (patched) <https://reviews.apache.org/r/64303/#comment271375> Also, as it stands it's a little unclear naming-wise to me that that `updateReservations` has a relationship to the track and untrack functions. But maybe we can just simplify and remove this? In favor of just having the caller untrack old and track the new. src/master/allocator/mesos/hierarchical.cpp Lines 2365-2366 (patched) <https://reviews.apache.org/r/64303/#comment271838> Can you match the function order in the header? So: track defined before untrack src/master/allocator/mesos/hierarchical.cpp Lines 2368-2369 (patched) <https://reviews.apache.org/r/64303/#comment271839> s/name/role/ here and elsewhere src/master/allocator/mesos/hierarchical.cpp Lines 2374 (patched) <https://reviews.apache.org/r/64303/#comment271840> how about scalarQuantitesToUntrack? s/&// src/master/allocator/mesos/hierarchical.cpp Lines 2374-2375 (patched) <https://reviews.apache.org/r/64303/#comment271844> A note here that we need to call toUnreserved to strip the static reservation metadata would be helpful for the reader. src/master/allocator/mesos/hierarchical.cpp Lines 2391 (patched) <https://reviews.apache.org/r/64303/#comment271841> how about scalarQuantitesToTrack? s/&// src/master/allocator/mesos/hierarchical.cpp Lines 2391-2392 (patched) <https://reviews.apache.org/r/64303/#comment271845> A note here that we need to call toUnreserved to strip the static reservation metadata would be helpful for the reader. src/master/allocator/mesos/hierarchical.cpp Lines 2394-2398 (patched) <https://reviews.apache.org/r/64303/#comment271842> This can just be: ``` reservationScalarQuantities[name] += scalarQuantitesToTrack; ``` - Benjamin Mahler On Dec. 4, 2017, 11:18 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64303/ > ----------------------------------------------------------- > > (Updated Dec. 4, 2017, 11:18 p.m.) > > > Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Michael > Park. > > > Repository: mesos > > > Description > ------- > > Resource reservations need to be tracked to make > sure quota limit will not be exceeded in the presence > of resource reservation. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.hpp > 41ffe17def133d7e735afa26a6fedd154e51f4b4 > src/master/allocator/mesos/hierarchical.cpp > 20b908cc891f9f9be3045cd9f8be83a11d37ab78 > > > Diff: https://reviews.apache.org/r/64303/diff/2/ > > > Testing > ------- > > > Thanks, > > Meng Zhu > >
