----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71861/#review218920 -----------------------------------------------------------
src/master/authorization.hpp Lines 38-46 (patched) <https://reviews.apache.org/r/71861/#comment306837> This constructor seems really unfortunate to me, it's pretty hard to know what it's for since it has no name (unlike a static function), and it seems like we don't want callers touching this, so we'd probably want whatever this is to be private? Seems like a private static helper would be best? That way we can keep a single clean (private) constructor: ``` private: ActionObject(Action a, Object&& o) : action(a), object(std::move(o)) {}; static ActionObject fromResourceWithLegacyValue(...); ``` src/master/authorization.hpp Lines 44 (patched) <https://reviews.apache.org/r/71861/#comment306836> Note the inconsistency between Action (here) and Action& (above), I commented on this argument in the initial patch so be sure to keep it consistent when you update this one. For enums (much like int or double), I think the style guide would prefer to just take it as non-const `Action action`. src/master/authorization.cpp Lines 42 (patched) <https://reviews.apache.org/r/71861/#comment306838> Ditto from previous reviews w.r.t. assignment style src/master/authorization.cpp Lines 98-100 (patched) <https://reviews.apache.org/r/71861/#comment306835> everything after "since" is only really relevant to RESERVE or UNRESERVE (pushing or popping) I think this function should probably be renamed to `getLegacyValue(...)` and the explanation of what the legacy `value` field is set to for a resource, and why. - Benjamin Mahler On Dec. 3, 2019, 5:53 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71861/ > ----------------------------------------------------------- > > (Updated Dec. 3, 2019, 5:53 p.m.) > > > Review request for mesos, Benjamin Bannier and Benjamin Mahler. > > > Bugs: MESOS-10023 and MESOS-10056 > https://issues.apache.org/jira/browse/MESOS-10023 > https://issues.apache.org/jira/browse/MESOS-10056 > > > Repository: mesos > > > Description > ------- > > Moved creating authz Object out of `Master::authorizeResizeVolume`. > > > Diffs > ----- > > src/master/authorization.hpp PRE-CREATION > src/master/authorization.cpp PRE-CREATION > src/master/http.cpp 6d8485656982a4d556b042a6a3d95cfed21370d3 > src/master/master.hpp 93630421d58e6fd26566e81a23cd910957795665 > src/master/master.cpp 14b90a5e276df055bb8a570331f27cab200c9869 > > > Diff: https://reviews.apache.org/r/71861/diff/2/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
