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



src/common/resources.cpp
<https://reviews.apache.org/r/32398/#comment131836>

    Let's use this function where we already check for this condition, like in 
`master/validation.cpp` in 
    ```
    Option<Error> validateDynamicReservation(
        const RepeatedPtrField<Resource>& resources)
    ```



src/common/resources.cpp
<https://reviews.apache.org/r/32398/#comment131837>

    To confirm we are on the same page here: we don't want to check for 
`resource.role() != "*"` here, because it's part of the validation and if one 
day we re-think and allow `resource.role() == "*" && 
resource.has_reservation()`, we adjust this function, correct?



src/common/resources_utils.cpp
<https://reviews.apache.org/r/32398/#comment131841>

    I like the name a lot, gives a good understanding on what's going to 
happen, but let's still leave a comment on what we going to achieve with 
stripping and why we need it.



src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/32398/#comment131842>

    Why don't you use the "standard" amount of resources `"cpus:1;mem:512"` we 
usually do in tests? Here and below.



src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/32398/#comment131840>

    What is the purpose of using the reverse order here?



src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/32398/#comment131839>

    What operation does this message correspond to? Could you please leave a 
short comment to all of them?



src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/32398/#comment131843>

    Even though `"reconnect"` is the default for `--recover`, let's be explicit 
and document our intention.



src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/32398/#comment131845>

    Again, `shutdown` param is `false` by default, but let's be explicit about 
our intention here.



src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/32398/#comment131844>

    IIUC we expect slave recovery to happen here. Let's document it, how about
    ```
    Future<Nothing> slaveRecovers = FUTURE_DISPATCH(_, &Slave::recover);
    ```



src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/32398/#comment131846>

    Let's name this guy `master2` for clarity.



src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/32398/#comment131848>

    Let's expand a comment a bit regarding compatibility. It's a bit hard to 
grasp what's happening: first slave instance declared `"cpus:8;mem:4096"`, 
`"cpus:8;mem:2048"` got reserved and checkpointed, second instance declares 
`"cpus:12;mem:2048"` and it's "compatible". Mention, that slave's declared 
resources should include checkpointed should suffice. Thanks!



src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/32398/#comment131847>

    Here you can reference to the previous test where you have already 
described what compatibility means.


- Alexander Rukletsov


On April 15, 2015, 3:55 p.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32398/
> -----------------------------------------------------------
> 
> (Updated April 15, 2015, 3:55 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-2491
>     https://issues.apache.org/jira/browse/MESOS-2491
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> * Added `isDynamicReservation(resource)` which returns true if the resource 
> is either a dynamic role reservation or a framework reservation.
> * Added the `isDynamicReservation` condition onto `needCheckpointing`.
> * Updated the `applyCheckpointedResources` to consider dynamic reservations.
> * Added tests.
> 
> 
> Diffs
> -----
> 
>   include/mesos/resources.hpp 56affd45e1e71e96ff5778b43271f81b9b9a9e4d 
>   src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059 
>   src/common/resources_utils.cpp fe04d57227fa193d6d11d2f76529c46aea74c6a1 
>   src/tests/reservation_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/32398/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Reply via email to