> On March 4, 2016, 6:45 a.m., Bernd Mathiske wrote:
> >

I also noticed a couple of these:
```
  MesosSchedulerDriver driver(
    &sched, DEFAULT_FRAMEWORK_INFO, master.get()->pid, DEFAULT_CREDENTIAL);
```
Now fixed (there were two spaces rather than four).

---

Also went through and changed a couple of these:
```
  Future<SlaveReregisteredMessage> slaveReregisteredMessage =
    FUTURE_PROTOBUF(
        SlaveReregisteredMessage(), master.get()->pid, slave.get()->pid);
```
To:
```
  Future<SlaveReregisteredMessage> slaveReregisteredMessage =
    FUTURE_PROTOBUF(
        SlaveReregisteredMessage(), 
        master.get()->pid, 
        slave.get()->pid);
```


> On March 4, 2016, 6:45 a.m., Bernd Mathiske wrote:
> > src/tests/master_tests.cpp, line 1053
> > <https://reviews.apache.org/r/43615/diff/13/?file=1280079#file1280079line1053>
> >
> >     Indentation see above.

For the above 3 issues, see the note after the next reply below.


> On March 4, 2016, 6:45 a.m., Bernd Mathiske wrote:
> > src/tests/master_allocator_tests.cpp, line 515
> > <https://reviews.apache.org/r/43615/diff/13/?file=1280074#file1280074line515>
> >
> >     Line break after "=" is prefered.
> >     
> >     Please update other places like this, too.

Sort-of related.  I noticed some tech-debt and filed this: 
https://issues.apache.org/jira/browse/MESOS-4868 (I added newlines after 
`StartMaster` in those tests).


> On March 4, 2016, 6:45 a.m., Bernd Mathiske wrote:
> > src/tests/container_logger_tests.cpp, line 302
> > <https://reviews.apache.org/r/43615/diff/13/?file=1280056#file1280056line302>
> >
> >     This indentation is OK, but prefered is this:
> >     
> >         Try<Owned<cluster::Slave>> slave = 
> >           StartSlave(detector.get(), containerizer.get(), flags);
> >           
> >     Same elsewhere. Please update all of these.

Fixed!


> On March 4, 2016, 6:45 a.m., Bernd Mathiske wrote:
> > src/tests/master_tests.cpp, line 1101
> > <https://reviews.apache.org/r/43615/diff/13/?file=1280079#file1280079line1101>
> >
> >     Insert blank line above, please.

Also added blank lines for this case in:

MesosSchedulerDriverTest, DropAckIfStopCalledBeforeAbort
MesosSchedulerDriverTest, ExplicitAcknowledgements
SchedulerDriverEventTest, Offers
SlaveTest, HTTPScheduler
SlaveTest, HTTPSchedulerLiveUpgrade


> On March 4, 2016, 6:45 a.m., Bernd Mathiske wrote:
> > src/tests/master_tests.cpp, line 1932
> > <https://reviews.apache.org/r/43615/diff/13/?file=1280079#file1280079line1932>
> >
> >     The master needs to be stopped here, not below, right? Otherwise we 
> > might be missing part of what we want to test here.

Good catch :D

I also missed this in:

ReconciliationTest, SlaveInTransition


> On March 4, 2016, 6:45 a.m., Bernd Mathiske wrote:
> > src/tests/master_tests.cpp, line 1140
> > <https://reviews.apache.org/r/43615/diff/13/?file=1280079#file1280079line1140>
> >
> >     It's OK to continue with the first arg on the same line in such cases.
> >     
> >     Here and elsewhere.

I believe the preferred style is:
```
EXPECT_EQ(
    ...,
    ...);
```
Rather than:
```
EXPECT_EQ(...
          ...);
```
(This is based on how we indented the MasterMaintenanceTests.)


> On March 4, 2016, 6:45 a.m., Bernd Mathiske wrote:
> > src/tests/master_tests.cpp, line 3754
> > <https://reviews.apache.org/r/43615/diff/13/?file=1280079#file1280079line3754>
> >
> >     This could go in line 3661.

Didn't notice that extra space (after `detector`).  Fixed that too.


- Joseph


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


