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

Reply via email to