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



src/master/master.cpp (lines 3141 - 3142)
<https://reviews.apache.org/r/40255/#comment170693>

    Blank line



src/master/master.cpp (lines 3153 - 3154)
<https://reviews.apache.org/r/40255/#comment170694>

    Blank line



src/master/master.cpp (line 3342)
<https://reviews.apache.org/r/40255/#comment170696>

    I see that you follow the pattern here, but why does the master commit 
suicide if the future fails? My understanding is that this does not violate any 
internal invariant and should lead to a retry. Am I missing something? Do we 
need a comment explaining the reason?



src/master/master.cpp (line 3348)
<https://reviews.apache.org/r/40255/#comment170697>

    You are following the pattern here, but are we sure that the framework has 
the principal? I also do not see any tests with frameworks without principals 
(nor in "reservation_tests.cpp"). It looks like an unsuccessful authorization 
for a framework without a principal can kill the master.



src/master/master.cpp (lines 3348 - 3349)
<https://reviews.apache.org/r/40255/#comment170698>

    I believe we need an extra blank line in this case. Also in other places 
above and below : )



src/tests/persistent_volume_tests.cpp (lines 717 - 719)
<https://reviews.apache.org/r/40255/#comment170704>

    Could you please add a test with a framework without a principal?
    
    In the same vein, do you think it makes sense to create a ticket for the 
same case for dynamic reservatons (even though we require the principal for 
now)?



src/tests/persistent_volume_tests.cpp (line 723)
<https://reviews.apache.org/r/40255/#comment170699>

    You don't need the `process::` prefix. Here and everywhere.



src/tests/persistent_volume_tests.cpp (line 759)
<https://reviews.apache.org/r/40255/#comment170700>

    Mind adding a comment why you statically reserve disk resources?



src/tests/persistent_volume_tests.cpp (line 790)
<https://reviews.apache.org/r/40255/#comment170701>

    Do you think it makes sense to extract "role1" into a constant?



src/tests/persistent_volume_tests.cpp (lines 821 - 822)
<https://reviews.apache.org/r/40255/#comment170702>

    Let's either insert a blank line or refactor the comment so that it refers 
to both `EXPECT_*`s. Same below.



src/tests/persistent_volume_tests.cpp (lines 869 - 871)
<https://reviews.apache.org/r/40255/#comment170703>

    Only for a framework with a principal. Can we add one more test with a 
framework *without* a principal? I believe now it will lead to a master crash : 
)



src/tests/persistent_volume_tests.cpp (lines 968 - 970)
<https://reviews.apache.org/r/40255/#comment170705>

    Do you think it makes sense to merge this test with the first one by 
creating a second framework with a different principal and allowing only that 
framework to destroy the volume?


- Alexander Rukletsov


On Dec. 8, 2015, 7:03 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40255/
> -----------------------------------------------------------
> 
> (Updated Dec. 8, 2015, 7:03 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-3065
>     https://issues.apache.org/jira/browse/MESOS-3065
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added framework authorization for persistent volumes.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
>   src/tests/persistent_volume_tests.cpp 
> 01b3c13751a5558d5f06edb8f650c8644dc54486 
> 
> Diff: https://reviews.apache.org/r/40255/diff/
> 
> 
> Testing
> -------
> 
> This is the fifth in a chain of 7 patches. New tests were added to 
> `persistent_volume_tests.cpp` in order to test a framework attempting both 
> successful and failed authorizations for `CREATE` and `DESTROY` offer 
> operations. `make check` was used to test after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Reply via email to