----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71859/#review218914 -----------------------------------------------------------
I'm really excited to see the synchronous authorization improvement! Thanks for tackling this Overall looks good, just a couple of copy related issues, and some style related notes. src/common/authorization.cpp Lines 58 (patched) <https://reviews.apache.org/r/71859/#comment306816> Don't forget those braces on the next line! :) src/common/authorization.cpp Lines 59 (patched) <https://reviews.apache.org/r/71859/#comment306817> can just be one line? ``` return stream << jsonify(JSON::Protobuf(object)); ``` src/master/authorization.hpp Lines 32 (patched) <https://reviews.apache.org/r/71859/#comment306819> Ditto here, brace on next line src/master/authorization.hpp Lines 36 (patched) <https://reviews.apache.org/r/71859/#comment306818> s/:/: / Also, the following looks better? ``` ActionObject(Action action_) : action(std::move(action_)) {}; ``` But I assume action is an enum? In that case, maybe we don't take it by ref or move it either? src/master/authorization.cpp Lines 41-42 (patched) <https://reviews.apache.org/r/71859/#comment306820> We try in new code to stick to the following assignment style, since it works automatically with moves and is also easier to read: ``` *actionObject.object->mutable_task_info() = task; *actionObject.object->mutable_framework_info() = framework; ``` src/master/authorization.cpp Lines 44 (patched) <https://reviews.apache.org/r/71859/#comment306821> Can you avoid the copy of the action object here? I'm guessing in the subsequent patches it will be more obvious why we return a vector here (e.g. other action object creators require return multiple values and we want a uniform interface?) src/master/master.cpp Line 3733 (original), 3740 (patched) <https://reviews.apache.org/r/71859/#comment306825> Ditto here: ``` *request.mutable_subject() = *subject; ``` src/master/master.cpp Lines 3736-3741 (original), 3743-3751 (patched) <https://reviews.apache.org/r/71859/#comment306826> Hm.. can you reply to this comment with an example of the logging before and after? I'm curious if it's looking worse or we have less information (e.g. task id). src/master/master.cpp Lines 3755 (patched) <https://reviews.apache.org/r/71859/#comment306827> thanks! src/master/master.cpp Lines 3759 (patched) <https://reviews.apache.org/r/71859/#comment306828> why the "if one is available"? That seems to imply there still being an async fallback? src/master/master.cpp Lines 3743-3748 (original), 3763-3767 (patched) <https://reviews.apache.org/r/71859/#comment306829> Why is this optimization up here rather than in collectAuthorizations? src/master/master.cpp Lines 4365-4377 (patched) <https://reviews.apache.org/r/71859/#comment306822> This patch has introduced an extra copy of all the tasks! Notice that before the lambda just returned a reference to the right field, now since it's not returning a reference we're copying it. Can you leave it as it was as a lambda without the copying? I guess since we need to call it twice now we move the declaration up: ``` const RepeatedPtrField<TaskInfo>& tasks = [&]() { if (operation.type() == Offer::Operation::LAUNCH) { return operation.launch().task_infos(); } else if (operation.type() == Offer::Operation::LAUNCH_GROUP) { return operation.launch_group().task_group().tasks(); } UNREACHABLE(); }(); // Use `tasks` twice below.. ``` src/master/master.cpp Lines 4825-4827 (original), 4843-4846 (patched) <https://reviews.apache.org/r/71859/#comment306823> Why the change here? Can you leave the old wrapping style as is? src/master/master.cpp Lines 4851-4853 (patched) <https://reviews.apache.org/r/71859/#comment306824> We tend to avoid wrapping way over to the right, so the following would be acceptable: ``` const Option<Principal> principal = framework->info.has_principal() ? Principal(framework->info.principal()) : Option<Principal>::none(); ``` ``` const Option<Principal> principal = framework->info.has_principal() ? Principal(framework->info.principal()) : Option<Principal>::none(); ``` ``` const Option<Principal> principal = framework->info.has_principal() ? Principal(framework->info.principal()) : Option<Principal>::none(); ``` - Benjamin Mahler On Dec. 3, 2019, 3:02 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71859/ > ----------------------------------------------------------- > > (Updated Dec. 3, 2019, 3:02 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 > ------- > > This is the first patch in the chain that extract Master code > generating Action-Object pairs into a dedicated ActionObject class, > thus seperating authz Object creation from feeding them into authorizer. > > This is a prerequisite to using ObjectApprover interface for > synchronous authorization of Scheduler API calls. > > > Diffs > ----- > > src/CMakeLists.txt ef9382dc77d37fed344b7267119f3251acd3088a > src/Makefile.am 111c156c8a7abc5ece04779e8ac8879a30c22dbf > src/common/authorization.hpp 565d5ca6620442803fa80be652ab7382102347f5 > src/common/authorization.cpp fa71b0e8e8b9541376a9fd199f4d7b9db56a3f0f > src/master/authorization.hpp PRE-CREATION > src/master/authorization.cpp PRE-CREATION > src/master/master.hpp 93630421d58e6fd26566e81a23cd910957795665 > src/master/master.cpp 14b90a5e276df055bb8a570331f27cab200c9869 > > > Diff: https://reviews.apache.org/r/71859/diff/2/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
