You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2015/04/24 10:18:29 UTC
Re: Review Request 32398: Persisted the reservation state on the
slave.
-----------------------------------------------------------
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
>
>
Re: Review Request 32398: Persisted the reservation state on the
slave.
Posted by Michael Park <mc...@gmail.com>.
> On April 24, 2015, 8:18 a.m., Alexander Rukletsov wrote:
> > src/common/resources_utils.cpp, lines 45-46
> > <https://reviews.apache.org/r/32398/diff/2/?file=921004#file921004line45>
> >
> > 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.
I think this comment was addressed when I addressed Jie's. Could you confirm that the comment I added is satisfactory?
> On April 24, 2015, 8:18 a.m., Alexander Rukletsov wrote:
> > src/tests/reservation_tests.cpp, line 607
> > <https://reviews.apache.org/r/32398/diff/2/?file=921005#file921005line607>
> >
> > Why don't you use the "standard" amount of resources `"cpus:1;mem:512"` we usually do in tests? Here and below.
I had it like that before because `slaveFlags.resources` goes through `Containerizer::resources` which tries to probe the OS for available resources if unspecified as opposed to `Resources::parse`. It was a problem because I was doing `EXPECT_EQ`, but I've changed the test pattern here is to perform `EXPECT_TRUE` on `contains` so we now use the "standard" amount of resources.
> On April 24, 2015, 8:18 a.m., Alexander Rukletsov wrote:
> > src/common/resources.cpp, line 444
> > <https://reviews.apache.org/r/32398/diff/2/?file=921003#file921003line444>
> >
> > 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?
Yeah, that's correct.
> On April 24, 2015, 8:18 a.m., Alexander Rukletsov wrote:
> > src/common/resources.cpp, lines 442-445
> > <https://reviews.apache.org/r/32398/diff/2/?file=921003#file921003line442>
> >
> > 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)
> > ```
I used the same pattern that exists for `isPersistentVolume` and `validatePersistentVolume`. `validatePersistentVolume` essentially repeats the checks that are performed in `isPersistentVolume`:
```cpp
bool Resources::isPersistentVolume(const Resource& resource)
{
return resource.has_disk() && resource.disk().has_persistence();
}
```
```cpp
// Validates that all the given resources are persistent volumes.
Option<Error> validatePersistentVolume(
const RepeatedPtrField<Resource>& volumes)
{
foreach (const Resource& volume, volumes) {
if (!volume.has_disk()) {
return Error("Resource " + stringify(volume) + " does not have DiskInfo");
} else if (!volume.disk().has_persistence()) {
return Error("'persistence' is not set in DiskInfo");
}
}
return None();
}
```
This was discussed with BenM and Jie, and the conclusion was that since this is validation code, we should try to provide detailed error messages rather than simply "not a persistent volume".
I think the logic could be leveraged the other way around, where `isPersistentVolume` would simply call: `return validatePersistentVolume(resource).isNone();` but that means `validatePersistentVolume` would need to live in `resources.cpp` rather than in `master.cpp` :(
> On April 24, 2015, 8:18 a.m., Alexander Rukletsov wrote:
> > src/tests/reservation_tests.cpp, lines 639-646
> > <https://reviews.apache.org/r/32398/diff/2/?file=921005#file921005line639>
> >
> > What is the purpose of using the reverse order here?
It's becuase the `FUTURE_PROTOBUF` has a `EXPECT_CALL` hiding inside of it, and the expectations are satisfied in reverse order in `gmock`.
>From [Using Multiple Expectations](https://code.google.com/p/googlemock/wiki/ForDummies#Using_Multiple_Expectations):
> By default, when a mock method is invoked, Google Mock will search the expectations in the reverse order they are defined, and stop when an active expectation that matches the arguments is found (you can think of it as "newer rules override older ones.")
> On April 24, 2015, 8:18 a.m., Alexander Rukletsov wrote:
> > src/tests/reservation_tests.cpp, lines 657-658
> > <https://reviews.apache.org/r/32398/diff/2/?file=921005#file921005line657>
> >
> > What operation does this message correspond to? Could you please leave a short comment to all of them?
Added short comments to each message.
> On April 24, 2015, 8:18 a.m., Alexander Rukletsov wrote:
> > src/tests/reservation_tests.cpp, line 684
> > <https://reviews.apache.org/r/32398/diff/2/?file=921005#file921005line684>
> >
> > Even though `"reconnect"` is the default for `--recover`, let's be explicit and document our intention.
Added an explicit `slaveFlags.recover = "reconnect";`
> On April 24, 2015, 8:18 a.m., Alexander Rukletsov wrote:
> > src/tests/reservation_tests.cpp, line 720
> > <https://reviews.apache.org/r/32398/diff/2/?file=921005#file921005line720>
> >
> > Again, `shutdown` param is `false` by default, but let's be explicit about our intention here.
We now explicitly call `Stop(slave.get(), false);`
> On April 24, 2015, 8:18 a.m., Alexander Rukletsov wrote:
> > src/tests/reservation_tests.cpp, line 724
> > <https://reviews.apache.org/r/32398/diff/2/?file=921005#file921005line724>
> >
> > IIUC we expect slave recovery to happen here. Let's document it, how about
> > ```
> > Future<Nothing> slaveRecovers = FUTURE_DISPATCH(_, &Slave::recover);
> > ```
Added.
> On April 24, 2015, 8:18 a.m., Alexander Rukletsov wrote:
> > src/tests/reservation_tests.cpp, line 808
> > <https://reviews.apache.org/r/32398/diff/2/?file=921005#file921005line808>
> >
> > Let's name this guy `master2` for clarity.
Good idea, fixed.
> On April 24, 2015, 8:18 a.m., Alexander Rukletsov wrote:
> > src/tests/reservation_tests.cpp, lines 892-893
> > <https://reviews.apache.org/r/32398/diff/2/?file=921005#file921005line892>
> >
> > 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!
Added a `NOTE` describing the requirements for "compatibility".
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32398/#review81456
-----------------------------------------------------------
On May 11, 2015, 10:25 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32398/
> -----------------------------------------------------------
>
> (Updated May 11, 2015, 10:25 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-2491
> https://issues.apache.org/jira/browse/MESOS-2491
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Added `isDynamicallyReserved(resource)` which returns true if the resource is a dynamic reservation.
> * Added the `isDynamicallyReserved` condition onto `needCheckpointing`.
> * Updated the `applyCheckpointedResources` to consider dynamic reservations.
> * Added tests.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 4c036d36e0e8ab3852786dd32b2d983d45891624
> src/common/resources.cpp 235930ff2dbb3ea49a3a0696dc070f2bd56fba4b
> 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: Persisted the reservation state on the
slave.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On April 24, 2015, 8:18 a.m., Alexander Rukletsov wrote:
> > src/common/resources.cpp, lines 442-445
> > <https://reviews.apache.org/r/32398/diff/2/?file=921003#file921003line442>
> >
> > 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)
> > ```
>
> Michael Park wrote:
> I used the same pattern that exists for `isPersistentVolume` and `validatePersistentVolume`. `validatePersistentVolume` essentially repeats the checks that are performed in `isPersistentVolume`:
>
> ```cpp
> bool Resources::isPersistentVolume(const Resource& resource)
> {
> return resource.has_disk() && resource.disk().has_persistence();
> }
> ```
>
> ```cpp
> // Validates that all the given resources are persistent volumes.
> Option<Error> validatePersistentVolume(
> const RepeatedPtrField<Resource>& volumes)
> {
> foreach (const Resource& volume, volumes) {
> if (!volume.has_disk()) {
> return Error("Resource " + stringify(volume) + " does not have DiskInfo");
> } else if (!volume.disk().has_persistence()) {
> return Error("'persistence' is not set in DiskInfo");
> }
> }
>
> return None();
> }
> ```
>
> This was discussed with BenM and Jie, and the conclusion was that since this is validation code, we should try to provide detailed error messages rather than simply "not a persistent volume".
>
> I think the logic could be leveraged the other way around, where `isPersistentVolume` would simply call: `return validatePersistentVolume(resource).isNone();` but that means `validatePersistentVolume` would need to live in `resources.cpp` rather than in `master.cpp` :(
I see. Better error messages seem to be a reasonable price for this. Dropping.
> On April 24, 2015, 8:18 a.m., Alexander Rukletsov wrote:
> > src/common/resources_utils.cpp, lines 45-46
> > <https://reviews.apache.org/r/32398/diff/2/?file=921004#file921004line45>
> >
> > 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.
>
> Michael Park wrote:
> I think this comment was addressed when I addressed Jie's. Could you confirm that the comment I added is satisfactory?
Yep, good enough.
> On April 24, 2015, 8:18 a.m., Alexander Rukletsov wrote:
> > src/tests/reservation_tests.cpp, lines 639-646
> > <https://reviews.apache.org/r/32398/diff/2/?file=921005#file921005line639>
> >
> > What is the purpose of using the reverse order here?
>
> Michael Park wrote:
> It's becuase the `FUTURE_PROTOBUF` has a `EXPECT_CALL` hiding inside of it, and the expectations are satisfied in reverse order in `gmock`.
>
> From [Using Multiple Expectations](https://code.google.com/p/googlemock/wiki/ForDummies#Using_Multiple_Expectations):
> > By default, when a mock method is invoked, Google Mock will search the expectations in the reverse order they are defined, and stop when an active expectation that matches the arguments is found (you can think of it as "newer rules override older ones.")
Didn't know that, good to know, thanks!
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32398/#review81456
-----------------------------------------------------------
On May 12, 2015, 6:44 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32398/
> -----------------------------------------------------------
>
> (Updated May 12, 2015, 6:44 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Jie Yu.
>
>
> Bugs: MESOS-2491
> https://issues.apache.org/jira/browse/MESOS-2491
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Added `isDynamicallyReserved(resource)` which returns true if the resource is a dynamic reservation.
> * Added the `isDynamicallyReserved` condition onto `needCheckpointing`.
> * Updated the `applyCheckpointedResources` to consider dynamic reservations.
> * Added tests.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 4c036d36e0e8ab3852786dd32b2d983d45891624
> src/common/resources.cpp 235930ff2dbb3ea49a3a0696dc070f2bd56fba4b
> 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
>
>