> On Dec. 1, 2015, 2:24 p.m., Michael Park wrote:
> > src/master/master.cpp, line 2771
> > <https://reviews.apache.org/r/39987/diff/9/?file=1149276#file1149276line2771>
> >
> >     Why is it that we need to perform validation within authorization? We 
> > perform `authorizeTask` with a `TaskInfo` that has not been validated yet, 
> > and take care of it later on. Does that pattern not work here?
> 
> Greg Mann wrote:
>     That pattern would work here, and the original patches I posted worked 
> that way. We ended up switching to this solution because it makes for a 
> cleaner implementation and eliminates some redundant code. However, it does 
> come at the cost of making error messaging more difficult (as you noted in 
> another patch) and decreasing the separation of functionality that can make 
> debugging easier. I don't have a strong preference for either path at the 
> moment. Perhaps Jie could chime in with his thoughts?
> 
> Jie Yu wrote:
>     I looked at the code again. I guess the reason we validate the operation 
> before authorization is because we want to get all the principals from 
> resources in UNRESERVE, and we want to make sure resources in UNRESERVE 
> operation is valid. As MPark pointed out, coupling authorization with 
> validation does cause some confusion when generating error messages. How 
> about the following:
>     
>     1) we don't perform validation in authorization. In 
> authorizeUnreserveResources, when we iterate all resources, we can do the 
> following as you previosly did (sorry about the back and forth, my bad).
>     ```
>     foreach (const Resource& resource, unreserve.resources()) {
>       if (Resources::isDynamicallyReserved(resource)) {
>         request.mutable_reserver_principals()->add_values(
>             resource.reservation().principal());
>       }
>     }
>     ```
>     
>     2) in Http::reserve and Http::unreserve, we still perform validation 
> first before calling master->authroizeXXX.
>     
>     3) in `Master::_accept`, we perform operation validation on 
> RESERVE/UNRESERVE.
>     
>     Let me know if that's better or not?
> 
> Greg Mann wrote:
>     No worries, Jie! I learned about `Future<>::repair()` in the process so I 
> consider it worthwhile :-)
>     
>     I have one question about your plan: for `LAUNCH` operations, tasks are 
> authorized before they get validated, and I'm wondering if it's worth making 
> this part of the interface consistent across all types of operations, i.e. 
> perhaps we should change the code for `LAUNCH` to validate before 
> authorization as well. If we decide to do this, I could do it here or in 
> another review.
> 
> Greg Mann wrote:
>     Since the validation for `LAUNCH` needs to be aware of tasks that were 
> previously launched on the same operation, it makes sense in that case for 
> validation to occur after authorization. However, in general, I think it 
> makes more sense for the operation to be validated before authorization, so 
> I'll follow the pattern that Jie suggested above for 
> `RESERVE/UNRESERVE/CREATE/DESTROY`. This means that the interface will have 
> slightly different expectations regarding validation depending on what type 
> of operation is being performed.
>     
>     An alternative would be to validate all operations *after* authorization 
> is performed, which in general seems less intuitive to me, but would keep the 
> interface consistent...

Greg, the code sample I suggested above actually tried to keep the interface 
consistent (validation is done separately and is decoupled from authorization). 
Just want to double check with you.


- Jie


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


On Nov. 30, 2015, 8:35 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> -----------------------------------------------------------
> 
> (Updated Nov. 30, 2015, 8:35 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
>     https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
> 
> Diff: https://reviews.apache.org/r/39987/diff/
> 
> 
> Testing
> -------
> 
> This is the third in a chain of 5 patches. `make check` was used to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Reply via email to