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 29748: Added tests for dynamic reservation.
Date Thu, 23 Apr 2015 16:15:29 GMT

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


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