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

Reply via email to