> On July 11, 2017, 5:23 p.m., Greg Mann wrote: > > src/common/http.hpp > > Line 178 (original), 188 (patched) > > <https://reviews.apache.org/r/60716/diff/2/?file=1771727#file1771727line191> > > > > 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)) {} > > > > ``` > > Greg Mann wrote: > Could you drop this instead of resolving, since we decided to drop it?
Oh, since it's written in TODO, I guess it's not 'dropped' in definition. Anyway, I can drop it now. > On July 11, 2017, 5:23 p.m., Greg Mann wrote: > > src/common/http.cpp > > Line 1144 (original), 1121 (patched) > > <https://reviews.apache.org/r/60716/diff/2/?file=1771728#file1771728line1144> > > > > 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. > > Greg Mann wrote: > Were you going to take care of this in a follow-up patch? If so, please > reply accordingly and drop the issue. That's a good update, I can do that in follow-up patch. - Quinn ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60716/#review180197 ----------------------------------------------------------- On July 12, 2017, 1:17 a.m., Quinn Leng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60716/ > ----------------------------------------------------------- > > (Updated July 12, 2017, 1:17 a.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 93c9b2e58600189867b85175fe4de2dc2f6bf33e > src/common/http.cpp 7dce4cdc82a64702431bbc4307757ec797cf4309 > src/master/http.cpp 948aa118101b6ce03410c9e0c945b6ca16668ca2 > > > Diff: https://reviews.apache.org/r/60716/diff/3/ > > > Testing > ------- > > Passed "make check -j48" > Passed 'GTEST_FILTER="MasterTest.TasksEndpoint" make check -j48' > > > Thanks, > > Quinn Leng > >
