mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 60107: Added filtering to the '/tasks' endpoint.
Date Wed, 05 Jul 2017 18:55:54 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message