mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bernd Mathiske <be...@mesosphere.io>
Subject Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.
Date Thu, 03 Mar 2016 13:22:13 GMT

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




src/tests/cluster.hpp (line 168)
<https://reviews.apache.org/r/43613/#comment183674>

    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)
<https://reviews.apache.org/r/43613/#comment183687>

    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)
<https://reviews.apache.org/r/43613/#comment183678>

    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)
<https://reviews.apache.org/r/43613/#comment183675>

    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)
<https://reviews.apache.org/r/43613/#comment183676>

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



src/tests/cluster.cpp (line 404)
<https://reviews.apache.org/r/43613/#comment183677>

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


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