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