mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rukletsov" <ruklet...@gmail.com>
Subject Re: Review Request 32398: Persisted the reservation state on the slave.
Date Wed, 13 May 2015 14:43:18 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message