> On Aug. 30, 2019, 12:19 a.m., Meng Zhu wrote: > > src/master/allocator/mesos/hierarchical.hpp > > Line 284 (original), 284 (patched) > > <https://reviews.apache.org/r/71398/diff/1/?file=2163095#file2163095line284> > > > > Currently this should just be offered, i.e all call sites are just for > > offered > > ``` > > offered(const Resources& offered) > > ``` > > > > I guess later when we start to track allocated, we can just simply use > > the call, we will want to differentiate between offered and allocated > > anyway. > > > > Also, I think we can go without the `track` here. > > > > ditto below.
Regarding `track`: - first, `trackOfferedOrAllocated()` (formerly `allocate()`) has a counterpart (formerly `unallocate()`), and I need a name for this counterpart. If I rename `allocate()` into `offered()`, then how should I call `unallocate()`? - second, we cannot have a verb in past tense here (`offered()` looks like some kind of getter, not a method which mutates state), however `offer()` also looks weird. `increaseOffered()`/`decreaseOffered()` could be an option, but it would be less consistent with the neighbouring code (and it looks like it tells us something about inner workings of `Slave`, of which we really shouldn't care). That's why I don't think we can go without `track` :( Why not just `trackOffered()`? First, the fact that it is always called with offered resources (even from `addResourceProvider()`) looks like a coincidence. Second, renaming `unallocate()` into `untrackOffered()` will be misleading: in reality, resources untracked here are in **allocated** state in some cases, but in **offered** state in other cases. Although renaming `allocate()` into `trackOffered()` is not misleading, having a pair `trackOffered()`/`untrackOfferedOrAllocated()` looks a bit weird to me in the current curcumstances. Sure, if we add tracking usage to `Slave`, we will no longer have this track/untrack as a pair. We'll need to have `Slave::trackOffered()` and some interface to transition offered into allocated - but due to the reasons mentioned above, I'm not sure that `trackOffered()` is a right thing to do at this point. - Andrei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71398/#review217498 ----------------------------------------------------------- On Aug. 29, 2019, 11:32 a.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71398/ > ----------------------------------------------------------- > > (Updated Aug. 29, 2019, 11:32 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 > ------- > > Renamed allocated into offeredOrAllocated throughout allocator::Slave. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.hpp > 65d103ea18b91adbdde3b0eb85113a1c0f4d990c > src/master/allocator/mesos/hierarchical.cpp > dd73d5b46c95ac7827d39ed93e0da097f4e8937a > > > Diff: https://reviews.apache.org/r/71398/diff/1/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
