Re: Review Request 70972: Added helpers to add and remove master minimum capabilities.

[Benjamin Mahler <bm...@apache.org>]

-----------------------------------------------------------
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
> 
>