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 Mon, 01 Jul 2019 17:04:11 GMT

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


Fix it, then Ship it!





src/master/master.cpp
Lines 1740 (patched)
<https://reviews.apache.org/r/70951/#comment303413>

    CHECK_NOT_CONTAINS will already print the second argument?



src/master/quota.cpp
Line 68 (original), 66 (patched)
<https://reviews.apache.org/r/70951/#comment303414>

    should be a 4 space indent..?



src/master/quota.cpp
Lines 68 (patched)
<https://reviews.apache.org/r/70951/#comment303416>

    If it fits, it seems more consistent and readable as `registryConfig`?



src/master/quota.cpp
Line 77 (original), 95 (patched)
<https://reviews.apache.org/r/70951/#comment303415>

    Should be a 4 space indent..?



src/tests/registrar_tests.cpp
Lines 947 (patched)
<https://reviews.apache.org/r/70951/#comment303418>

    the `-> bool` shouldn't be necessary?



src/tests/registrar_tests.cpp
Line 955 (original), 971 (patched)
<https://reviews.apache.org/r/70951/#comment303420>

    Hm.. shouldn't we just be expecting that there's no v2 quota capability written down rather
than none at all? (another place the capabilities abstraction would help.. :)).



src/tests/registrar_tests.cpp
Line 994 (original), 1004 (patched)
<https://reviews.apache.org/r/70951/#comment303422>

    Would be great to see the expected vs actual result from these, one way to cleanly do
that is using an expected value from json.
    
    That also has the benefit of the reader knowing what the expected value is without having
to read all previous operations (diffs) and apply them mentally. (which is a significant cognitive
load).



src/tests/registrar_tests.cpp
Lines 1004-1005 (original), 1006-1008 (patched)
<https://reviews.apache.org/r/70951/#comment303423>

    Ditto here, these are pretty brittle since they assume quota v2 is the only capability
rather than only looking at quota v2.
    
    If any capability gets added by default, then all these will need to get updated.


- Benjamin Mahler


On June 28, 2019, 9:43 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70951/
> -----------------------------------------------------------
> 
> (Updated June 28, 2019, 9:43 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/5/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


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