mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Park" <mcyp...@gmail.com>
Subject Re: Review Request 32398: Persisted the reservation state on the slave.
Date Mon, 11 May 2015 22:25:53 GMT


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


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