----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71486/#review217797 -----------------------------------------------------------
Looking at the next patch, it looks like we use `recoverResources(....true)` to untrack the allocated resources. And use a separate function to `trackAllocated`. I think this asymmetry will definitely lead to some frowns on caller/reader. I would suggest let's do an explicitly untrack independent of the recover call. Reasons being: - The symmetry would be easier to read and less error-prone (forgetting to call untrack) - It clearly indicates that `allocated` is a `tracked` value that passively follows the master action - It avoid any changes to the current recoverResources One minor concern is that people might forget to call track/Untrack altogether. If we can have a launch helper or something in the master, that would be great. But don't worry too much about it if it is non-trivial work. include/mesos/allocator/allocator.hpp Lines 393 (patched) <https://reviews.apache.org/r/71486/#comment305186> I know it is for backward compatibility. But it is strange to have a default value here in the first glance. It also forces callers to be explicit about setting the field correctly. Also, I think it is generally not a good idea to add default arguments to the interface. This comment might not be relevant, see my top-level comment. - Meng Zhu On Sept. 17, 2019, 6:30 a.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71486/ > ----------------------------------------------------------- > > (Updated Sept. 17, 2019, 6:30 a.m.) > > > Review request for mesos, Benjamin Mahler and Meng Zhu. > > > Bugs: MESOS-9949 > https://issues.apache.org/jira/browse/MESOS-9949 > > > Repository: mesos > > > Description > ------- > > This patch extends the signature of `recoverResources()` with a flag > indicating wheteher the resources being recovered were actaully > allocated to a framework, or only offered. > > This is a prerequisite for tracking quota consumption in the allocator. > > > Diffs > ----- > > include/mesos/allocator/allocator.hpp > 2bab53ab5fb25931a724c20a039e1301983ba574 > src/master/allocator/mesos/allocator.hpp > 6921581745bf876ee42bcfb62b59245f23fcbf47 > src/master/allocator/mesos/hierarchical.hpp > 5ea37912dadded47eaa2d354889c95533b191c59 > src/master/allocator/mesos/hierarchical.cpp > 256fdce61ccfb768605cac1579c9a6632cd26fec > src/master/master.cpp a2c289a8b6e30eac690f77c7fc1b12b16f62f541 > src/tests/allocator.hpp 01e6d2ce3d0655ad408f605c7be84cd7c0f0d479 > src/tests/master_allocator_tests.cpp > e9607577c57c4ae36133a649667b5a416ba9dece > src/tests/slave_recovery_tests.cpp d99752ab082f1aca9fb77659378d0bca5a0598eb > > > Diff: https://reviews.apache.org/r/71486/diff/2/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
