mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rukletsov" <ruklet...@gmail.com>
Subject Re: Review Request 40255: [5/7] Added framework authorization for persistent volumes.
Date Wed, 16 Dec 2015 14:38:58 GMT

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


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