> On Dec. 19, 2014, 2:05 a.m., Adam B wrote: > > src/common/resources.cpp, line 492 > > <https://reviews.apache.org/r/28698/diff/2/?file=793362#file793362line492> > > > > Is CHECK appropriate here, or should we instead return a Try<Resources>?
I think returning a `Try<Resources>` may be better, but I think that probably breaks frameworks. What's our preferred solution in these situations? > On Dec. 19, 2014, 2:05 a.m., Adam B wrote: > > src/common/resources.cpp, line 512 > > <https://reviews.apache.org/r/28698/diff/2/?file=793362#file793362line512> > > > > No need for `explicit` on a two-parameter constructor In this case, it's such a narrow scoped utility that it probably doesn't matter. In terms of coding practice, I tend to make implicit conversions be the special cases rather than have them by default. Also an update with C++11. With [copy-list-initialization](http://en.cppreference.com/w/cpp/language/list_initialization), `explicit` on constructors with multiple parameters do have effect. The following code compiles. ``` class Foo { public: Foo(int, int) {} }; void F(const Foo&) {} int main() { F({0, 0}); } ``` `Foo(int, int)` needs to be `explicit` to disallow this to compile. > On Dec. 19, 2014, 2:05 a.m., Adam B wrote: > > src/common/resources.cpp, line 513 > > <https://reviews.apache.org/r/28698/diff/2/?file=793362#file793362line513> > > > > `const Option<string>& _role = None(),`? > > Then you wouldn't need the default constructor (which should maybe > > initialize role+reservation to None() anyway?) By providing only the default constructor which makes `(role=None, reservationType=None)`, and this constructor which makes `(role=Some, reservationType=None/Some)`, We can guarantee that we never get `(role=None, reservationType=Some)`. This is why I used `CHECK(role.isSome())` below. > On Dec. 19, 2014, 2:05 a.m., Adam B wrote: > > src/common/resources.cpp, line 523 > > <https://reviews.apache.org/r/28698/diff/2/?file=793362#file793362line523> > > > > CHECK might be too strong here (and below). Perhaps return a > > `Try<Resources>`? We provide restricted constructors to guarantee states of the object as explained above. I think `CHECK` is suitable in this case. > On Dec. 19, 2014, 2:05 a.m., Adam B wrote: > > src/common/resources.cpp, lines 556-557 > > <https://reviews.apache.org/r/28698/diff/2/?file=793362#file793362line556> > > > > If target.reservation == STATIC, aren't these two the same? (ditto for > > if target.role=="*") > > But I guess the `total -= resource` prevents us from adding a resource > > twice anyways. Yes, yes. This was essentially how it was done before so I just kept it. I was thinking of actually fixing this to partition the `total` sorted by preference and iterate over it one time rather than doing all the `total -= resource`, etc. But I do like how the list of filters clearly show the contraints being relaxed. I can add a comment here to help readability. - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28698/#review65561 ----------------------------------------------------------- On Dec. 30, 2014, 11:51 p.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28698/ > ----------------------------------------------------------- > > (Updated Dec. 30, 2014, 11:51 p.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, and > Vinod Kone. > > > Repository: mesos-git > > > Description > ------- > > Modified Resources to account for reservation type. > > > Diffs > ----- > > include/mesos/resources.hpp f1517b73def9aff4039f95a89f66208ba1d21c0f > src/common/resources.cpp c17e1791130e7d545bb7cdd54d97d65325d3a69e > > Diff: https://reviews.apache.org/r/28698/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Michael Park > >
