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 32398: Persisted the reservation state on the slave.
Date Fri, 24 Apr 2015 08:18:29 GMT

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


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