Re: Review Request 32398: Persist the reservation state on the slave.

[Jie Yu]

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

Ship it!


Nice tests!


src/common/resources.cpp


Since we already have `isReserved`. How about calling it 
`isDynamicallyReserved`?



src/common/resources_utils.cpp


So bad that we are effectively duplicating the logics in Resources::apply 
here. But I couldn't come up with a better way without introducing stripping.

Maybe add a command about what we are doing here. We are effectively 
infering offer operations from checkpointed resources and apply them to the 
totalResources.



src/tests/reservation_tests.cpp


I think for initialization list, I considently remove leading and tailing 
spaces around { and }:

```
{offer.id()},
{RESERVE(...), ..., UNRESERVE(..)}
```



src/tests/reservation_tests.cpp


Do you want to say something about persitent volumes in the comment?:)



src/tests/reservation_tests.cpp


compatible or not compatible?


- Jie Yu


On April 8, 2015, 6:38 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32398/
> ---
> 
> (Updated April 8, 2015, 6:38 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
> 
>

Re: Review Request 32398: Persist the reservation state on the slave.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [32139, 32140, 32149, 32150, 29748, 32333]

Failed command: ./support/apply-review.sh -n -r 32333

Error:
 2015-04-08 19:54:50 URL:https://reviews.apache.org/r/32333/diff/raw/ 
[16704/16704] -> "32333.patch" [1]
error: patch failed: src/tests/reservation_tests.cpp:286
error: src/tests/reservation_tests.cpp: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On April 8, 2015, 6:38 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32398/
> ---
> 
> (Updated April 8, 2015, 6:38 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
> 
>

Re: Review Request 32398: Persist the reservation state on the slave.

[Michael Park]

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

(Updated April 8, 2015, 6:38 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 (updated)
-

  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