----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60022/#review178170 -----------------------------------------------------------
src/common/resources_utils.hpp Lines 61 (patched) <https://reviews.apache.org/r/60022/#comment252024> Typo src/common/resources_utils.hpp Lines 63 (patched) <https://reviews.apache.org/r/60022/#comment252025> Can we remove the 3x repitition of this? Seems sufficient to have the header comment. src/common/resources_utils.hpp Lines 75 (patched) <https://reviews.apache.org/r/60022/#comment252026> Typo. src/common/resources_utils.hpp Lines 76 (patched) <https://reviews.apache.org/r/60022/#comment252027> We also do this for frameworks that send old resources, right? i.e., clarify that the old-agent conversion discussion is an example. src/common/resources_utils.hpp Lines 96 (patched) <https://reviews.apache.org/r/60022/#comment252029> Can we clarify this slightly? "We inject the resources with the "pre-reservation-refinement" format to enable backward compatibility with external tooling. If the master has been upgraded to a version that supports reservation refinement but no refined reservations have been made, the endpoints will return the data in both the new and old formats to maximize backward compatibility. However, once a reservation refinement is made to a resource, that resource is only returned in the new format." src/common/resources_utils.hpp Lines 103 (patched) <https://reviews.apache.org/r/60022/#comment252030> "Converts the given `Resource` to the specified `ResourceFormat`." src/common/resources_utils.hpp Lines 105 (patched) <https://reviews.apache.org/r/60022/#comment252031> I'd remove these comments, they seem confusing/redundant with the discussion above. src/common/resources_utils.cpp Lines 87 (patched) <https://reviews.apache.org/r/60022/#comment252035> I'm confused by this function's precondition. When converting to `PRE_RESERVATION_REFINEMENT`, the resources _cannot_ be in the pre-refined format; but when converting to `POST_RESERVATION_REFINEMENT`, the resources _can_ be in the post-refined format. Seems inconsistent. src/common/resources_utils.cpp Lines 125 (patched) <https://reviews.apache.org/r/60022/#comment252032> Typo src/common/resources_utils.cpp Lines 135 (patched) <https://reviews.apache.org/r/60022/#comment252034> "we're in" src/common/resources_utils.cpp Lines 158 (patched) <https://reviews.apache.org/r/60022/#comment252033> I feel like `reservation->CopyFrom(resource->reservation())` would be more idiomatic...? - Neil Conway On June 15, 2017, 8:48 a.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60022/ > ----------------------------------------------------------- > > (Updated June 15, 2017, 8:48 a.m.) > > > Review request for mesos, Benjamin Mahler and Neil Conway. > > > Bugs: MESOS-7655 > https://issues.apache.org/jira/browse/MESOS-7655 > > > Repository: mesos > > > Description > ------- > > With reservation refinement, we introduced a new resource format to > enable creating a refined reservation on top of an existing reservation. > We still have the "pre-reservation-refinement" format which uses > the `Resource.role` and `Resource.reservation` fields, and we now also > have the "post-reservation-refinement" format which uses > the `Resource.reservations` field to represent the reservation state. > > In order to simplify our code, we use `convertResourceFormat` to > canonicalize the resources into the "post-reservation-refinement" in > memory, and convert them back as necessary. > > > Diffs > ----- > > src/common/resources_utils.hpp 2840f459288bbe8440eda08119d4f86a8be5a291 > src/common/resources_utils.cpp c3088433d8c147b06dbef6f310fbe4059c3dc0ba > > > Diff: https://reviews.apache.org/r/60022/diff/8/ > > > Testing > ------- > > > Thanks, > > Michael Park > >
