mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Quinn Leng <quinn.leng....@gmail.com>
Subject Re: Review Request 60107: Added filtering to the '/tasks' endpoint.
Date Mon, 03 Jul 2017 23:58:53 GMT


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


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