----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60716/#review180197 -----------------------------------------------------------
src/common/http.hpp Line 175 (original), 173 (patched) <https://reviews.apache.org/r/60716/#comment255254> Why rvalue references here? src/common/http.hpp Lines 179-180 (patched) <https://reviews.apache.org/r/60716/#comment255238> Fits on one line. src/common/http.hpp Line 178 (original), 188 (patched) <https://reviews.apache.org/r/60716/#comment255255> If we take the approver by rvalue reference here - i.e., `Owned<ObjectApprover>&&` - then we can avoid copies: ``` AuthorizationAcceptor(const process::Owned<ObjectApprover>&& approver) : objectApprover(std::forward(approver)) {} ``` src/common/http.cpp Line 1144 (original), 1121 (patched) <https://reviews.apache.org/r/60716/#comment255239> In this context, the `AcceptingObjectApprover` seems like an unnecessary level of indirection to me. What do you think about making the `objectApprover` member an `Option<Owned<ObjectApprover>>`, and then `accept()` could return true if `objectApprover.isNone()`? We could take care of this with a follow-up patch. src/common/http.cpp Line 1155 (original), 1130 (patched) <https://reviews.apache.org/r/60716/#comment255257> If we change the acceptor constructor to take the approver by rvalue reference, then this will have to change to: ``` new AuthorizationAcceptor(std::move(approver))); ``` - Greg Mann On July 7, 2017, 10:25 p.m., Quinn Leng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60716/ > ----------------------------------------------------------- > > (Updated July 7, 2017, 10:25 p.m.) > > > Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and > Vinod Kone. > > > Bugs: MESOS-7630 > https://issues.apache.org/jira/browse/MESOS-7630 > > > Repository: mesos > > > Description > ------- > > Cleaned up authentication acceptor to have one class. > > Replace different Authorization related Acceptor classes with one > AuthorizationAcceptor class. > > Single static create function for AuthorizationAcceptor. > Templated 'accept' function to take different number and types of > parameters. > > Removed ObjectAcceptor parent class, since no inheritance feature > provided by it. > > > Diffs > ----- > > src/common/http.hpp b7e4a8adcbcaa3a962af795c67694a35161b6c1a > src/common/http.cpp fdb591eccf273260902f3f695cf431f72ee3d817 > src/master/http.cpp 175a44ce7fb5be509453c25eaa9ec29f35adba3a > > > Diff: https://reviews.apache.org/r/60716/diff/2/ > > > Testing > ------- > > Passed "make check -j48" > Passed 'GTEST_FILTER="MasterTest.TasksEndpoint" make check -j48' > > > Thanks, > > Quinn Leng > >
