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



src/master/master.cpp (lines 3016 - 3028)
<https://reviews.apache.org/r/39989/#comment166313>

    Let's move the validation to aurhtorizeReserveResources.
    
    You also need to add a NOTE in `_accept` saying that no need to validate 
the operation again since it's already validated during authorization.



src/master/master.cpp (lines 3053 - 3054)
<https://reviews.apache.org/r/39989/#comment166312>

    I don't think the comments here is necessary. We need to avoid 
over-commenting. If the code itself is quite clear about what it's about to do, 
no need for the extra comments here.



src/master/master.cpp (line 3160)
<https://reviews.apache.org/r/39989/#comment166314>

    No need for this comment.



src/master/master.cpp (line 3195)
<https://reviews.apache.org/r/39989/#comment166311>

    I don't think this comment add more value. The code itself is quite clear 
what it is about to do.



src/master/master.cpp (line 3202)
<https://reviews.apache.org/r/39989/#comment166315>

    Ditto.



src/master/master.cpp (line 3206)
<https://reviews.apache.org/r/39989/#comment166316>

    Remove this comment.



src/master/master.cpp (line 3237)
<https://reviews.apache.org/r/39989/#comment166317>

    Ditto.



src/master/master.cpp (line 3310)
<https://reviews.apache.org/r/39989/#comment166318>

    Ditto.



src/tests/reservation_tests.cpp (lines 1657 - 1668)
<https://reviews.apache.org/r/39989/#comment166320>

    This might be flaky because master might send out an offer before it 
processes the terminal status update.


- Jie Yu


On Nov. 20, 2015, 12:20 a.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> -----------------------------------------------------------
> 
> (Updated Nov. 20, 2015, 12:20 a.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 framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
>   src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> -------
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Reply via email to