You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rojas <al...@mesosphere.io> on 2017/07/03 09:51:24 UTC
Re: Review Request 60107: Added filtering to the '/tasks' endpoint.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60107/#review179488
-----------------------------------------------------------
I think the acceptor files do not belong in the `common/http.?pp` files. Perhaps create a new header called acceptors?
src/common/http.hpp
Lines 163-171 (patched)
<https://reviews.apache.org/r/60107/#comment254231>
This class is not necessary. The acceptor as a concept exist, but we don't use any of the advantages of inheritance, I think this would be served better in a namespace.
src/common/http.hpp
Lines 175-229 (patched)
<https://reviews.apache.org/r/60107/#comment254235>
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.
src/common/http.hpp
Lines 181 (patched)
<https://reviews.apache.org/r/60107/#comment254232>
This should be a const reference, we don't want to copy `Owned` pointers.
- Alexander Rojas
On July 1, 2017, 1:57 a.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60107/
> -----------------------------------------------------------
>
> (Updated July 1, 2017, 1:57 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
> -------
>
> 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
>
>
Re: Review Request 60107: Added filtering to the '/tasks' endpoint.
Posted by Greg Mann <gr...@mesosphere.io>.
> 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
>
>
Re: Review Request 60107: Added filtering to the '/tasks' endpoint.
Posted by Quinn Leng <qu...@gmail.com>.
> On July 3, 2017, 9:51 a.m., Alexander Rojas wrote:
> > I think the acceptor files do not belong in the `common/http.?pp` files. Perhaps create a new header called acceptors?
Agree, since these Acceptors contain logic about not only HTTP request but also Authorization. It's better to move these logic into one specific file. Do you think it's good to call it common/acceptor.?pp
> On July 3, 2017, 9:51 a.m., Alexander Rojas wrote:
> > src/common/http.hpp
> > Lines 163-171 (patched)
> > <https://reviews.apache.org/r/60107/diff/15/?file=1767977#file1767977line163>
> >
> > This class is not necessary. The acceptor as a concept exist, but we don't use any of the advantages of inheritance, I think this would be served better in a namespace.
It's kind of inbetween logically consistent and programacally consistent. We haven't used a lot of inheritance feature (overriding) of the concept, but there do exist some logical inheritance connection between them. This ObjectAcceptor connects these AuthorizeAcceptor and IDAcceptor.
> 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.
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.
> 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.
Agree, I'll update it in another patch. Thanks for pointing out~
- Quinn
-----------------------------------------------------------
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
>
>