----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66982/#review202540 -----------------------------------------------------------
include/mesos/allocator/allocator.hpp Lines 372 (patched) <https://reviews.apache.org/r/66982/#comment284358> Could we not add a default argument here? From an interface perspective adding a new argument with a default value is not nice to anybody implementing an allocator along this interface * they will need to carefully maintain the exact same default argument in their derived class to avoid suprising behavior * it can lead to surprising ABI errors when the dependent module is not recompiled and the faulty override is not caught. Even though we have default arguments in some functions here, it would be great to not add to that bad practice unless we have a very strong argument in favor of doing so. Also see e.g., https://herbsutter.com/2013/05/22/gotw-5-solution-overriding-virtual-functions/. * * * Please also call out this change in the `CHANGELOG` as it pertains to a module interface. - Benjamin Bannier On May 7, 2018, 8:09 a.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66982/ > ----------------------------------------------------------- > > (Updated May 7, 2018, 8:09 a.m.) > > > Review request for mesos, Benjamin Mahler, Kapil Arya, Till Toenshoff, and > Vinod Kone. > > > Repository: mesos > > > Description > ------- > > The `decline` flag acts as an hint to the allocator that this resource > recover is triggered by the framework "declining" the offered resources. > Currently this includes: framework `DECLINE` calls, framework `ACCEPT` > calls with filters explicitly set and offer timeouts. Note, framework > `ACCEPT` calls with unset filters (i.e. default filters) are not > considered as "declining" offered resources. > > > Diffs > ----- > > include/mesos/allocator/allocator.hpp > e5a5355646c834280008867e89eb258eaefc2f1d > src/master/allocator/mesos/allocator.hpp > c453c015b234deff7efd00269da25dcec8cbf1ae > src/master/allocator/mesos/hierarchical.hpp > 955ae3e6a9e3c790fb260311ec4b4ef725a826d3 > src/master/allocator/mesos/hierarchical.cpp > 1000968be6a2935a4cac571414d7f06d7df7acf0 > src/master/master.cpp 3b5d2eba3f602f68a6bb1e00444b01fb58a1bfc2 > src/tests/allocator.hpp 341efa665ad0ce897e087fb8d73ec50fd041d559 > src/tests/master_allocator_tests.cpp > e1aef8a9625a805e7ad2dfad37bfeedee82f160d > src/tests/slave_recovery_tests.cpp afe8b8a6cc21421a37164016d7fe01bf96a559da > > > Diff: https://reviews.apache.org/r/66982/diff/1/ > > > Testing > ------- > > make check > > > Thanks, > > Meng Zhu > >
