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