----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70951/#review216159 -----------------------------------------------------------
src/master/master.cpp Lines 1740-1741 (patched) <https://reviews.apache.org/r/70951/#comment303203> Why not just do a .contains here? Or CHECK_CONTAINS? src/master/quota.hpp Line 64 (original), 64 (patched) <https://reviews.apache.org/r/70951/#comment303204> What is this comment saying? This constructor is for the UPDATE_QUOTA master call path? Why say that? There's also no other constructor, and this comment seems to suggest that there are others for other `XXX_QUOTA` things? src/master/quota.cpp Lines 57-63 (original), 58-67 (patched) <https://reviews.apache.org/r/70951/#comment303205> Storing a mutable reference to the configs seems to help readability here? Also, Maybe find_if is clearer? And the if condition confused me a bit, maybe it's clearer to do it by default quota first, then whether it's found? ``` ...& configs; int configIndex = find_if( configs.begin(), configs.end(), [&](const QuotaConfig& config) { return role == config.role(); }) - configs.begin(); if (Quota(config) == DEFAULT_QUOTA)) { // Erase if present, otherwise no-op. if (configIndex < configs.size()) { configs.DeleteSubrange(index, 1); } } else { // Modify if present, otherwise insert. if (configIndex < configs.size()) { *configs.Mutable(index) = config; } else { *configs.Add() = config; } } ``` Ditto for the infos logic below. src/master/quota.cpp Line 75 (original), 102 (patched) <https://reviews.apache.org/r/70951/#comment303206> s/std// src/master/quota.cpp Lines 83-84 (original), 105-113 (patched) <https://reviews.apache.org/r/70951/#comment303207> Ditto find_if? src/master/quota.cpp Lines 86-89 (original), 115-127 (patched) <https://reviews.apache.org/r/70951/#comment303208> Ditto here, feels more readable if thinking about the inner cases first: // if capability needs to be present // make it present // else: capability needs to be absent // make it absent src/master/quota.cpp Line 91 (original), 129 (patched) <https://reviews.apache.org/r/70951/#comment303209> This is supposed to return true when there's been a mutation, but it's always returning true? The case where it's a no-op should return false. src/tests/registrar_tests.cpp Lines 927-948 (patched) <https://reviews.apache.org/r/70951/#comment303211> Pull this towards the top of the test body, before the twoResourceGuarantees stuff is declared? src/tests/registrar_tests.cpp Lines 944-946 (original), 965-967 (patched) <https://reviews.apache.org/r/70951/#comment303210> Shouldn't we be sanity checking the registry protobuf at this point? This can be done by hitting the registrar's endpoint. Ditto in the other cases. src/tests/registrar_tests.cpp Lines 954-960 (original), 975-986 (patched) <https://reviews.apache.org/r/70951/#comment303213> I'm finding this test rather challenging to read, can we make it more readable than all the micro-checks by showing clearly what the expected registry looks like in json? E.g. ``` Registry expected = fromJSON(R"~( { "configs": [ ... ] "capabilities": [ ... ] } )~"); EXPECT_EQ(expected, actual); ``` That will make it a lot less of a struggle to read - Benjamin Mahler On June 26, 2019, 6:03 a.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70951/ > ----------------------------------------------------------- > > (Updated June 26, 2019, 6:03 a.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/2/ > > > Testing > ------- > > make check > > > Thanks, > > Meng Zhu > >
