----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67671/#review205772 -----------------------------------------------------------
Fix it, then Ship it! src/resource_provider/registry.hpp Lines 21 (patched) <https://reviews.apache.org/r/67671/#comment288675> This is not used. I guess you probably has used this originally but changed the implementation to use explicit comparison, which seems better as the code doesn't rely on the semantics of the message differencer. src/resource_provider/registry.hpp Lines 38-50 (patched) <https://reviews.apache.org/r/67671/#comment288677> We could write the code in a style similar to `src/common/type_utils.cpp` is: ``` return left.id() == right.id() && (!left.has_type() || !right.has_type() || left.type() == right.type()) && (!left.has_name() || !right.has_name() || left.name() == right.name())); ``` But whether this is more readable or not is subjective so not opening an issue here. src/resource_provider/registry.hpp Lines 66 (patched) <https://reviews.apache.org/r/67671/#comment288678> Would `google::protobuf::util::MessageToJsonString` be better here? src/resource_provider/registry.proto Lines 32-33 (patched) <https://reviews.apache.org/r/67671/#comment288674> Let's swap the order: ``` optional string type = 2; optional string name = 3; ``` - Chun-Hung Hsiao On June 20, 2018, 12:35 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67671/ > ----------------------------------------------------------- > > (Updated June 20, 2018, 12:35 p.m.) > > > Review request for mesos, Chun-Hung Hsiao and Jan Schlicht. > > > Bugs: MESOS-8838 > https://issues.apache.org/jira/browse/MESOS-8838 > > > Repository: mesos > > > Description > ------- > > Since the agent uses e.g., a resource provider's name or type to > construct paths to persist resource provider state, changes to this > information on resource provider resubscription are not supported. > > This patch persists a resource provider's name and type in the > resource provider registry and rejects a resource provider > resubscription if incompatible changes are detected. Since we did not > persist this information previous to mesos-1.7.0 we cannot and do not > perform validation against resource provider registry information > stored with earlier versions of Mesos. > > > Diffs > ----- > > src/resource_provider/manager.cpp 6400e707ee834c73d4bf18c1f8d8344e8cf714cc > src/resource_provider/registrar.hpp > ded56e1c24a1f8b8db8dc70151682a7deb9e6544 > src/resource_provider/registrar.cpp > a855a2b61fdbfc96a12a133c702ecaf7af666d8b > src/resource_provider/registry.hpp 4c6c4d40686e3af8bbc7affbe3fa673479cc2fbf > src/resource_provider/registry.proto > 491263ef679cd4cea59338b002c6845d890f8eae > src/tests/resource_provider_manager_tests.cpp > 58bdbf0e88f59b5cb0cad42e38b24029fc0f2b41 > > > Diff: https://reviews.apache.org/r/67671/diff/1/ > > > Testing > ------- > > `sudo make check` > > > Thanks, > > Benjamin Bannier > >
