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