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


How about a test trying to unreserve statically reserved resources? Let's 
document the expected behaviour explicitly in the test.


src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131620>

    How about we use a single resource string for clarity? Here we start a 
slave with `"cpus:1;mem:512;disk:0;ports:[]"` resources, while expect 
`"cpus:1;mem:512"` in the offer.



src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131622>

    The default for `Flags::allocation_interval` is 1s, but I hope the test is 
faster, right? Why do we get the next offer earlier than after 1s?



src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131631>

    ... and decline it.



src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131640>

    Isn't setting this param to 0 tells master that framework1 can be offered 
the same resource immediately? If you want framework2 to get the offer, this 
looks flaky.



src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131648>

    ... and reserve it for a different role.



src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131649>

    I'm not sure what is the preferred way of doing this in our codebase, but I 
would rather prefer having "role1" and "role2" defined in the beginning of the 
test with descriptive names, e.g.
    ```
    const std::string frameworkRole = "role1";
    const std::string reservationRole = "role2";
    ```



src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131654>

    Mind leaving a comment here or above why updateAllocation should not be 
called? Like you do in the next test.



src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131650>

    // In the next offer, still expect an offer with the unreserved resources.



src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131651>

    Mind leaving a comment that we have a default credential set there and it 
is different to the one we are going to use to reserve?



src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131652>

    Again, let's move it to the beginning, closer to `DEFAULT_FRAMEWORK_INFO`.



src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131653>

    Mind leaving a comment here or above why updateAllocation should not be 
called? Like you do in the next test.


- Alexander Rukletsov


On April 8, 2015, 5:05 p.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29748/
> -----------------------------------------------------------
> 
> (Updated April 8, 2015, 5:05 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
> 
> 
> Bugs: MESOS-2489
>     https://issues.apache.org/jira/browse/MESOS-2489
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 
>   src/tests/reservation_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29748/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Reply via email to