----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28698/#review65561 -----------------------------------------------------------
First pass. I'll go over it again once I've read the rest of the patches and you've responded to these comments. include/mesos/resources.hpp <https://reviews.apache.org/r/28698/#comment108826> So you expect that this will be called by something (e.g. checkpoint recovery) that already knows whether to interpret the strings as a static or dynamic reservation? include/mesos/resources.hpp <https://reviews.apache.org/r/28698/#comment108827> Does this always assume STATIC? Shouldn't you be able to parse a string into name/value/role while also specifying the reservation type? include/mesos/resources.hpp <https://reviews.apache.org/r/28698/#comment108786> Should this be updated to be a `hashmap<role, hashmap<reservation, Resources>>`? Or is it good enough to leave this hashmap only partitioned by role? src/common/resources.cpp <https://reviews.apache.org/r/28698/#comment108828> I'd agree that ignoring reservations and returning all resources for that role makes sense, although it seems `Resources::reserved` is only called twice, once in allocate() where you wouldn't care what kind of reservation it is (and do want to include dynamic reservations), and once in Filter, where you are explicitly adding the reservation type in your patch. I doubt that anybody is using `reserved(role)` in their frameworks, but maybe they'd appreciate a note in the upgrades doc. src/common/resources.cpp <https://reviews.apache.org/r/28698/#comment108787> Is CHECK appropriate here, or should we instead return a Try<Resources>? src/common/resources.cpp <https://reviews.apache.org/r/28698/#comment108812> Maybe this should go in an inner namespace (or within the Resources class), since it's only used here in Resources::find(), and shouldn't dominate the Mesos::Filter namespace. src/common/resources.cpp <https://reviews.apache.org/r/28698/#comment108806> No need for `explicit` on a two-parameter constructor src/common/resources.cpp <https://reviews.apache.org/r/28698/#comment108808> `const Option<string>& _role = None(),`? Then you wouldn't need the default constructor (which should maybe initialize role+reservation to None() anyway?) src/common/resources.cpp <https://reviews.apache.org/r/28698/#comment108816> CHECK might be too strong here (and below). Perhaps return a `Try<Resources>`? src/common/resources.cpp <https://reviews.apache.org/r/28698/#comment108795> Blank line before comment src/common/resources.cpp <https://reviews.apache.org/r/28698/#comment108796> Blank line before comment src/common/resources.cpp <https://reviews.apache.org/r/28698/#comment108799> Do we officially support initializer_list (in gcc 4.4)? It doesn't show up on the approved list on http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/ src/common/resources.cpp <https://reviews.apache.org/r/28698/#comment108801> 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. - Adam B On Dec. 16, 2014, 4:56 p.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28698/ > ----------------------------------------------------------- > > (Updated Dec. 16, 2014, 4:56 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 296553af93ec8457dad04ac018f03f36df654fdc > src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92 > > Diff: https://reviews.apache.org/r/28698/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Michael Park > >