On March 4, 2016, 4:14 p.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43615/
> -----------------------------------------------------------
> 
> (Updated March 4, 2016, 4:14 p.m.)
> 
> 
> Review request for mesos, 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
> -------
> 
> Includes the following changes:
> 
> * Added the `<process/owned.hpp>` header where appropriate.
> * Added the namespace `using process::Owned;` where appropriate.
> * Generally replaced `Try<PID<Master>>` with `Owned<cluster::Master>`.  And 
> `Try<PID<Slave>>` with `Owned<cluster::Slave>`.
> * Added the (now required) `MasterDetector` argument to all slaves.  Before, 
> this was fetched from the first master in `Cluster`.
> * Removed `Shutdown();` from all tests.
> * Replaced `Stop(...)` with the appropriate master/slave destruction calls.
> * Wrap various slave objects in `Owned` (i.e. containerizers, isolators, 
> launchers, etc).
> * Replace `CHECK` in tests with `ASSERT`.
> 
> 
> Diffs
> -----
> 
>   src/tests/authentication_tests.cpp 85f14c3d453ca5aeffa1c915f38fe3031c2cf712 
>   src/tests/command_executor_tests.cpp 
> 0d2fcf6d4b8d9a925eb6748e6bd33cf279b8f7f8 
>   src/tests/container_logger_tests.cpp 
> 00f4129e46aa9268fbb66da25b34e61004fa87b2 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6aecd912fc84b72d2b64f7a842891fddcbc469ac 
>   src/tests/containerizer/external_containerizer_test.cpp 
> 8e1dbe306a088eb16cd3b9c6174b95fad5685da4 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> e72239a55724f1aeeec5362cc370c93dbeca7164 
>   src/tests/containerizer/isolator_tests.cpp 
> 342037ce0a5f8caa4e3cf1550b8f9a7cc328acf9 
>   src/tests/containerizer/memory_pressure_tests.cpp 
> 03879d99c371f296f8d9904666911b34209c114d 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 983a63333be160aefe5a32acb6111bb3c85230ec 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 5b685bfd842d0d98e8ea5ec5ddea8d2cd893dd81 
>   src/tests/credentials_tests.cpp 7edcc857e0f6f8e80e265deeec59d6349d392224 
>   src/tests/disk_quota_tests.cpp 413e562026a4fc9779f616e921ae2fa2ca51e012 
>   src/tests/exception_tests.cpp 6b71316d545e97f14a45daa14d0fd95204befd3b 
>   src/tests/executor_http_api_tests.cpp 
> 2fc0893f5f5e80a783296fb31b30abe86d92df1b 
>   src/tests/fault_tolerance_tests.cpp 
> f2b8dba809518cf716b2b5a7a6a8a5fe62e57646 
>   src/tests/gc_tests.cpp 61a8abb9581dc4602b197a88a677b19386969cbf 
>   src/tests/health_check_tests.cpp 59ef31970af2d255abe169dfbc2e6e0314d29e9a 
>   src/tests/hook_tests.cpp 88ad7806bf9f3194e434743c35199a896edea92c 
>   src/tests/master_allocator_tests.cpp 
> cba7c36471f93b678d94e1da0251a28a893696b1 
>   src/tests/master_authorization_tests.cpp 
> 29c89fb11da792c3e71eb880a19657ea225b3cc8 
>   src/tests/master_contender_detector_tests.cpp 
> 255ab8119a04b55bb4f1b61dee19c4be64499376 
>   src/tests/master_quota_tests.cpp 4fabc1473ec3e048afe7171abbb8d6e49e863847 
>   src/tests/master_slave_reconciliation_tests.cpp 
> d41178eb41df519073fc0890c5716bbc9fed6ad2 
>   src/tests/master_tests.cpp 36595772b34bcb8d37dbc74d247bdf4614f10150 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
>   src/tests/metrics_tests.cpp 419d275e0b32817388120222bd433ee6f4835efd 
>   src/tests/monitor_tests.cpp 869c9e032817e8859a968232d4a61556a3d53d45 
>   src/tests/oversubscription_tests.cpp 
> e528476cd83b0e3f7ae8cea7d86dfabc1f66484e 
>   src/tests/partition_tests.cpp 3776a0a104582f60b9f19ea58b011485194399b9 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 81185a161498394020a27f1f5bf747bac5425f43 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
>   src/tests/rate_limiting_tests.cpp 352a5e7a58b500c25c7c8a421047c4e79434e38e 
>   src/tests/reconciliation_tests.cpp 97112c4d64c75a16fdd7bbefd517a039fbf55b64 
>   src/tests/registrar_zookeeper_tests.cpp 
> 3df9779ee5d076e16f6a538326693a36f986b6d0 
>   src/tests/repair_tests.cpp bb104562659e135492f9857e5b452c8a0a9e97da 
>   src/tests/reservation_endpoints_tests.cpp 
> f95ae7a32c3809d150adf1e9e515a3b527e61699 
>   src/tests/reservation_tests.cpp d7f9de6f2bce061316916260f356efdb96ecd482 
>   src/tests/role_tests.cpp fc3a72894631279460ee7971a4627d73c3d8c351 
>   src/tests/scheduler_driver_tests.cpp 
> f6dc25d82ae5f1e77fc6ede7ff2660ed0d9ea039 
>   src/tests/scheduler_event_call_tests.cpp 
> 8c02ceeb3ec1783cb2f63f100700508e70f586e4 
>   src/tests/scheduler_http_api_tests.cpp 
> dfb0f51fec67a3951e396eab28eedb0dbf9493ae 
>   src/tests/slave_tests.cpp 124e9587180f2a55e659d966d1c9060234c19457 
>   src/tests/status_update_manager_tests.cpp 
> d64d3b8c96270478f6b681c038de77c3a9eb68fe 
>   src/tests/teardown_tests.cpp 6e9e2e64f1666c2a81f5d859ee014ee14365b6b0 
> 
> Diff: https://reviews.apache.org/r/43615/diff/
> 
> 
> Testing
> -------
> 
> sudo make check
> ```
>          | OSX | CentOS 7 | CentOS 6 | Debian 8 | Ubuntu 12 | Ubuntu 14 | 
> Ubuntu 15 |
>  Non-SSL |  :) |  !@#$    |    !^    |    ^&    |     :)    |    :)     |     
> :)    |
> With-SSL |  :) |  !@ $^&* |    !     |    :)    |     ^     |    :)     |     
> ^     |
> 
>  :) = Passed.
>   ! = DockerContainerizerTest.ROOT_DOCKER_LaunchWithPersistentVolumes
>   @ = ProvisionerDockerRegistryPullerTest.ROOT_INTERNET_CURL_ShellCommand
>   # = LinuxFilesystemIsolatorTest.ROOT_VolumeFromSandbox
>   $ = LinuxFilesystemIsolatorTest.ROOT_RecoverOrphanedPersistentVolume
>   % = MemoryPressureMesosTest.CGROUPS_ROOT_Statistics
>   ^ = MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery
>   & = LinuxFilesystemIsolatorTest.ROOT_SandboxEnvironmentVariable
>   * = LinuxFilesystemIsolatorTest.ROOT_MultipleContainers
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Reply via email to