----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60036/#review178026 -----------------------------------------------------------
Fix it, then Ship it! For the commit summary, how about: ``` Avoid exposing Resource.role in JSON endpoints for reservation refinements. ``` "Printing" seems inaccurate? And you can say whether it's a temporary hack (due to the deprecation being deferred) in the description? include/mesos/resources.hpp Lines 664-674 (patched) <https://reviews.apache.org/r/60036/#comment251790> Can you file a ticket about the deprecation (and the details here about why it's critical to mark it as deprecated) with a target version and refer to it here? You'll probably also want another ticket for the protobuf -> json reflection change: don't show defaults for deprecated fields. src/master/http.cpp Line 2329 (original), 2329 (patched) <https://reviews.apache.org/r/60036/#comment251789> I think we need comments on all of these touch points pointing to the ticket. Can you add to the existing tests to ensure that the role isn't showing up in the json? - Benjamin Mahler On June 15, 2017, 7:39 a.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60036/ > ----------------------------------------------------------- > > (Updated June 15, 2017, 7:39 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-7655 > https://issues.apache.org/jira/browse/MESOS-7655 > > > Repository: mesos > > > Description > ------- > > With reservation refinement, we use the `Resource.reservations` field > to express the reservation state. Due to the fact that `Resource.role` > is an optional field with a default value, our generic Protobuf to > JSON converter prints out the `role` field even if it's not set. > In order to mitigate the confusion for the users, we introduce a hack > for now to conditionally remove the `role` field. > > > Diffs > ----- > > include/mesos/resources.hpp 2b55847be79c7930b598ef31c932e8aca0fa73db > src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 > src/slave/http.cpp 78b35865e465ff1e8e7e4950fdb60e3a48b916b6 > > > Diff: https://reviews.apache.org/r/60036/diff/5/ > > > Testing > ------- > > > Thanks, > > Michael Park > >
