mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 70951: Updated registry operation `UpdateQuota` to persist `QuotaConfig`.
Date Thu, 27 Jun 2019 22:14:52 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70951/#review216206
-----------------------------------------------------------




src/master/quota.hpp
Lines 64-65 (original), 58-59 (patched)
<https://reviews.apache.org/r/70951/#comment303296>

    I don't think we need to mention RepeatedPtrField here, you can just say "multiple"
    
    But, do we need to take RepeatedPtrField? Seems like it should just take std::vector?



src/master/quota.hpp
Line 89 (original), 68 (patched)
<https://reviews.apache.org/r/70951/#comment303297>

    Why not std::vector?



src/master/quota.cpp
Line 57 (original), 59 (patched)
<https://reviews.apache.org/r/70951/#comment303299>

    After looking at the complexity of returning whether there is mutation, I think we can
just always return true with a TODO. (up to you)
    
    My hunch is we'll maybe just remove the mutation return value, unless there's some real
world optimization to be gained from it.



src/master/quota.cpp
Lines 66-72 (patched)
<https://reviews.apache.org/r/70951/#comment303301>

    Wrap at 4:
    
    ```
        int configIndex = find_if(
            registryConfigs.begin(),
            registryConfigs.end(),
            [&](const QuotaConfig& config_) {
              return config_.role() == config.role();
            })
          - registryConfigs.begin();
    ```



src/master/quota.cpp
Lines 97-103 (patched)
<https://reviews.apache.org/r/70951/#comment303303>

    Ditto w.r.t. wrapping



src/master/quota.cpp
Lines 75-89 (original), 113-141 (patched)
<https://reviews.apache.org/r/70951/#comment303298>

    Let's pull something out for this, since it's going to come up again.
    
    E.g.
    
    ```
    protobuf::master::Capabilities capabilities = registry->minimum_capabilities();
    
    capabilities.quotaV2 = !registryConfigs.empty();
    
    *registry->mutable_minimum_capabilities() = capabilities.toStrings();
    ```
    
    Very clean and readable!
    
    For this to work, we have to:
    
    -Update protobuf::master::Capabilities to take in repeated strings (in addition to the
existing repeated proto constructor)
    -Carry the unknown capabilities as a member of protobuf::master::Capabilities (we can
just tackle the strings for now and leave the existing TODO on the protobuf constructor)
    -Add a toStrings() that goes back to repeated string



src/tests/registrar_tests.cpp
Lines 914-935 (original), 936-958 (patched)
<https://reviews.apache.org/r/70951/#comment303294>

    I find all these variables actually hurt readability, per offline suggestion just inlining
the strings and seeing expected json would make this a breeze to read.


- Benjamin Mahler


On June 27, 2019, 9:13 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70951/
> -----------------------------------------------------------
> 
> (Updated June 27, 2019, 9:13 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9601
>     https://issues.apache.org/jira/browse/MESOS-9601
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The new operations will mutate the `quota_configs` field in
> the registry to persist `QuotaConfigs` configured by the new
> `UPDATE_QUOTA` call as well as the legacy `SET_QUOTA` and
> `REMOVE_QUOTA` calls.
> 
> The operation removes any entries in the legacy `quotas` field
> with the same role name. In addition, it also adds/removes the
> minimum capability `QUOTA_V2` accordingly: if `quota_configs`
> is empty the capability will be removed otherwise it will
> be added.
> 
> This operation replaces the `REMOVE_QUOTA` operation.
> 
> Also fixed affected tests.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp 826362d1a9ed034a49934b1810d6e1e6cffd90cd 
>   src/master/quota.hpp 959e312b861fc0b59519a9ab5cc51061b55b534c 
>   src/master/quota.cpp a7ee845d5916e859218b0168c7b682f77549aafc 
>   src/master/quota_handler.cpp 476e87eb67e07a2ec6c7b258667cf94ff9c4f8eb 
>   src/tests/registrar_tests.cpp 81979d78f1348791e28f1afea4ca2999de6362b8 
> 
> 
> Diff: https://reviews.apache.org/r/70951/diff/4/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message