----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70549/#review215654 -----------------------------------------------------------
Fix it, then Ship it! include/mesos/authorizer/authorizer.proto Line 53 (original), 53 (patched) <https://reviews.apache.org/r/70549/#comment302436> [deprecated = true]? include/mesos/authorizer/authorizer.proto Lines 59 (patched) <https://reviews.apache.org/r/70549/#comment302435> See comment below about just using the role. include/mesos/authorizer/authorizer.proto Line 139 (original), 147 (patched) <https://reviews.apache.org/r/70549/#comment302433> [deprecated = true]? I believe we can deprecate enum values? https://github.com/protocolbuffers/protobuf/issues/5567 include/mesos/authorizer/authorizer.proto Lines 149-150 (patched) <https://reviews.apache.org/r/70549/#comment302434> Per alexr's comment, I also think we should just keep this simple and use the role only. That means deprecating the `QuotaInfo` field as well, right? src/master/master.hpp Lines 1255-1257 (original), 1255-1257 (patched) <https://reviews.apache.org/r/70549/#comment302437> Add a comment noting that this is the deprecated path? src/master/quota_handler.cpp Lines 832-833 (patched) <https://reviews.apache.org/r/70549/#comment302438> Open / close quotes for role on the same line for readability? ``` << "' to update quota config" << " for role '" << quotaConfig.role() << "'"; ``` src/master/quota_handler.cpp Lines 840 (patched) <https://reviews.apache.org/r/70549/#comment302439> Probably we should never use CopyFrom and stick with the following pattern: ``` *request.mutable_subject() = *subject; ``` That enables moving when the rhs is an rvalue, but in this case you need to explicitly move: ``` *request.mutable_subject() = std::move(*subject); ``` src/master/quota_handler.cpp Lines 843 (patched) <https://reviews.apache.org/r/70549/#comment302440> Ditto here about avoiding copyFrom - Benjamin Mahler On May 22, 2019, 12:48 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70549/ > ----------------------------------------------------------- > > (Updated May 22, 2019, 12:48 p.m.) > > > Review request for mesos, Alexander Rukletsov, Andrei Sekretenko, and > Benjamin Mahler. > > > Bugs: MESOS-9640 > https://issues.apache.org/jira/browse/MESOS-9640 > > > Repository: mesos > > > Description > ------- > > A new authorizable action `UPDATE_QUOTA_WITH_CONFIG` is added. > This disambiguates with the old action `UPDATE_QUOTA` which > are used for the old `SetQuota` and `RemoveQuota` calls. > `UPDATE_QUOTA` action requires `QuotaInfo` as the object while > the new `UpdatedQuota` call uses `QuotaConfig`. To keep it compatible > with any external authorization modules, a new action is introduced. > > > Diffs > ----- > > include/mesos/authorizer/authorizer.proto > e2740c402732bb37db991ec92b9301e58b33215b > src/master/master.hpp 5ad128d00d3cdf0dca89eb637ae196987bdce412 > src/master/quota_handler.cpp a18d8bafda5604d1844f7f7ed31d4ea80fbf6d04 > src/tests/master_authorization_tests.cpp > ee69910a34416728bf14ed23f4a6faae6c1204a0 > > > Diff: https://reviews.apache.org/r/70549/diff/3/ > > > Testing > ------- > > make check > > > Thanks, > > Meng Zhu > >
