> On March 9, 2018, 5:30 a.m., Greg Mann wrote:
> > src/slave/http.cpp
> > Lines 2641-2665 (original), 2431-2469 (patched)
> > <https://reviews.apache.org/r/65313/diff/6/?file=1972598#file1972598line2668>
> >
> >     Why did you opt for a new templated method instead of the pre-existing 
> > lambda, here and elsewhere? Is it necessary because we pass the action as a 
> > template parameter?
> 
> Alexander Rojas wrote:
>     It has to do more with the `Approvers::approved<T>()` being parametrize, 
> which means that the type needs to be resolved on compilation time, so this 
> is a neat trick for that.
> 
> Greg Mann wrote:
>     IMO it's unfortunate that we need to do this, since it makes this code 
> harder to read.
>     
>     Actually, looking again at `ObjectApprovers`, I wonder if the action 
> really needs to be a template parameter. We could also do something like:
>     ```
>     class ObjectApprovers {
>       template <typename... Args>
>       bool approved(const authorization::Action& action, const Args&... args);
>     
>     ...
>     ```
>     
>     Then a callsite would look like
>     ```
>     if (!approvers->approved(SOME_ACTION, someInfo)) {
>     ```
>     which seems OK to me. WDYT?

As we discussed in chat, we can get rid of some of these extra continuations if 
we duplicate some code. Let's get this stuff merged now to avoid more rebasing, 
then we can clean up later.


- Greg


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65313/#review198917
-----------------------------------------------------------


On March 9, 2018, 1:06 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65313/
> -----------------------------------------------------------
> 
> (Updated March 9, 2018, 1:06 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Greg Mann.
> 
> 
> Bugs: MESOS-8434
>     https://issues.apache.org/jira/browse/MESOS-8434
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch makes uses of the new `ObjectApprovers` class which greatly
> simplifies the logic for constructing and using authorization.
> 
> 
> Diffs
> -----
> 
>   src/slave/http.hpp c33adeb2ddd36e8be1324b3ddb1401bdf7e4e80b 
>   src/slave/http.cpp 7d7fa2b4ec2e1f8f65c5264ce72590d0d8195b9b 
>   src/slave/slave.cpp 8cb6899bf15fb697c3cb2784f63b7c2d5729d219 
> 
> 
> Diff: https://reviews.apache.org/r/65313/diff/7/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Reply via email to