Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-03-19 Thread Michael Park 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review123785
---



Committed with the following formatting fixes.


src/tests/cluster.cpp (lines 157 - 158)


Fits on one line.



src/tests/cluster.cpp (lines 220 - 221)


Fits on one line.



src/tests/cluster.cpp (lines 240 - 241)


Fits on one line.



src/tests/cluster.cpp (lines 249 - 250)


Fits on one line.



src/tests/cluster.cpp (lines 266 - 268)


```
  slaveRemovalLimiter.isSome() ? slaveRemovalLimiter
   : master->slaveRemovalLimiter,
```



src/tests/cluster.cpp (lines 274 - 276)


```
StandaloneMasterDetector* _detector = CHECK_NOTNULL(
dynamic_cast(master->detector.get()));
```


- Michael Park


On March 14, 2016, 9:31 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> ---
> 
> (Updated March 14, 2016, 9:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem 
> Harutyunyan, and Michael Park.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope 
> of test objects to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  
> The `Slave` object performs cleanup originally found in 
> `cluster::Slave::stop`.  `cluster::Master::start` and `cluster::Slave::start` 
> were changed to factory methods.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp e5796d3cc17f814bec8f02dccf40515b65cfea91 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-03-15 Thread Michael Park 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review123727
---


Ship it!




I think there are a few parts that are a bit clunky, the `zookeeperURL` for 
example, but I think this is a good first iteration to ship to make our tests 
more stable.

- Michael Park


On March 14, 2016, 9:31 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> ---
> 
> (Updated March 14, 2016, 9:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem 
> Harutyunyan, and Michael Park.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope 
> of test objects to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  
> The `Slave` object performs cleanup originally found in 
> `cluster::Slave::stop`.  `cluster::Master::start` and `cluster::Slave::start` 
> were changed to factory methods.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp e5796d3cc17f814bec8f02dccf40515b65cfea91 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-03-14 Thread Joseph Wu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/
---

(Updated March 14, 2016, 2:31 p.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem Harutyunyan, 
and Michael Park.


Changes
---

Rebase and integrate Alexander's authorizer changes.


Bugs: MESOS-4633 and MESOS-4634
https://issues.apache.org/jira/browse/MESOS-4633
https://issues.apache.org/jira/browse/MESOS-4634


Repository: mesos


Description
---

Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope of 
test objects to the test body.

Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  The 
`Slave` object performs cleanup originally found in `cluster::Slave::stop`.  
`cluster::Master::start` and `cluster::Slave::start` were changed to factory 
methods.


Diffs (updated)
-

  src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
  src/tests/cluster.cpp e5796d3cc17f814bec8f02dccf40515b65cfea91 

Diff: https://reviews.apache.org/r/43613/diff/


Testing
---

Tests are run at the end of this review chain.


Thanks,

Joseph Wu

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-03-10 Thread Joseph Wu 


> On March 9, 2016, 10:46 a.m., Michael Park wrote:
> > src/tests/cluster.hpp, line 88
> > 
> >
> > Why do we need to return `Try` as opposed to 
> > `Try`?
> 
> Joseph Wu wrote:
> We return an `Owned` so that the tests can use `.reset()` to destruct the 
> master/agents.  If we just had a `Try<...>`, the tests would have to rely on 
> scope to destruct, which could get ugly if you have combinations of 
> masters/agents in a test :)
> 
> i.e.
> ```
> Try master = StartMaster();
> Try slave = StartSlave(...);
> 
> // Do stuff.
> 
> master->reset();
> slave->reset();
> 
> // More stuff
> ```
> Vs.
> ```
> {
>   // Have to construct slave after master...
>   Try slave;
>   
>   {
> Try master = StartMaster();
> slave = StartSlave(...);
> 
> // Do stuff.
>   }
> }
> 
> // More stuff.
> ```
> 
> Michael Park wrote:
> I think your second example you meant to leave out the `Owned`, right?
> Assuming that is the case, I would consider using `Try

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-03-09 Thread Michael Park 


> On March 9, 2016, 6:46 p.m., Michael Park wrote:
> > src/tests/cluster.hpp, line 88
> > 
> >
> > Why do we need to return `Try` as opposed to 
> > `Try`?
> 
> Joseph Wu wrote:
> We return an `Owned` so that the tests can use `.reset()` to destruct the 
> master/agents.  If we just had a `Try<...>`, the tests would have to rely on 
> scope to destruct, which could get ugly if you have combinations of 
> masters/agents in a test :)
> 
> i.e.
> ```
> Try master = StartMaster();
> Try slave = StartSlave(...);
> 
> // Do stuff.
> 
> master->reset();
> slave->reset();
> 
> // More stuff
> ```
> Vs.
> ```
> {
>   // Have to construct slave after master...
>   Try slave;
>   
>   {
> Try master = StartMaster();
> slave = StartSlave(...);
> 
> // Do stuff.
>   }
> }
> 
> // More stuff.
> ```

I think your second example you meant to leave out the `Owned`, right?
Assuming that is the case, I would consider using `Try

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-03-09 Thread Joseph Wu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/
---

(Updated March 9, 2016, 12:10 p.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem Harutyunyan, 
and Michael Park.


Changes
---

Change Master/Slave default constructor to use the `default` keyword.


Bugs: MESOS-4633 and MESOS-4634
https://issues.apache.org/jira/browse/MESOS-4633
https://issues.apache.org/jira/browse/MESOS-4634


Repository: mesos


Description
---

Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope of 
test objects to the test body.

Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  The 
`Slave` object performs cleanup originally found in `cluster::Slave::stop`.  
`cluster::Master::start` and `cluster::Slave::start` were changed to factory 
methods.


Diffs (updated)
-

  src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
  src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 

Diff: https://reviews.apache.org/r/43613/diff/


Testing
---

Tests are run at the end of this review chain.


Thanks,

Joseph Wu

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-03-09 Thread Joseph Wu 


> On March 9, 2016, 10:46 a.m., Michael Park wrote:
> > src/tests/cluster.hpp, line 88
> > 
> >
> > Why do we need to return `Try` as opposed to 
> > `Try`?

We return an `Owned` so that the tests can use `.reset()` to destruct the 
master/agents.  If we just had a `Try<...>`, the tests would have to rely on 
scope to destruct, which could get ugly if you have combinations of 
masters/agents in a test :)

i.e.
```
Try master = StartMaster();
Try slave = StartSlave(...);

// Do stuff.

master->reset();
slave->reset();

// More stuff
```
Vs.
```
{
  // Have to construct slave after master...
  Try slave;
  
  {
Try master = StartMaster();
slave = StartSlave(...);

// Do stuff.
  }
}

// More stuff.
```


> On March 9, 2016, 10:46 a.m., Michael Park wrote:
> > src/tests/cluster.hpp, line 108
> > 
> >
> > We're storing a `process::Owned`, but returning an 
> > instance of `process::Owned` here. This probably only works 
> > because `Owned` is implemented as `Shared` currently. This should either be 
> > `Shared`, or something like `const MasterDetector&`/`const MasterDetector*`.

Each call to `detector()` returns a new instance of a MasterDetector; the 
comment above this line should say the same thing.  (Am I interpretting your 
issue correctly?)

This is mostly a convenience function so that you don't see something like this 
in all the tests:
```
// For the non-zookeeper case:
Owned detector(new StandaloneMasterDetector(master.get()->pid);
```


> On March 9, 2016, 10:46 a.m., Michael Park wrote:
> > src/tests/cluster.hpp, line 141
> > 
> >
> > The zookeeper setup is for multi-master setup, right? Before, we had a 
> > zookeeper url per-`Masters`, now we have it per-`Master`. Is the idea to 
> > simply store duplicates instead?

If we had multi-master tests, this would store duplicates (we also store it in 
MesosTest).  (Maybe it will be useful in future to store different zookeeper 
URLs per master?)  We currently don't support multi-master tests, and I'm 
guessing we will tweak zookeeper variables when we move to support multi-master 
tests.


> On March 9, 2016, 10:46 a.m., Michael Park wrote:
> > src/tests/cluster.hpp, line 142
> > 
> >
> > `Files` used to be at the cluster level, now it is at a per-`Master` 
> > level. Could you explain briefly what this is used for and why it's ok to 
> > do this?

`Files` is also at a per-`Slave` level now :)

The `Files` is mostly a dependency that doesn't play a big role in the tests.  
Besides `files_test.cpp`, I believe tests don't use `Files` at all.
(All it does is expose some files on disk via an HTTP endpoint.)

It's safe to have multiple instances of `Files` because of the above reason, 
and because `Files` is read-only.


> On March 9, 2016, 10:46 a.m., Michael Park wrote:
> > src/tests/cluster.hpp, lines 236-237
> > 
> >
> > Could you explain why this used to be owned, but now is non-owned?

The `MasterDetector` used to be populated by the `Cluster` object, because the 
`Cluster` held a `Master` object.  When the `MasterDetector` was made in that 
way, the `Slave` would own the detector.

Now that `Master` and `Slave` live at the test level, all `MasterDetector` 
objects are owned in the test scope.
(If `MasterDetector` is always owned by the test, this eliminates the need for 
two `StartSlave` varieties for `MasterDetector*` and `Owned`.)

Note: Technically, we don't need to store this pointer in the `Slave` at the 
moment.  Compared to `Containerizer* containerizer;` which is used in the 
`Slave` destructor, the `detector` is merely passed into the `slave::Slave` 
object and left alone afterwards.


- Joseph


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review122760
---


On March 2, 2016, 1:45 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> ---
> 
> (Updated March 2, 2016, 1:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem 
> Harutyunyan, and Michael Park.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Major rewrite of the `tests/cluster` helpers.

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-03-09 Thread Michael Park 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review122760
---



Pass over the header file for questions + review of the API.


src/tests/cluster.hpp (line 82)


Why do we need to return `Try` as opposed to 
`Try`?



src/tests/cluster.hpp (line 93)


We're storing a `process::Owned`, but returning an instance 
of `process::Owned` here. This probably only works because 
`Owned` is implemented as `Shared` currently. This should either be `Shared`, 
or something like `const MasterDetector&`/`const MasterDetector*`.



src/tests/cluster.hpp (line 102)


`Master() = default;`



src/tests/cluster.hpp (line 108)


The zookeeper setup is for multi-master setup, right? Before, we had a 
zookeeper url per-`Masters`, now we have it per-`Master`. Is the idea to simply 
store duplicates instead?



src/tests/cluster.hpp (line 109)


`Files` used to be at the cluster level, now it is at a per-`Master` level. 
Could you explain briefly what this is used for and why it's ok to do this?



src/tests/cluster.hpp (lines 186 - 187)


Could you explain why this used to be owned, but now is non-owned?


- Michael Park


On March 2, 2016, 9:45 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> ---
> 
> (Updated March 2, 2016, 9:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem 
> Harutyunyan, and Michael Park.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope 
> of test objects to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  
> The `Slave` object performs cleanup originally found in 
> `cluster::Slave::stop`.  `cluster::Master::start` and `cluster::Slave::start` 
> were changed to factory methods.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-03-04 Thread Bernd Mathiske 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review122066
---


Ship it!




Note to reviewers: the resulting code can still be improved (as could the 
original), but if you focus just on the refactoring, without making additional 
improvements which could be addressed in extra patches, I think we are done 
here.

- Bernd Mathiske


On March 2, 2016, 1:45 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> ---
> 
> (Updated March 2, 2016, 1:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem 
> Harutyunyan, and Michael Park.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope 
> of test objects to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  
> The `Slave` object performs cleanup originally found in 
> `cluster::Slave::stop`.  `cluster::Master::start` and `cluster::Slave::start` 
> were changed to factory methods.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-03-04 Thread Bernd Mathiske 


> On March 3, 2016, 5:22 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.hpp, line 213
> > 
> >
> > I don't think we need this constructor. We are using an instance of 
> > this merely as an aggregate temporary variable for the injections into the 
> > actual instance to be constructed.
> 
> Joseph Wu wrote:
> I'd argue the small constructor is better for readability.
> 
> The alternative would involve adding one temporary variable for each 
> injection into the `cluster::Slave::start` factory.  The private constructor 
> would then look like:
> ```
> class Slave 
> {
> ...
> 
> private:
>   Slave(
>   MasterDetector* _detector,
>   slave::Flags _flags,
>   slave::Containerizer* _containerizer,
>   process::Owned& _ownedContainerizer,
>   process::Owned& _fetcher,
>   process::Owned& _gc,
>   process::Owned& _qosController,
>   process::Owned& _resourceEstimator,
>   process::Owned& _statusUpdateManager)
> : detector(_detector), 
>   flags(_flags),
>   containerizer(_containerizer),
>   ownedContainerizer(_ownedContainerizer),
>   fetcher(_fetcher),
>   gc(_gc),
>   qosController(_qosController),
>   resourceEstimator(_resourceEstimator),
>   statusUpdateManager(_statusUpdateManager)
>   {
> slave = new slave::Slave(
>   id.isSome() ? id.get() : process::ID::generate("slave"),
>   flags,
>   detector,
>   slave->containerizer,
>   >files,
>   gc.getOrElse(slave->gc.get()),
>   statusUpdateManager.getOrElse(slave->statusUpdateManager.get()),
>   resourceEstimator.getOrElse(slave->resourceEstimator.get()),
>   qosController.getOrElse(slave->qosController.get()));
>   }
> }
> ```
> 
> That's a lot of code bloat just to pass some variables around.
> Also, at least one constructor is necessary so that we can enforce the 
> `private`-ness of said constructor.  It wouldn't make sense to have tests 
> construct the `cluster::Slave` in any other way.

The way I see it we need exactly one constructor and that can be the 
non-trivial one. The objective is that all members can be const then. 

I don't mind a large number of temp variables in a local scope.

That said, I would agree with leaving the code as is for now, because it 
simplifies the current patch and does not introduce a second kind of change. 
However, I would recommend an extra patch on top of that for later.


> On March 3, 2016, 5:22 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.cpp, line 279
> > 
> >
> > We shouldn't really use a temp master object just to reset it with the 
> > same values it already has. I'd prefer a more functional style with only 
> > one constructor. See also the slave below.
> 
> Joseph Wu wrote:
> We aren't resetting the `cluster::Master` object though.  This line is 
> passing objects owned by `cluster::Master` into `master::Master`.
> 
> Would it be more clear if the line read like this?
> ```
>   master->master = new master::Master(...
> ```

Dropping for same reason as above. Refactoring and improving the programming 
style can be dojne in 2 steps instead of one. Let's stick to the refactoring 
for now.


> On March 3, 2016, 5:22 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.cpp, line 339
> > 
> >
> > Before this patch, with a cluster object that holds all masters, it 
> > makes sense to revoke the authenticator for all masters at shutdown. 
> > However, in our tests, each master constructor sets the authenticator in 
> > libprocess the same way, with an equivalent value. And libprocess assumes 
> > ownership, i.e. it will destruct any lingering authenticator eventually. It 
> > will also destruct the previous one if a new one is set. 
> > 
> > Therefore, we don't need to actively unset the authenticator here. In 
> > fact, this prevents multi-master tests. If one master gets destructed, it 
> > takes the authenticator for the others with it. Because there is only one - 
> > in libprocess.
> 
> Joseph Wu wrote:
> There are a couple other problems with multi-master tests 
> (https://issues.apache.org/jira/browse/MESOS-2976).
> 
> Note that the comment right below was retained from here 
> https://reviews.apache.org/r/43614/diff/2#1.97
> ```
> // This means that multiple masters in tests are not supported.
> ```
> 
> ---
> 
> Looks like there are also a few tests that require this.
> On OSX, I removed this line and saw these tests fail:
> ```
> [  FAILED  ] MasterQuotaTest.NoAuthenticationNoAuthorization
>

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-03-03 Thread Joseph Wu 


> On March 3, 2016, 5:22 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.hpp, line 213
> > 
> >
> > I don't think we need this constructor. We are using an instance of 
> > this merely as an aggregate temporary variable for the injections into the 
> > actual instance to be constructed.

I'd argue the small constructor is better for readability.

The alternative would involve adding one temporary variable for each injection 
into the `cluster::Slave::start` factory.  The private constructor would then 
look like:
```
class Slave 
{
...

private:
  Slave(
  MasterDetector* _detector,
  slave::Flags _flags,
  slave::Containerizer* _containerizer,
  process::Owned& _ownedContainerizer,
  process::Owned& _fetcher,
  process::Owned& _gc,
  process::Owned& _qosController,
  process::Owned& _resourceEstimator,
  process::Owned& _statusUpdateManager)
: detector(_detector), 
  flags(_flags),
  containerizer(_containerizer),
  ownedContainerizer(_ownedContainerizer),
  fetcher(_fetcher),
  gc(_gc),
  qosController(_qosController),
  resourceEstimator(_resourceEstimator),
  statusUpdateManager(_statusUpdateManager)
  {
slave = new slave::Slave(
  id.isSome() ? id.get() : process::ID::generate("slave"),
  flags,
  detector,
  slave->containerizer,
  >files,
  gc.getOrElse(slave->gc.get()),
  statusUpdateManager.getOrElse(slave->statusUpdateManager.get()),
  resourceEstimator.getOrElse(slave->resourceEstimator.get()),
  qosController.getOrElse(slave->qosController.get()));
  }
}
```

That's a lot of code bloat just to pass some variables around.
Also, at least one constructor is necessary so that we can enforce the 
`private`-ness of said constructor.  It wouldn't make sense to have tests 
construct the `cluster::Slave` in any other way.


> On March 3, 2016, 5:22 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.cpp, line 279
> > 
> >
> > We shouldn't really use a temp master object just to reset it with the 
> > same values it already has. I'd prefer a more functional style with only 
> > one constructor. See also the slave below.

We aren't resetting the `cluster::Master` object though.  This line is passing 
objects owned by `cluster::Master` into `master::Master`.

Would it be more clear if the line read like this?
```
  master->master = new master::Master(...
```


> On March 3, 2016, 5:22 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.cpp, line 339
> > 
> >
> > Before this patch, with a cluster object that holds all masters, it 
> > makes sense to revoke the authenticator for all masters at shutdown. 
> > However, in our tests, each master constructor sets the authenticator in 
> > libprocess the same way, with an equivalent value. And libprocess assumes 
> > ownership, i.e. it will destruct any lingering authenticator eventually. It 
> > will also destruct the previous one if a new one is set. 
> > 
> > Therefore, we don't need to actively unset the authenticator here. In 
> > fact, this prevents multi-master tests. If one master gets destructed, it 
> > takes the authenticator for the others with it. Because there is only one - 
> > in libprocess.

There are a couple other problems with multi-master tests 
(https://issues.apache.org/jira/browse/MESOS-2976).

Note that the comment right below was retained from here 
https://reviews.apache.org/r/43614/diff/2#1.97
```
// This means that multiple masters in tests are not supported.
```

---

Looks like there are also a few tests that require this.
On OSX, I removed this line and saw these tests fail:
```
[  FAILED  ] MasterQuotaTest.NoAuthenticationNoAuthorization
[  FAILED  ] MasterQuotaTest.AuthorizeQuotaRequestsWithoutPrincipal
[  FAILED  ] PersistentVolumeEndpointsTest.NoAuthentication
[  FAILED  ] ReservationEndpointsTest.ReserveAndUnreserveNoAuthentication
```

I'm guessing these tests assume the authenticator is cleaned up in previous 
tests.


> On March 3, 2016, 5:22 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.cpp, line 431
> > 
> >
> > I suggest we avoid this temporary slave instance and create the real 
> > one in one swoop below. This is then closer to functional style.
> >  
> > Then there is no need to copy parameters to `slave->...` here either.

See my response about the `cluster::Slave` constructor above.  If you feel 
that's the proper way of doing things, then I'll change this accordingly.


- Joseph


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review121835

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-03-03 Thread Bernd Mathiske 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review121835
---




src/tests/cluster.hpp (line 168)


I don't think we need this constructor. We are using an instance of this 
merely as an aggregate temporary variable for the injections into the actual 
instance to be constructed.



src/tests/cluster.cpp (line 237)


We shouldn't really use a temp master object just to reset it with the same 
values it already has. I'd prefer a more functional style with only one 
constructor. See also the slave below.



src/tests/cluster.cpp (line 294)


Before this patch, with a cluster object that holds all masters, it makes 
sense to revoke the authenticator for all masters at shutdown. However, in our 
tests, each master constructor sets the authenticator in libprocess the same 
way, with an equivalent value. And libprocess assumes ownership, i.e. it will 
destruct any lingering authenticator eventually. It will also destruct the 
previous one if a new one is set. 

Therefore, we don't need to actively unset the authenticator here. In fact, 
this prevents multi-master tests. If one master gets destructed, it takes the 
authenticator for the others with it. Because there is only one - in libprocess.



src/tests/cluster.cpp (line 331)


I suggest we avoid this temporary slave instance and create the real one in 
one swoop below. This is then closer to functional style.
 
Then there is no need to copy parameters to `slave->...` here either.



src/tests/cluster.cpp (line 335)


These statements can be avoided if we don't use a temp slave var with 
default constructor.



src/tests/cluster.cpp (line 404)


This could move inside the constructor, so the pid can be a constant. But 
that's probably an uncommon pattern in Mesos. So OK to leave as is.


- Bernd Mathiske


On March 2, 2016, 1:45 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> ---
> 
> (Updated March 2, 2016, 1:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem 
> Harutyunyan, and Michael Park.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope 
> of test objects to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  
> The `Slave` object performs cleanup originally found in 
> `cluster::Slave::stop`.  `cluster::Master::start` and `cluster::Slave::start` 
> were changed to factory methods.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-03-02 Thread Joseph Wu 


> On March 1, 2016, 5:56 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.cpp, line 97
> > 
> >
> > The local assertions here do not stop the test outside the factory 
> > method from progressing. This hazard creates implicit dependencies between 
> > the implementation here and test code following call sites.
> > 
> > Not making the return type here a Try makes the whole method and code 
> > following its call sites more fragile. Everything may look rock-solid right 
> > now, but if anyone later makes any changes, she must keep in mind that none 
> > of the pointers in members of the resulting master object can become stale. 
> > Otherwise they can cause test crashes. Not saying there are crashes now, 
> > but the probability of introducing them just went up.
> > 
> > Worse: should every test writer read the implementation code here and 
> > consider this? This kind of dependency slows down development. I suggest we 
> > avoid it.
> > 
> > If there were still an ASSERT_SOME(master) in every test right behind 
> > the creation of the master, then the test would exit prematurely and no 
> > stale members could cause crashes.

Ok.  Changed all these asserts back into errors.


> On March 1, 2016, 5:56 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.cpp, line 313
> > 
> >
> > FAIL() is commonly used to indicate a test failure. But such a failure 
> > should pertain to the subject matter of the individual test, which is not 
> > the case here.
> > 
> > gtest might simply mark this test as failed, but the situation is 
> > actually worse. Here we are looking at a malfunction of non-specific 
> > support code.
> > 
> > However, if we use a Try return type as discussed above, the 
> > ASSERT_SOME in the top level test code makes things right again.
> > 
> > Still, strictly speaking, it "look more "correct" if writing this here, 
> > assuming the Try:
> > 
> > LOG(FATAL) << "Failed to wait for _recover";
> > return;

I've changed this one to return an Error.

Note that `LOG(FATAL)` is roughly equivalent to `EXIT(EXIT_FAILURE)`.  (Rather 
than `ASSERT_TRUE(false)`.)


> On March 1, 2016, 5:56 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.cpp, line 124
> > 
> >
> > In order to cause an Error to be returned as the overall result when 
> > such an assertion fires, you could capture a bool by reference that gets 
> > set at the very end of the inline closure. If the bool is false, we have an 
> > Error.

Ended up getting rid of the inner closure for the `start` factories.


- Joseph


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review121433
---


On Feb. 26, 2016, 11:50 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> ---
> 
> (Updated Feb. 26, 2016, 11:50 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem 
> Harutyunyan, and Michael Park.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope 
> of test objects to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  
> The `Slave` object performs cleanup originally found in 
> `cluster::Slave::stop`.  `cluster::Master::start` and `cluster::Slave::start` 
> were changed to factory methods.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-03-02 Thread Bernd Mathiske 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review121637
---




src/tests/cluster.cpp (line 134)


Since flags are a parameter here, I'd prefer an Error instead of an ASSERT 
in this place (and others). Yes, the ASSERT would work, too, in a way, but here 
is the difference in semantics compared to the ASSERT in line 116 above. There 
an internal operation of the factory function fails without any interdependence 
with the outside. Here, in line 134, an external input has been erroneous. We 
should catch this outside the factory function or report it back to there. I 
find this stylistically more clean.



src/tests/cluster.cpp (line 312)


Note that an ASSERT is only the correct measure to use here if we return an 
Error then and guard the factory function with an ASSERT in every test's top 
scope. Otherwise, CHECK was correct, preventing potential access to a NULL 
pointer.


- Bernd Mathiske


On Feb. 26, 2016, 11:50 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> ---
> 
> (Updated Feb. 26, 2016, 11:50 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem 
> Harutyunyan, and Michael Park.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope 
> of test objects to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  
> The `Slave` object performs cleanup originally found in 
> `cluster::Slave::stop`.  `cluster::Master::start` and `cluster::Slave::start` 
> were changed to factory methods.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-03-01 Thread Bernd Mathiske 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review121433
---




src/tests/cluster.cpp (line 91)


The local assertions here do not stop the test outside the factory method 
from progressing. This hazard creates implicit dependencies between the 
implementation here and test code following call sites.

Not making the return type here a Try makes the whole method and code 
following its call sites more fragile. Everything may look rock-solid right 
now, but if anyone later makes any changes, she must keep in mind that none of 
the pointers in members of the resulting master object can become stale. 
Otherwise they can cause test crashes. Not saying there are crashes now, but 
the probability of introducing them just went up.

Worse: should every test writer read the implementation code here and 
consider this? This kind of dependency slows down development. I suggest we 
avoid it.

If there were still an ASSERT_SOME(master) in every test right behind the 
creation of the master, then the test would exit prematurely and no stale 
members could cause crashes.



src/tests/cluster.cpp (line 109)


In order to cause an Error to be returned as the overall result when such 
an assertion fires, you could capture a bool by reference that gets set at the 
very end of the inline closure. If the bool is false, we have an Error.



src/tests/cluster.cpp (line 233)


FAIL() is commonly used to indicate a test failure. But such a failure 
should pertain to the subject matter of the individual test, which is not the 
case here.

gtest might simply mark this test as failed, but the situation is actually 
worse. Here we are looking at a malfunction of non-specific support code.

However, if we use a Try return type as discussed above, the ASSERT_SOME in 
the top level test code makes things right again.

Still, strictly speaking, it "look more "correct" if writing this here, 
assuming the Try:

LOG(FATAL) << "Failed to wait for _recover";
return;


- Bernd Mathiske


On Feb. 26, 2016, 11:50 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> ---
> 
> (Updated Feb. 26, 2016, 11:50 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem 
> Harutyunyan, and Michael Park.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope 
> of test objects to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  
> The `Slave` object performs cleanup originally found in 
> `cluster::Slave::stop`.  `cluster::Master::start` and `cluster::Slave::start` 
> were changed to factory methods.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-02-26 Thread Joseph Wu 


> On Feb. 26, 2016, 4:19 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.hpp, line 260
> > 
> >
> > All this is nice to know, but ho does it connect to the variable 
> > "isShutdown" and why is it named this way? What does "isShutdown == true" 
> > signify? This should be in the comment IMHO. Or maybe a better variableName 
> > could make this more easy to grasp? Suggestion:
> > 
> > bool cleanUpContainersInDestructor = true

I've reworked this group of associated comments; and renamed the bool while I 
was at it.

I think its clearer now (and more verbose about why it exists).


> On Feb. 26, 2016, 4:19 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.hpp, line 264
> > 
> >
> > This global var makes the code hard to follow.
> > 
> > And I am quite confused by the source code comment.
> > 
> > Which is the case here?
> > 
> > A. The tests in question execute shutdown logic themselves. So they 
> > still need the containerizer to be destroyed. But that's what isShutdown 
> > switches on and off futher down below, not the shutdown logic. 
> > 
> > B. The tests in question want the shutdown logic to be executed by 
> > "terminate()" but not the containerizer destruction. This is what setting 
> > isShutdown to true seems to cause. But the tests where I see terminate() in 
> > use sometimes need the containerizer destroyed, because they create a new 
> > one right away. And sometimes they do not.
> > 
> > What am I missing?

A) The tests execute shutdown logic themselves.  They want to destroy the 
containerizer, **but not the containers**.  `isShutdown` only determines 
whether we destroy containers.  The containerizer's destructor does not destroy 
containers.

B) Tests will do `slave->terminate()` to shutdown the slave.  They need to do 
`slave.reset()` to call destructors on all the slave's dependencies.
Tests need to recreate the containerizer because you cannot use the same 
containerizer instance over two slaves (i.e. you can't `containerizer->recover` 
twice).
Tests that don't recreate the containerizer all use the `TestContainerizer`, 
which is just a mock anyway.

(I'll leave this issue open so that you can close, if the new comments + my 
notes are sufficient.)


> On Feb. 26, 2016, 4:19 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.cpp, line 504
> > 
> >
> > Should this be placed after the cointainerizer destruction maybe? Say 
> > line 399.
> > 
> > But then I still do not get to observe a consistent use pattern of 
> > terminate(). See my comment above.

As noted above, the container**izer** isn't destructed here.  The *containers* 
are.


- Joseph


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review120685
---


On Feb. 26, 2016, 11:50 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> ---
> 
> (Updated Feb. 26, 2016, 11:50 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem 
> Harutyunyan, and Michael Park.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope 
> of test objects to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  
> The `Slave` object performs cleanup originally found in 
> `cluster::Slave::stop`.  `cluster::Master::start` and `cluster::Slave::start` 
> were changed to factory methods.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-02-26 Thread Joseph Wu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/
---

(Updated Feb. 26, 2016, 11:50 a.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem Harutyunyan, 
and Michael Park.


Changes
---

Reworked some comments and renamed `isShutdown`.


Bugs: MESOS-4633 and MESOS-4634
https://issues.apache.org/jira/browse/MESOS-4633
https://issues.apache.org/jira/browse/MESOS-4634


Repository: mesos


Description
---

Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope of 
test objects to the test body.

Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  The 
`Slave` object performs cleanup originally found in `cluster::Slave::stop`.  
`cluster::Master::start` and `cluster::Slave::start` were changed to factory 
methods.


Diffs (updated)
-

  src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
  src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 

Diff: https://reviews.apache.org/r/43613/diff/


Testing
---

Tests are run at the end of this review chain.


Thanks,

Joseph Wu

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-02-26 Thread Bernd Mathiske 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review120685
---




src/tests/cluster.hpp (line 111)


s/were/are/



src/tests/cluster.hpp (line 181)


All this is nice to know, but ho does it connect to the variable 
"isShutdown" and why is it named this way? What does "isShutdown == true" 
signify? This should be in the comment IMHO. Or maybe a better variableName 
could make this more easy to grasp? Suggestion:

bool cleanUpContainersInDestructor = true



src/tests/cluster.hpp (line 185)


This global var makes the code hard to follow.

And I am quite confused by the source code comment.

Which is the case here?

A. The tests in question execute shutdown logic themselves. So they still 
need the containerizer to be destroyed. But that's what isShutdown switches on 
and off futher down below, not the shutdown logic. 

B. The tests in question want the shutdown logic to be executed by 
"terminate()" but not the containerizer destruction. This is what setting 
isShutdown to true seems to cause. But the tests where I see terminate() in use 
sometimes need the containerizer destroyed, because they create a new one right 
away. And sometimes they do not.

What am I missing?



src/tests/cluster.cpp (line 368)


Should this be placed after the cointainerizer destruction maybe? Say line 
399.

But then I still do not get to observe a consistent use pattern of 
terminate(). See my comment above.


- Bernd Mathiske


On Feb. 24, 2016, 12:06 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> ---
> 
> (Updated Feb. 24, 2016, 12:06 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem 
> Harutyunyan, and Michael Park.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope 
> of test objects to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  
> The `Slave` object performs cleanup originally found in 
> `cluster::Slave::stop`.  `cluster::Master::start` and `cluster::Slave::start` 
> were changed to factory methods.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-02-19 Thread Joseph Wu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/
---

(Updated Feb. 19, 2016, 4:50 p.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Artem 
Harutyunyan.


Changes
---

Addressed Alexander's comments.  (Also see: 
https://reviews.apache.org/r/43615/diff/4-5/)


Bugs: MESOS-4633 and MESOS-4634
https://issues.apache.org/jira/browse/MESOS-4633
https://issues.apache.org/jira/browse/MESOS-4634


Repository: mesos


Description
---

Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope of 
test objects to the test body.

Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  The 
`Slave` object performs cleanup originally found in `cluster::Slave::stop`.  
`cluster::Master::start` and `cluster::Slave::start` were changed to factory 
methods.


Diffs (updated)
-

  src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
  src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 

Diff: https://reviews.apache.org/r/43613/diff/


Testing
---

Tests are run at the end of this review chain.


Thanks,

Joseph Wu

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-02-19 Thread Joseph Wu 


> On Feb. 19, 2016, 3:46 p.m., Alexander Rojas wrote:
> > src/tests/cluster.hpp, line 163
> > 
> >
> > Not yours (is the only unchanged line here) but if all other pointer 
> > objects are of type `process::Owned` but here it is `std::shared_ptr`. I 
> > think we could move towards consistency here and change the type to 
> > `process::Owned`.

Unfortunately, this one is a `shared_ptr` because the underlying `Master` 
objects expects it as a `shared_ptr` (I don't know why we break the pattern 
here.)

If we want to change the pattern, we would do what the 
TestingMesosSchedulerDriver does :)
```
// No-op destructor as _detector lives on the stack.
detector =
  std::shared_ptr(_detector, [](MasterDetector*) {});
```
But I don't think this is worth the extra complexity.  (We would need to copy 
the shared_ptr into the new no-op destructor and make sure that all future 
tests understand this.)


> On Feb. 19, 2016, 3:46 p.m., Alexander Rojas wrote:
> > src/tests/cluster.cpp, lines 250-251
> > 
> >
> > If we stick to `std::shared_ptr` I would suggest to change its 
> > contstruction to `std::make_shared`

I'll also change the 4 tests that pass in this shared_ptr.


> On Feb. 19, 2016, 3:46 p.m., Alexander Rojas wrote:
> > src/tests/cluster.hpp, line 96
> > 
> >
> > I'm pondering if calling the factory method `start()` is the right way. 
> > People are already used to the `create()` name. In that case we can also 
> > add an `start()` public method. Not an issue but an idea.
> > 
> > Same goes for slave.

I was pondering this too.

I kept the "start" because this factory method calls `process::spawn` before 
returning.


> On Feb. 19, 2016, 3:46 p.m., Alexander Rojas wrote:
> > src/tests/cluster.cpp, line 519
> > 
> >
> > Comming from a world with exceptions, destructors are not supposed to 
> > throw, which makes me feel uneasy about an `ASSERT` here. 
> > 
> > But feel free to drop.

A gtest assert doesn't throw, it actually returns; which I think still makes 
sense inside a destructor.


- Joseph


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review119989
---


On Feb. 19, 2016, 11:36 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> ---
> 
> (Updated Feb. 19, 2016, 11:36 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Artem 
> Harutyunyan.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope 
> of test objects to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  
> The `Slave` object performs cleanup originally found in 
> `cluster::Slave::stop`.  `cluster::Master::start` and `cluster::Slave::start` 
> were changed to factory methods.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-02-19 Thread Alexander Rojas 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review119989
---




src/tests/cluster.hpp (line 82)


I'm pondering if calling the factory method `start()` is the right way. 
People are already used to the `create()` name. In that case we can also add an 
`start()` public method. Not an issue but an idea.

Same goes for slave.



src/tests/cluster.hpp (lines 105 - 106)


I think we are slowly introducing the pattern of using default deleted 
functions in C++, if you do 

```shell
ag --cpp --ignore-dir='build' '= ?delete;' .
```

you will find a bunch of instances of it. Seems like a perfect opportunity 
to do:

```c++
Master(const Master&) = delete;
Master& operator=(const Master&) = delete;
```



src/tests/cluster.hpp (line 122)


Not yours (is the only unchanged line here) but if all other pointer 
objects are of type `process::Owned` but here it is `std::shared_ptr`. I think 
we could move towards consistency here and change the type to `process::Owned`.



src/tests/cluster.cpp (lines 192 - 193)


If we stick to `std::shared_ptr` I would suggest to change its 
contstruction to `std::make_shared`



src/tests/cluster.cpp (line 374)


Given that you are calling in this same function and there's no risk of 
races at this point, I will recommend using `[this]() {…`.



src/tests/cluster.cpp (line 396)


Comming from a world with exceptions, destructors are not supposed to 
throw, which makes me feel uneasy about an `ASSERT` here. 

But feel free to drop.


- Alexander Rojas


On Feb. 19, 2016, 8:36 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> ---
> 
> (Updated Feb. 19, 2016, 8:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Artem 
> Harutyunyan.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope 
> of test objects to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  
> The `Slave` object performs cleanup originally found in 
> `cluster::Slave::stop`.  `cluster::Master::start` and `cluster::Slave::start` 
> were changed to factory methods.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-02-19 Thread Joseph Wu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/
---

(Updated Feb. 19, 2016, 11:25 a.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Artem 
Harutyunyan.


Changes
---

Variety of commenting tweaks.


Bugs: MESOS-4633 and MESOS-4634
https://issues.apache.org/jira/browse/MESOS-4633
https://issues.apache.org/jira/browse/MESOS-4634


Repository: mesos


Description
---

Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope of 
test objects to the test body.

Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  The 
`Slave` object performs cleanup originally found in `cluster::Slave::stop`.  
`cluster::Master::start` and `cluster::Slave::start` were changed to factory 
methods.


Diffs (updated)
-

  src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
  src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 

Diff: https://reviews.apache.org/r/43613/diff/


Testing
---

Tests are run at the end of this review chain.


Thanks,

Joseph Wu

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-02-19 Thread Joseph Wu 


> On Feb. 19, 2016, 3:16 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.hpp, line 205
> > 
> >
> > Slightly unclear which is which. Attempt to clarify:
> > 
> > ... Once this method has been called, its receiver will no longer 
> > perform cleanup in its destructor.
> > 
> > Vernacular alternative:
> > 
> > ... When this method is called, this wrapper instance will no longer 
> > perform cleanup during its destruction.

I reworded and added a tidbit about *not* destroying containers when these two 
methods are called.


- Joseph


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review119858
---


On Feb. 19, 2016, 11:25 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> ---
> 
> (Updated Feb. 19, 2016, 11:25 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Artem 
> Harutyunyan.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope 
> of test objects to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  
> The `Slave` object performs cleanup originally found in 
> `cluster::Slave::stop`.  `cluster::Master::start` and `cluster::Slave::start` 
> were changed to factory methods.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-02-19 Thread Bernd Mathiske 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review119858
---



First pass. Sorry, did not get through it in one seating.


src/tests/cluster.hpp (line 77)


s/  / /

Here and in other places in this patch. Note that we only use one blank 
after a full stop everywhere else in the code base.



src/tests/cluster.hpp (line 154)


Slightly unclear which is which. Attempt to clarify:

... Once this method has been called, its receiver will no longer perform 
cleanup in its destructor.

Vernacular alternative:

... When this method is called, this wrapper instance will no longer 
perform cleanup during its destruction.



src/tests/cluster.hpp (line 159)


See above.



src/tests/cluster.hpp (line 181)


What do you mean by "only"? Does it make more sense if you erase that word?



src/tests/cluster.hpp (line 188)


How would a pointer manage anything?



src/tests/cluster.cpp (line 101)


s/NOTE: // no need to be dramatic about this little tidbit :-)

s/lambda/extra/



src/tests/cluster.cpp (line 136)


I think you can keep the first clause on the same line, then align 
underneath.


- Bernd Mathiske


On Feb. 17, 2016, 12:27 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> ---
> 
> (Updated Feb. 17, 2016, 12:27 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Artem 
> Harutyunyan.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope 
> of test objects to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  
> The `Slave` object performs cleanup originally found in 
> `cluster::Slave::stop`.  `cluster::Master::start` and `cluster::Slave::start` 
> were changed to factory methods.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-02-17 Thread Joseph Wu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/
---

(Updated Feb. 17, 2016, 12:27 p.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Artem 
Harutyunyan.


Changes
---

Accidentally used a default capture by reference.  Changed to default capture 
by value.


Bugs: MESOS-4633 and MESOS-4634
https://issues.apache.org/jira/browse/MESOS-4633
https://issues.apache.org/jira/browse/MESOS-4634


Repository: mesos


Description
---

Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope of 
test objects to the test body.

Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  The 
`Slave` object performs cleanup originally found in `cluster::Slave::stop`.  
`cluster::Master::start` and `cluster::Slave::start` were changed to factory 
methods.


Diffs (updated)
-

  src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
  src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 

Diff: https://reviews.apache.org/r/43613/diff/


Testing
---

Tests are run at the end of this review chain.


Thanks,

Joseph Wu

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-02-17 Thread Joseph Wu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/
---

(Updated Feb. 17, 2016, 10:25 a.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Artem 
Harutyunyan.


Changes
---

Moved `populate` private helper functions into lambda closures.  Expanded on 
cleanup comments.


Bugs: MESOS-4633 and MESOS-4634
https://issues.apache.org/jira/browse/MESOS-4633
https://issues.apache.org/jira/browse/MESOS-4634


Repository: mesos


Description
---

Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope of 
test objects to the test body.

Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  The 
`Slave` object performs cleanup originally found in `cluster::Slave::stop`.  
`cluster::Master::start` and `cluster::Slave::start` were changed to factory 
methods.


Diffs (updated)
-

  src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
  src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 

Diff: https://reviews.apache.org/r/43613/diff/


Testing
---

Tests are run at the end of this review chain.


Thanks,

Joseph Wu

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-02-17 Thread Joseph Wu 


> On Feb. 17, 2016, 8:32 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.hpp, line 91
> > 
> >
> > What do you mean by "some" of its state?

In both of these objects, we don't do everything possible to cleanup.  Some 
things might be left behind (i.e. files in the sandbox, docker stuff, etc).  

I'll expand the comment to include exactly what we clean up:

* We call the destructors of each owned individual injection.
* For master, we unset an authenticator.
* For agent, we destroy containers and undo some cgroups.


> On Feb. 17, 2016, 8:32 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.cpp, line 106
> > 
> >
> > How about using an inline closure with return type void here? Then you 
> > don't have to declare populate in the header and you don't have to pass all 
> > these parameters, you can capture them.
> > 
> > AFAIK we are merely solving a syntactical problem here: macro ASSERT_* 
> > contains "return ..." with a return type that does not match start(). Right?

Yeah, that seems neater.  Changing...


- Joseph


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review119464
---


On Feb. 16, 2016, 2:15 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> ---
> 
> (Updated Feb. 16, 2016, 2:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Artem 
> Harutyunyan.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope 
> of test objects to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  
> The `Slave` object performs cleanup originally found in 
> `cluster::Slave::stop`.  `cluster::Master::start` and `cluster::Slave::start` 
> were changed to factory methods.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-02-17 Thread Bernd Mathiske 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review119464
---




src/tests/cluster.hpp (line 77)


What do you mean by "some" of its state?



src/tests/cluster.cpp (line 100)


How about using an inline closure with return type void here? Then you 
don't have to declare populate in the header and you don't have to pass all 
these parameters, you can capture them.

AFAIK we are merely solving a syntactical problem here: macro ASSERT_* 
contains "return ..." with a return type that does not match start(). Right?


- Bernd Mathiske


On Feb. 16, 2016, 2:15 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> ---
> 
> (Updated Feb. 16, 2016, 2:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Artem 
> Harutyunyan.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope 
> of test objects to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  
> The `Slave` object performs cleanup originally found in 
> `cluster::Slave::stop`.  `cluster::Master::start` and `cluster::Slave::start` 
> were changed to factory methods.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 43613: Refactor cluster test helpers into self-contained objects.

2016-02-16 Thread Joseph Wu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/
---

Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Artem 
Harutyunyan.


Bugs: MESOS-4633 and MESOS-4634
https://issues.apache.org/jira/browse/MESOS-4633
https://issues.apache.org/jira/browse/MESOS-4634


Repository: mesos


Description
---

Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope of 
test objects to the test body.

Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  The 
`Slave` object performs cleanup originally found in `cluster::Slave::stop`.  
`cluster::Master::start` and `cluster::Slave::start` were changed to factory 
methods.


Diffs
-

  src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
  src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 

Diff: https://reviews.apache.org/r/43613/diff/


Testing
---

Tests are run at the end of this review chain.


Thanks,

Joseph Wu