----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70972/#review216276 -----------------------------------------------------------
Fix it, then Ship it! Is there a patch with the appropriate tests added to protobuf_utils_tests.cpp? src/common/protobuf_utils.hpp Lines 483 (patched) <https://reviews.apache.org/r/70972/#comment303408> boolean_condition_for_adding_the_capability hinders this, just say? ``` // capabilities.quotaV2 = needsV2; ``` src/common/protobuf_utils.hpp Lines 492 (patched) <https://reviews.apache.org/r/70972/#comment303409> s/mis/miss/ src/common/protobuf_utils.hpp Lines 501-502 (patched) <https://reviews.apache.org/r/70972/#comment303410> Can you wrap / phrase it consistently with the comment above? (e.g. "it is a noop if already absent.") src/common/protobuf_utils.cpp Lines 1367 (patched) <https://reviews.apache.org/r/70972/#comment303411> This is copying since this function returns a const ref, so use const ref: ``` const string& s = MasterInfo_Capability_Type_Name(capability); ``` Or, do you even need if if the wrapping is fixed? Either way, would be good to be consistent between these two functions (either use a variable in both, or don't in both). src/common/protobuf_utils.cpp Lines 1369-1375 (patched) <https://reviews.apache.org/r/70972/#comment303412> Fix the wrapping to be the same as the one below? - Benjamin Mahler On June 28, 2019, 9:40 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70972/ > ----------------------------------------------------------- > > (Updated June 28, 2019, 9:40 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-9601 > https://issues.apache.org/jira/browse/MESOS-9601 > > > Repository: mesos > > > Description > ------- > > Also added a TODO about refactoring the helpers. > > > Diffs > ----- > > src/common/protobuf_utils.hpp f6ea9230d38079b24060922872ee93d9f038b98e > src/common/protobuf_utils.cpp 9ff0cf59c658c7c6a3a439a77aff13aff3c20fe5 > > > Diff: https://reviews.apache.org/r/70972/diff/1/ > > > Testing > ------- > > make check > The call is used in subsequent patches which have dedicated tests for > checking the minimum capabilities. > > > Thanks, > > Meng Zhu > >
