> On 八月 1, 2016, 10:39 p.m., Jiang Yan Xu wrote: > > Can we keep add/subtract private? The fact they were added to save some > > redudant work suggests their `private` nature. Otherwise what if people use > > `add` directly instead of `+=`? > > > > Looks like they were changed to public just so it can be called from the > > free-standing `convertJSON`, which is a private util method itself. We can > > just friend it right? > > Benjamin Mahler wrote: > Making it private sounds good and having a friend sounds good. > > Guangya Liu wrote: > Thanks Jinag Yan and Ben, I will follow up a patch later. > > Jiang Yan Xu wrote: > Hey Guangya have you started on it? If not we can take care of it because > we'd like to also consolidate `Resources::add(const Resource& r)` and > `add(const Resource_& r);` > > Guangya Liu wrote: > Yes, I was working on this but found one issue and still checking. > Currently, the function `static Try<Resources> convertJSON` is a `static` > function and the `friend` do not support `static` function so here we may > need to make the `Try<Resources> convertJSON` as a non-static function if we > want to make it a `friend`, what do you think of this? > > BTW: Can we make this and `consolidate Resources::add(const Resource& r) > and add(const Resource_& r);` as separate patches? > > Guangya Liu wrote: > Jiang Yan, I posted a patch here https://reviews.apache.org/r/50836/ and > added you as reviewer, can you help check? Thanks. > > Guangya Liu wrote: > @Ben and @Yan, I found another place where we can enhance for allocator > https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274 > and this was only used by quota now, I think we can use `add` here. The > problem is that @Yan porposed to make the `add` as a private method and I've > posted a patch here https://reviews.apache.org/r/50836/diff/1#index_header , > but after a second thought, perhaps we can enable framework developers or > other users call this `add` API directly. As sometimes, the user may want to > traverse all `Resources` object and make some update for each `Resource` > object and then add those `Resource` again just like what we did in allocator > for now. So what about keep `add` as a public method?
Had some offline discussion with Ben, we will make those two methods `add/subtract` as private for now as it only impacts `quota` related logic in allocator. And we can add some other wrapper functions to expose those two APIs if they are needed in future. - Guangya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50568/#review144394 ----------------------------------------------------------- On 七月 29, 2016, 1:34 a.m., Guangya Liu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50568/ > ----------------------------------------------------------- > > (Updated 七月 29, 2016, 1:34 a.m.) > > > Review request for mesos, Benjamin Mahler, Joris Van Remoortere, and Klaus Ma. > > > Repository: mesos > > > Description > ------- > > This is a follow up action for MESOS-5919. Based on the patch of > https://reviews.apache.org/r/50553/ and > https://reviews.apache.org/r/50557/ , we should update -=/+= to > subtract/add if the resource object is from resources object. > > > Diffs > ----- > > include/mesos/resources.hpp 88a9feabf66ed34e7e5b1c6cb7e831818e7f7883 > include/mesos/v1/resources.hpp 054ed00a03319ae5e350542add34f497eaf79152 > src/common/resources.cpp 3dbff24d6859d3b1ed8589cec50170a5202cfbcb > src/v1/resources.cpp 3c85dc8aa8125962b44e60806ece83a7653d0dc7 > > Diff: https://reviews.apache.org/r/50568/diff/ > > > Testing > ------- > > make > make check > > > Thanks, > > Guangya Liu > >
