> On July 3, 2017, 9:51 a.m., Alexander Rojas wrote: > > src/common/http.hpp > > Lines 175-229 (patched) > > <https://reviews.apache.org/r/60107/diff/15/?file=1767977#file1767977line175> > > > > What is the point of having multiple different > > `AuthorizationAcceptors`? Each have a method with a different signature, so > > I can see them merge into one common acceptor. > > Quinn Leng wrote: > One possible issue with single AuthorizationAcceptor is that we > initialize an approver for each AuthorizationAcceptor, and these approvers > are initialized with different parameters (specifically, the > authorization::VIEW_TASK, authorization::VIEW_FRAMEWORK .etc..), thus it's > not good idea to have one AuthorizationAcceptor providing authorization for > different kind of objects. Alternatively, we can use multiple > AuthorizationAcceptor objects, each is an instance of AuthorizationAcceptor > initialized with different constructor, but then it will be necessary that we > call the correct kind of 'accept' for that AuthorizationAcceptor. That might > be confusing.
@arojas, the current implementation encodes the authorization action in the class type, so that the callsite doesn't need to specify it explicitly. Do you think this is a reasonable approach? If we go with a single authorization acceptor, the action will need to be specified by the callsite. > On July 3, 2017, 9:51 a.m., Alexander Rojas wrote: > > src/common/http.hpp > > Lines 181 (patched) > > <https://reviews.apache.org/r/60107/diff/15/?file=1767977#file1767977line181> > > > > This should be a const reference, we don't want to copy `Owned` > > pointers. > > Quinn Leng wrote: > Agree, I'll update it in another patch. Thanks for pointing out~ This is the member variable - I'm pretty sure we shouldn't be holding the object as a const ref here. If we want to avoid copies when passing the approver from `create()` to the `AuthorizationAcceptor` constructor, we could pass it as an rvalue reference. - Greg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60107/#review179488 ----------------------------------------------------------- On June 30, 2017, 11:57 p.m., Quinn Leng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60107/ > ----------------------------------------------------------- > > (Updated June 30, 2017, 11:57 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 > ------- > > Added filtering to the '/tasks' endpoint. > > > Diffs > ----- > > src/common/http.hpp 93d6088e97c2384f9f6d26e010a501abf2deb43e > src/common/http.cpp 2f7718cbc2e449b4f7c89754e8f84eac2f3c35b6 > src/master/http.cpp 4dd43fd7c3fb986f4eed78bce574b6d3af156b67 > src/tests/master_tests.cpp 6cd4be7e5ba48c6562ce91b28e76b065522cfbea > > > Diff: https://reviews.apache.org/r/60107/diff/15/ > > > Testing > ------- > > Passed "make check" > Passed "GTEST_FILTER="MasterTest.TasksEndpoint" make check -j48" > Passed "GLOG_v=1 ./bin/mesos-tests.sh > --gtest_filter="MasterTest.TasksEndpoint" --gtest_repeat=1000 > --gtest_break_on_failure" > > > Thanks, > > Quinn Leng > >
