Re: Review Request 48034: Added v1 files to CMakeLists.txt.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On May 30, 2016, 3:21 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48034/
> ---
> 
> (Updated May 30, 2016, 3:21 a.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was forgotten to be fixed as part of `821dc94`.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 9eabd781763adba39b60526c1b4a7d99b3f1 
> 
> Diff: https://reviews.apache.org/r/48034/diff/
> 
> 
> Testing
> ---
> 
> Ran cmake build under OS X.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 47521: Added creator principal to '/create-volumes' tests.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On May 27, 2016, 3:55 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47521/
> ---
> 
> (Updated May 27, 2016, 3:55 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Neil Conway.
> 
> 
> Bugs: MESOS-5005
> https://issues.apache.org/jira/browse/MESOS-5005
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A creator principal is added to the persistent volumes
> used in the PersistentVolumeEndpointsTests.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> a57461d881b2bf0175f83b50b0a46167acd5bd3e 
> 
> Diff: https://reviews.apache.org/r/47521/diff/
> 
> 
> Testing
> ---
> 
> `make check` was used to test on OSX.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 47516: Added creator principal to tests.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On May 26, 2016, 11:28 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47516/
> ---
> 
> (Updated May 26, 2016, 11:28 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Neil Conway.
> 
> 
> Bugs: MESOS-5005
> https://issues.apache.org/jira/browse/MESOS-5005
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a creator principal to the persistent
> volumes used in various tests.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> f43165388f29513ab8be6594ab6647e8f9eb5a93 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 4293416ac8434e9eb7e80724480a54936a2fe24a 
>   src/tests/disk_quota_tests.cpp 1564f70854ff5630da1d7348a034f726d67b1757 
>   src/tests/reservation_tests.cpp 2d7fb21e2fe153c2b62dfd60bbaccb350a157391 
>   src/tests/role_tests.cpp 24959d6e0f83ef7b62b0586be18661aa3cac91dd 
> 
> Diff: https://reviews.apache.org/r/47516/diff/
> 
> 
> Testing
> ---
> 
> `make check` was used to test on OSX at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 47522: Added creator principal to persistent volume tests.

[Bernd Mathiske]

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




src/tests/persistent_volume_tests.cpp (line 1448)
<https://reviews.apache.org/r/47522/#comment200238>

As we have "volume and subsequent statements in a scope block above, can we 
have the same here, please?



src/tests/persistent_volume_tests.cpp (line 1665)
<https://reviews.apache.org/r/47522/#comment200241>

Same here: above block around "volume" with braces, this one without.


- Bernd Mathiske


On May 26, 2016, 11:25 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47522/
> ---
> 
> (Updated May 26, 2016, 11:25 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Neil Conway.
> 
> 
> Bugs: MESOS-5005
> https://issues.apache.org/jira/browse/MESOS-5005
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a creator principal to the
> persistent volumes used in the PersistentVolumeTests.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> d246f35046fff469b847c908de2b305ae629212f 
> 
> Diff: https://reviews.apache.org/r/47522/diff/
> 
> 
> Testing
> ---
> 
> `make check` was used to test on OSX at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 47521: Added creator principal to '/create-volumes' tests.

[Bernd Mathiske]

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




src/tests/persistent_volume_endpoints_tests.cpp (line 660)
<https://reviews.apache.org/r/47521/#comment200231>

I don't think the reservation principal should be necessary here. Should 
not have any effect either way, right? But the documentation for this 
recommends not setting a reservation principal unless there is a reservation. 
Also I think we can avoid confusing by using None() here instead, so it is 
clear which parameter triggers the expected response.



src/tests/persistent_volume_endpoints_tests.cpp (line 718)
<https://reviews.apache.org/r/47521/#comment200232>

Same here.



src/tests/persistent_volume_endpoints_tests.cpp (line 1010)
<https://reviews.apache.org/r/47521/#comment200233>

In the above case, this part is within the scope block, here it is not. 
Please move the opening curly brace up.


- Bernd Mathiske


On May 26, 2016, 10:37 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47521/
> ---
> 
> (Updated May 26, 2016, 10:37 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Neil Conway.
> 
> 
> Bugs: MESOS-5005
> https://issues.apache.org/jira/browse/MESOS-5005
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A creator principal is added to the persistent volumes
> used in the PersistentVolumeEndpointsTests.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> a57461d881b2bf0175f83b50b0a46167acd5bd3e 
> 
> Diff: https://reviews.apache.org/r/47521/diff/
> 
> 
> Testing
> ---
> 
> `make check` was used to test on OSX.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 47359: Fixed a typo in persistent volume docs.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On May 24, 2016, 3:14 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47359/
> ---
> 
> (Updated May 24, 2016, 3:14 p.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a typo in persistent volume docs.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md c13d79124f0b5ea2a715b4d2990fda4e06b2fb02 
> 
> Diff: https://reviews.apache.org/r/47359/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 47360: Updated dynamic reservation and persistent volume docs.

[Bernd Mathiske]

> On May 14, 2016, 6:55 a.m., Guangya Liu wrote:
> > docs/persistent-volume.md, line 96
> > <https://reviews.apache.org/r/47360/diff/2/?file=1383410#file1383410line96>
> >
> > Can you please show more detail for `may take any value, or may be 
> > omitted.`
> > 
> > a) In which condition can take any value if the framework did not 
> > provide a principal.
> > b) In which condition the `principal` will be omitted if the framework 
> > did not provide a principal.
> > 
> > Ditto for others.
> 
> Greg Mann wrote:
> I changed the text slightly to clarify my meaning. This text is saying 
> that if frameworkInfo.principal is not provided, then in all cases 
> disk.persistence.principal can take any value or can be left unset. Let me 
> know if my changes don't make this any clearer!
> 
> Guangya Liu wrote:
> Thanks Greg, one minor comment for this: can we simplify the sentense as 
> that the `principal` will be ignored for such case?
> 
> Greg Mann wrote:
> It's not quite correct to say that the principal is ignored in such 
> cases; the principal in `DiskInfo` might be used for authorization even if 
> the framework doesn't provided a principal when registering.
> 
> Guangya Liu wrote:
> Got it, thanks Greg. I think that it would make sense if you can add 
> above info to the document to make sure end user know the role of principal 
> in DiskInfo even if framework does not register with a principal.

I agree. @greggoman, can you please add a few words concerning this?


- Bernd


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


On May 24, 2016, 3:09 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47360/
> ---
> 
> (Updated May 24, 2016, 3:09 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Neil Conway.
> 
> 
> Bugs: MESOS-5215
> https://issues.apache.org/jira/browse/MESOS-5215
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the documentation of RESERVE and
> CREATE operations, both via offer operations and
> operator endpoints. Specifically, we clarify the
> Mesos master's expectations for the values of the
> `principal` fields found in `ReservationInfo` and
> `DiskInfo.Persistence`.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md c13d79124f0b5ea2a715b4d2990fda4e06b2fb02 
>   docs/reservation.md a400d19aec7a48d122ba1c9c23d38d792b8dbe6f 
> 
> Diff: https://reviews.apache.org/r/47360/diff/
> 
> 
> Testing
> ---
> 
> Viewed with the Mesos website container: 
> https://github.com/mesosphere/mesos-website-container
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 47528: Updated validation tests with creator principal.

[Bernd Mathiske]

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


Fix it, then Ship it!





src/tests/master_validation_tests.cpp (line 466)
<https://reviews.apache.org/r/47528/#comment199673>

More Hamming distance than just the "2" would make the difference between 
the principals even easier to spot.


- Bernd Mathiske


On May 23, 2016, 9:44 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47528/
> ---
> 
> (Updated May 23, 2016, 9:44 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Neil Conway.
> 
> 
> Bugs: MESOS-5005
> https://issues.apache.org/jira/browse/MESOS-5005
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The master validation tests are updated to include the
> new `validate()` signature, and a new test is added,
> `CreateOperationValidationTest.NonMatchingPrincipal`,
> to ensure that a non-matching creator principal will
> be invalidated.
> 
> 
> Diffs
> -
> 
>   src/tests/master_validation_tests.cpp 
> ca4442aa1ef0087a7d058d1b3aa430a1dbc16960 
> 
> Diff: https://reviews.apache.org/r/47528/diff/
> 
> 
> Testing
> ---
> 
> `make check` was used to test on OSX at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 47522: Added creator principal to persistent volume tests.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On May 18, 2016, 1:06 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47522/
> ---
> 
> (Updated May 18, 2016, 1:06 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Neil Conway.
> 
> 
> Bugs: MESOS-5005
> https://issues.apache.org/jira/browse/MESOS-5005
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a creator principal to the
> persistent volumes used in the PersistentVolumeTests.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> d246f35046fff469b847c908de2b305ae629212f 
> 
> Diff: https://reviews.apache.org/r/47522/diff/
> 
> 
> Testing
> ---
> 
> `make check` was used to test on OSX at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 47520: Updated test helpers with creator principal.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On May 18, 2016, 1:04 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47520/
> ---
> 
> (Updated May 18, 2016, 1:04 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Neil Conway.
> 
> 
> Bugs: MESOS-5005
> https://issues.apache.org/jira/browse/MESOS-5005
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the `createPersistentVolume` and
> `createDiskInfo` test helper functions to accept an
> argument specifying the creator principal to be included
> in `DiskInfo.Persistence`.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 79bf1ff16412ce2a510a9b75ab1ac91c1c182653 
> 
> Diff: https://reviews.apache.org/r/47520/diff/
> 
> 
> Testing
> ---
> 
> `make check` was done on OSX at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 47519: Updated an example framework to specify its principal.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On May 18, 2016, 1:04 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47519/
> ---
> 
> (Updated May 18, 2016, 1:04 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Neil Conway.
> 
> 
> Bugs: MESOS-5005
> https://issues.apache.org/jira/browse/MESOS-5005
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the creator principal into the
> persistent volumes created by the persistent volume
> example framework.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 53a6f6f82101ebb75abdc8f586fb23ab13298979 
> 
> Diff: https://reviews.apache.org/r/47519/diff/
> 
> 
> Testing
> ---
> 
> Tested manually with the following commands:
> `bin/mesos-master.sh --ip=127.0.0.1 --work_dir="$HOME/var/mesos1" 
> --authenticate=false`
> `bin/mesos-slave.sh --master=127.0.0.1:5050 --work_dir="$HOME/var/mesos2" 
> --"resources=disk(test):2048;mem(test):2048;cpu(test):4"`
> `src/persistent-volume-framework --master=127.0.0.1:5050`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 47528: Updated validation tests with creator principal.

[Bernd Mathiske]

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




src/tests/master_validation_tests.cpp (line 451)
<https://reviews.apache.org/r/47528/#comment199144>

Please add a test description for every new test.



src/tests/master_validation_tests.cpp (line 454)
<https://reviews.apache.org/r/47528/#comment199145>

Where are we setting the principal for DiskInfo.Persistence?

How is this block of code any different from the one below?


- Bernd Mathiske


On May 18, 2016, 1:02 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47528/
> ---
> 
> (Updated May 18, 2016, 1:02 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Neil Conway.
> 
> 
> Bugs: MESOS-5005
> https://issues.apache.org/jira/browse/MESOS-5005
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The master validation tests are updated to include the
> new `validate()` signature, and a new test is added,
> `CreateOperationValidationTest.NonMatchingPrincipal`,
> to ensure that a non-matching creator principal will
> be invalidated.
> 
> 
> Diffs
> -
> 
>   src/tests/master_validation_tests.cpp 
> ca4442aa1ef0087a7d058d1b3aa430a1dbc16960 
> 
> Diff: https://reviews.apache.org/r/47528/diff/
> 
> 
> Testing
> ---
> 
> `make check` was used to test on OSX at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 47515: Enforced a constraint on `DiskInfo.Persistence.principal`.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On May 22, 2016, 9:37 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47515/
> ---
> 
> (Updated May 22, 2016, 9:37 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Neil Conway.
> 
> 
> Bugs: MESOS-5005
> https://issues.apache.org/jira/browse/MESOS-5005
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch enforces the constraint that the principal
> found in `DiskInfo.Persistence` should equal that of
> the framework or operator responsible for creating
> the volume.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp b36b439a1fa07c52146deff2b90728f92676ade3 
>   src/master/master.cpp 0c05938de3e4eaeea2187559e81559f85318fa73 
>   src/master/validation.hpp f29f9753c75e5abc3402ed23381edcea26517ab3 
>   src/master/validation.cpp f490b899758bdac9676a6f6939918efa6ac52781 
> 
> Diff: https://reviews.apache.org/r/47515/diff/
> 
> 
> Testing
> ---
> 
> `make check` was used to test on OSX at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 47515: Enforced a constraint on `DiskInfo.Persistence.principal`.

[Bernd Mathiske]

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




src/master/master.cpp (line 3750)
<https://reviews.apache.org/r/47515/#comment198337>

Although there does not seem to be a rule in the style guide for this, I'd 
suggest:
{code}
  Option principal = framework->info.has_principal() ? 
framework->info.principal() :
Option::none();
{code}



src/master/validation.cpp (line 823)
<https://reviews.apache.org/r/47515/#comment198335>

was -> has been
    
    same below


- Bernd Mathiske


On May 18, 2016, 1:01 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47515/
> ---
> 
> (Updated May 18, 2016, 1:01 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Neil Conway.
> 
> 
> Bugs: MESOS-5005
> https://issues.apache.org/jira/browse/MESOS-5005
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch enforces the constraint that the principal
> found in `DiskInfo.Persistence` should equal that of
> the framework or operator responsible for creating
> the volume.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp c4ca343baa318fbb05ba2cbc3ed892c16478dbcc 
>   src/master/master.cpp b8c732a6178777544f0d09708177d9c68ab0532b 
>   src/master/validation.hpp f29f9753c75e5abc3402ed23381edcea26517ab3 
>   src/master/validation.cpp f490b899758bdac9676a6f6939918efa6ac52781 
> 
> Diff: https://reviews.apache.org/r/47515/diff/
> 
> 
> Testing
> ---
> 
> `make check` was used to test on OSX at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 46242: Removed a check in Reserve operation validation.

[Bernd Mathiske]

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


Fix it, then Ship it!





src/tests/reservation_endpoints_tests.cpp (line 1442)
<https://reviews.apache.org/r/46242/#comment197146>

Unlike other similar lines you are not using DEFAULT_CREDENTIAL.principal() 
here. Does this mean you want to intentionally use a different one? If so how 
do you ensure this is different? Just "principal" could become the same if 
someone else "creatively" updates DEFAULT_CREDENTIAL.principal later... Maybe 
add some random letters?

On the other hand, I don't think it matters here, so we might as well 
create conformity to avoid confusion and use DEFAULT_CREDENTIAL.principal() 
instead.


- Bernd Mathiske


On April 14, 2016, 11:25 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46242/
> ---
> 
> (Updated April 14, 2016, 11:25 p.m.)
> 
> 
> Review request for mesos, Adam B and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5212
> https://issues.apache.org/jira/browse/MESOS-5212
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When a dynamic reservation is made by an operator
> or framework with no principal, this patch removes
> the check which enforced that the principal in
> ReservationInfo must be None() in that case.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 13423436a4e6361fde6fa75133eebf5c02c8381f 
>   src/tests/master_validation_tests.cpp 
> 8a5bf9477596f13b2fb3a1348337ad2fe53a034d 
>   src/tests/reservation_endpoints_tests.cpp 
> f014290ed9f279df4c774aeb7ce7bd38fd1cc854 
> 
> Diff: https://reviews.apache.org/r/46242/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 46813: Update distribute to setuptools.

[Bernd Mathiske]

> On April 29, 2016, 9:20 a.m., haosdent huang wrote:
> > 3rdparty/versions.am, line 25
> > 
> >
> > I saw the latest version of setuptools is `20.10.1` in 
> > https://pypi.python.org/pypi/setuptools Do we have any reasons to use 
> > 20.9.0?
> 
> Zhiwei Chen wrote:
> oh, it's latest version is 20.9.0 when I first testing this patch. I will 
> try to upgrade to the latest one.
> 
> Zhiwei Chen wrote:
> I found setuptools now upgrade to 21.0.0, it's version grows too fast[1] 
> and there seems no significant changes[2], so I decided not to catch up the 
> latest version.
> 
> [1]: https://github.com/pypa/setuptools/releases
> [2]: https://pythonhosted.org/setuptools/history.html
> 
> haosdent huang wrote:
> Got it, thank you very much for your explanation!

Thanks for checking! If nobody objects, I will merge this RR today.


- Bernd


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


On April 29, 2016, 2:13 a.m., Zhiwei Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46813/
> ---
> 
> (Updated April 29, 2016, 2:13 a.m.)
> 
> 
> Review request for mesos, Adam B, haosdent huang, Kapil Arya, and Joseph Wu.
> 
> 
> Bugs: MESOS-5244
> https://issues.apache.org/jira/browse/MESOS-5244
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update distribute to setuptools.
> 
> 
> Diffs
> -
> 
>   3rdparty/Makefile.am 0523151d924a1398334767bd401ae84e41bdceb7 
>   3rdparty/distribute-0.6.26.tar.gz 3a598ccd4bfdfdf250a6d7a28552677394f14a76 
>   3rdparty/setuptools-20.9.0.tar.gz PRE-CREATION 
>   3rdparty/versions.am a94723e213dbbbd1df7e1b784386023412f7b961 
>   LICENSE 19130d977986dc9e23872817251ffcaa5a3c2c7d 
>   configure.ac ae91a07e2bafd97566ac6e6873990302acad14c9 
>   mpi/mpiexec-mesos.in 021dd5b6b9c1c601b81d5bf49fda1bd23c2040ee 
>   src/Makefile.am 53de98f43629dc94f7619324369caf88407b2f41 
>   src/examples/python/test-containerizer.in 
> b8258e75a3cd5f8d18ab8eff8196c5374db4f629 
>   src/examples/python/test-executor.in 
> 6be0ad2d339ff43d074e9688978af1ddf956803c 
>   src/examples/python/test-framework.in 
> 59bab82962c5846c6be288f3d392986e146e9da6 
>   support/coverage.sh 7edfec27d29fa900d35894c7778fcb7cfde513b8 
> 
> Diff: https://reviews.apache.org/r/46813/diff/
> 
> 
> Testing
> ---
> 
> make && make install on Ubuntu 14.04/16.04, RHEL 6.6/7.1 and OS X 10.10.5.
> 
> NOTE: I tested on my RHEL 6.6 and failed, because the package 
> pytz-2010h-2.el6.noarch was installed, when I removed this package verything 
> ok. The default RHEL/CentOS 6.6 has no pytz-2010h-2.el6.noarch, so I think 
> it's ok for this patch.
> 
> 
> Thanks,
> 
> Zhiwei Chen
> 
>

Re: Review Request 46832: Fixed the CMake build.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On April 29, 2016, 6:31 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46832/
> ---
> 
> (Updated April 29, 2016, 6:31 a.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake builds failed to compile under Linux because of a missing include.
> This was due to a typo in 'src/CMakeLists.txt', resulting in
> 'state.pb.h' not being created.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt f991743f1a467f24a786e925983390a411792327 
> 
> Diff: https://reviews.apache.org/r/46832/diff/
> 
> 
> Testing
> ---
> 
> mkdir build && cd build && cmake .. && make (with Fedora 23)
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46813: Update distribute to setuptools.

[Bernd Mathiske]

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


Ship it!




Having applied this patch to the master branch, I ran builds and tests for 
CentOS 6, CentOS7, Debian 8, Fedora 23, Ubuntu 12/14/15. (With and without SLL 
enabled.) In each case, building with setuptools instead of distribute worked 
equally well. I also tried Ubuntu 16. In this case building works with 
setuptools, but not with distribute. I'd say let's move on and switch to 
setuptools now.

- Bernd Mathiske


On April 29, 2016, 2:13 a.m., Zhiwei Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46813/
> ---
> 
> (Updated April 29, 2016, 2:13 a.m.)
> 
> 
> Review request for mesos, Adam B, haosdent huang, Kapil Arya, and Joseph Wu.
> 
> 
> Bugs: MESOS-5244
> https://issues.apache.org/jira/browse/MESOS-5244
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update distribute to setuptools.
> 
> 
> Diffs
> -
> 
>   3rdparty/Makefile.am 0523151d924a1398334767bd401ae84e41bdceb7 
>   3rdparty/distribute-0.6.26.tar.gz 3a598ccd4bfdfdf250a6d7a28552677394f14a76 
>   3rdparty/setuptools-20.9.0.tar.gz PRE-CREATION 
>   3rdparty/versions.am a94723e213dbbbd1df7e1b784386023412f7b961 
>   LICENSE 19130d977986dc9e23872817251ffcaa5a3c2c7d 
>   configure.ac ae91a07e2bafd97566ac6e6873990302acad14c9 
>   mpi/mpiexec-mesos.in 021dd5b6b9c1c601b81d5bf49fda1bd23c2040ee 
>   src/Makefile.am 53de98f43629dc94f7619324369caf88407b2f41 
>   src/examples/python/test-containerizer.in 
> b8258e75a3cd5f8d18ab8eff8196c5374db4f629 
>   src/examples/python/test-executor.in 
> 6be0ad2d339ff43d074e9688978af1ddf956803c 
>   src/examples/python/test-framework.in 
> 59bab82962c5846c6be288f3d392986e146e9da6 
>   support/coverage.sh 7edfec27d29fa900d35894c7778fcb7cfde513b8 
> 
> Diff: https://reviews.apache.org/r/46813/diff/
> 
> 
> Testing
> ---
> 
> make && make install on Ubuntu 14.04/16.04, RHEL 6.6/7.1 and OS X 10.10.5.
> 
> NOTE: I tested on my RHEL 6.6 and failed, because the package 
> pytz-2010h-2.el6.noarch was installed, when I removed this package verything 
> ok. The default RHEL/CentOS 6.6 has no pytz-2010h-2.el6.noarch, so I think 
> it's ok for this patch.
> 
> 
> Thanks,
> 
> Zhiwei Chen
> 
>

Re: Review Request 46717: Added sketch of fetcher cache metrics.

[Bernd Mathiske]

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




src/slave/containerizer/fetcher.cpp (line 337)
<https://reviews.apache.org/r/46717/#comment194627>

Note that we don't necessarily use the whole cache dir. The agent flag 
fetcher_cache_size is free to be set to a different value, a logical usage 
limit.


- Bernd Mathiske


On April 26, 2016, 5:30 p.m., Michael Browning wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46717/
> ---
> 
> (Updated April 26, 2016, 5:30 p.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Bugs: MESOS-4760
> https://issues.apache.org/jira/browse/MESOS-4760
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added sketch of fetcher cache metrics.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.hpp 
> 9e3018dc087ed55c61b2824d0105bc5339b83043 
>   src/slave/containerizer/fetcher.cpp 
> 176d8863d1becd8864218a0012ab45c614f0ad77 
>   src/slave/metrics.cpp 86eb8db644227cb593e52305bfbd05444bb87a6e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
> 
> Diff: https://reviews.apache.org/r/46717/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Browning
> 
>

Re: Review Request 45209: Made sure all Python modules are installed.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On April 11, 2016, 5:15 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45209/
> ---
> 
> (Updated April 11, 2016, 5:15 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-5010
> https://issues.apache.org/jira/browse/MESOS-5010
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The way our build sets up PYTHONPATH it appears to pip as if the
> mesos.cli package is already installed when it is only present in some
> directory in PYTHONPATH, but not in the installation target location.
> pip subsequently skips mesos.cli leading to a broken install where
> files required by e.g., mesos-ps are not installed.
> 
> Explicitly ignoring installed packages lets us install mesos.cli.
> 
> This appears to be due to some change in behavior first occurring with
> pip-6.0.1.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
> 
> Diff: https://reviews.apache.org/r/45209/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 45039: Updated the comment for launching tasks and accepting offers.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On April 5, 2016, 6:27 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45039/
> ---
> 
> (Updated April 5, 2016, 6:27 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the task does not pass validation,
> its resources are considered declined.
> 
> 
> Diffs
> -
> 
>   docs/app-framework-development-guide.md 
> 1d8bebde67f69fd414509b8861571137d3569b46 
>   include/mesos/scheduler.hpp 14c7ff964aa7b94f439d16e605380661d2279d54 
>   src/java/src/org/apache/mesos/SchedulerDriver.java 
> bf866f5ebece2505eaa27bf39a1382cd1a2a069a 
>   src/python/interface/src/mesos/interface/__init__.py 
> 232890daa6d222ae1c86906bbc484c8e635c4eb7 
> 
> Diff: https://reviews.apache.org/r/45039/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44710: Fixed placement of mock call expectation for fetcher cache tests.

[Bernd Mathiske]

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

(Updated March 11, 2016, 7:53 a.m.)


Review request for mesos, Jie Yu, Jan Schlicht, and Till Toenshoff.


Changes
---

Bug ID, potential reviewers added.


Bugs: MESOS-2858
https://issues.apache.org/jira/browse/MESOS-2858


Repository: mesos


Description
---

The EXPECT_CALL() statement in question should not be repeated in the loop once 
per task. It should only be called exactly once before the driver is started.


Diffs
-

  src/tests/fetcher_cache_tests.cpp f9c48f5d938c2601cb8f826029d6969d676ab98e 

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


Testing
---

make check - all fetcher cache tests succeeded.


Thanks,

Bernd Mathiske

Review Request 44710: Fixed placement of mock call expectation for fetcher cache tests.

[Bernd Mathiske]

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

Review request for mesos.


Repository: mesos


Description
---

The EXPECT_CALL() statement in question should not be repeated in the loop once 
per task. It should only be called exactly once before the driver is started.


Diffs
-

  src/tests/fetcher_cache_tests.cpp f9c48f5d938c2601cb8f826029d6969d676ab98e 

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


Testing
---

make check - all fetcher cache tests succeeded.


Thanks,

Bernd Mathiske

Re: Review Request 43629: Especially updated tests to use the updated MesosTest helpers.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On March 4, 2016, 3:55 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43629/
> ---
> 
> (Updated March 4, 2016, 3:55 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
> ---
> 
> Continuation of https://reviews.apache.org/r/43615/ with a slightly different 
> pattern.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp f9c48f5d938c2601cb8f826029d6969d676ab98e 
>   src/tests/resource_offers_tests.cpp 
> 0bad45dd1dabecc88fef1ab46e8ea26718070b33 
>   src/tests/slave_recovery_tests.cpp bd7b94f3f1fac6705e5bf14c6f6103b540cde56c 
> 
> Diff: https://reviews.apache.org/r/43629/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43629: Especially updated tests to use the updated MesosTest helpers.

[Bernd Mathiske]

> On March 3, 2016, 5:55 a.m., Bernd Mathiske wrote:
> > src/tests/slave_recovery_tests.cpp, line 3461
> > <https://reviews.apache.org/r/43629/diff/4/?file=1278087#file1278087line3461>
> >
> > Why was this moved up here? Couldn't this be in line 3389/3402?
> 
> Joseph Wu wrote:
> In this case, it's because all injections of `Try<Owned> 
> slave` must be stack-allocated above.  Even though we use `containerizer2` 
> much later, if we put it lower down, it would be deallocated before the 
> second `slave` (and then segfault!).
> 
> Bernd Mathiske wrote:
> I am not following yet. 
> 
> 1. How could it be deallocated before it has been allocated? How could it 
> even be referenced then? I assume "it" refers to sontainerizer2. 
> 2. The second slave would still be constructed later than containerizer2.
> 
> Joseph Wu wrote:
> In the updated test, the items go on the stack like this:
> ```
> containerizer1
> containerizer2
> slave
> ```
> 
> If I don't move `containerizer2` up, the stack will look like this:
> ```
> containerizer1
> slave
> containerizer2
> ```
> because re-constructing `slave` doesn't change `slave`'s order in the 
> stack.

So it's about the destruction order, not the construction order. Good point!


- Bernd


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


On March 4, 2016, 3:55 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43629/
> ---
> 
> (Updated March 4, 2016, 3:55 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
> ---
> 
> Continuation of https://reviews.apache.org/r/43615/ with a slightly different 
> pattern.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp f9c48f5d938c2601cb8f826029d6969d676ab98e 
>   src/tests/resource_offers_tests.cpp 
> 0bad45dd1dabecc88fef1ab46e8ea26718070b33 
>   src/tests/slave_recovery_tests.cpp bd7b94f3f1fac6705e5bf14c6f6103b540cde56c 
> 
> Diff: https://reviews.apache.org/r/43629/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43615: Update test suite to use the reworked MesosTest helpers.

[Bernd Mathiske]

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


Fix it, then Ship it!





src/tests/scheduler_event_call_tests.cpp (line 367)
<https://reviews.apache.org/r/43615/#comment184551>

In most other places you have the blank line before the detector.



src/tests/slave_tests.cpp (line 179)
<https://reviews.apache.org/r/43615/#comment184552>

With this line you have now introduced a 3rd way to write this stretch of 
code. Please pick one. This one is probably the least controversial. :-)


- Bernd Mathiske


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 `` header where appropriate.
> * Added the namespace `using process::Owned;` where appropriate.
> * Generally replaced `Try<PID>` with `Owned`.  And 
> `Try<PID>` with `Owned`.
> * 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 
> 983a6be160aefe5a32acb6111bb3c85230ec 
>   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 
> bf

Re: Review Request 43615: Update test suite to use the reworked MesosTest helpers.

[Bernd Mathiske]

> On March 4, 2016, 6:45 a.m., Bernd Mathiske wrote:
> >
> 
> Joseph Wu wrote:
> I also noticed a couple of these:
> ```
>   MesosSchedulerDriver driver(
> , 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 =
> FUTURE_PROTOBUF(
> SlaveReregisteredMessage(), master.get()->pid, slave.get()->pid);
> ```
> To:
> ```
>   Future slaveReregisteredMessage =
> FUTURE_PROTOBUF(
> SlaveReregisteredMessage(), 
> master.get()->pid, 
> slave.get()->pid);
> ```

OK!


> 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.
> 
> Joseph Wu wrote:
> I believe the preferred style is:
> ```
> EXPECT_EQ(
> ...,
> ...);
> ```
> Rather than:
> ```
> EXPECT_EQ(...
>   ...);
> ```
> (This is based on how we indented the MasterMaintenanceTests.)

AFAIK, it depends on how long the args are. If they are short, either way is 
fine. If they are "too long" you are always right :-)


- Bernd


---
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 `` header where appropriate.
> * Added the namespace `using process::Owned;` where appropriate.
> * Generally replaced `Try<PID>` with `Owned`.  And 
> `Try<PID>` with `Owned`.
> * 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 
> 983a6be160aefe5a32acb6111bb3c85230ec 
>   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.

Re: Review Request 44448: Replaced EXIT(1) with EXIT(EXIT_FAILURE).

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On March 7, 2016, 5:09 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8/
> ---
> 
> (Updated March 7, 2016, 5:09 a.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/examples/balloon_framework.cpp 16eea37ee327c6da1db96a6f4e3e596be5a7a6ad 
>   src/examples/docker_no_executor_framework.cpp 
> cf7b4e1b4a27a3a6d484b1e205b5ca1c3ec583e1 
>   src/examples/load_generator_framework.cpp 
> 7d64b6617564f43ef383ee60d92da92b2c958c47 
>   src/examples/long_lived_framework.cpp 
> c4c3aa68dc3e6e001f9a746ea5151b8ad958856f 
>   src/examples/test_framework.cpp c9695c4aa20ab8e726ad47b2ebbd3f10777cf828 
>   src/examples/test_http_executor.cpp 
> 55a427fb240fdfa018ac70fa3af5a6654cb71979 
>   src/examples/test_http_framework.cpp 
> 0f8f0b75d68ad9f5da8bd96ca670fad29933a399 
>   src/executor/executor.cpp c3e95ea7e4edf78f2a65ddc15e213aba66e69db2 
>   src/jvm/jvm.cpp 909d34a0112b219456dce76126029c603077e66d 
>   src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
>   src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 
>   src/logging/logging.cpp 8d9e4e9b200a0df1c67d4e7cd57107b7780f9812 
>   src/master/master.cpp 57ff4a39039f573b8586bc03f873f97826b97f6f 
>   src/sched/sched.cpp 525255eec808c3fe5c0e38b3d1a2086bbd4eb171 
>   src/scheduler/scheduler.cpp b010a819132fb80810e7f8ce96778109f2e8b35e 
>   src/slave/containerizer/mesos/launcher.cpp 
> c4ae1c5c319f9def2f18bafbad102ddf25d3c9f4 
>   src/slave/containerizer/mesos/provisioner/store.cpp 
> 8324269c786d2894a4c009428f1a6ec381c209e5 
>   src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
>   src/tests/containerizer/cgroups_tests.cpp 
> acaed9b3f8a04964092cef413133834d0cf5a145 
>   src/tests/environment.cpp ee1bbe6b4e3dda1e27b63d71a08ef0d1d254741a 
> 
> Diff: https://reviews.apache.org/r/8/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44447: Replaced EXIT(1) with EXIT(EXIT_FAILURE) in libprocess.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On March 7, 2016, 5:09 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7/
> ---
> 
> (Updated March 7, 2016, 5:09 a.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp 
> 15b982f101994ce717ceeb29a1bd0028bd2ba940 
> 
> Diff: https://reviews.apache.org/r/7/diff/
> 
> 
> Testing
> ---
> 
> Chain tested in https://reviews.apache.org/r/8/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44446: Replaced EXIT(1) with EXIT(EXIT_FAILURE) in stout.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On March 7, 2016, 5:08 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6/
> ---
> 
> (Updated March 7, 2016, 5:08 a.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> 96ffc427007b3b1447d291bba0d029646118d4b3 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/fork.hpp 
> 8d8ed0b85c5e281e686b23acae202cfd2d1ffb33 
> 
> Diff: https://reviews.apache.org/r/6/diff/
> 
> 
> Testing
> ---
> 
> Chain tested in https://reviews.apache.org/r/8/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 43629: Especially updated tests to use the updated MesosTest helpers.

[Bernd Mathiske]

> On March 3, 2016, 5:55 a.m., Bernd Mathiske wrote:
> > src/tests/slave_recovery_tests.cpp, line 3461
> > <https://reviews.apache.org/r/43629/diff/4/?file=1278087#file1278087line3461>
> >
> > Why was this moved up here? Couldn't this be in line 3389/3402?
> 
> Joseph Wu wrote:
> In this case, it's because all injections of `Try<Owned> 
> slave` must be stack-allocated above.  Even though we use `containerizer2` 
> much later, if we put it lower down, it would be deallocated before the 
> second `slave` (and then segfault!).

I am not following yet. 

1. How could it be deallocated before it has been allocated? How could it even 
be referenced then? I assume "it" refers to sontainerizer2. 
2. The second slave would still be constructed later than containerizer2.


- Bernd


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


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/43629/
> ---
> 
> (Updated March 2, 2016, 1:45 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
> ---
> 
> Continuation of https://reviews.apache.org/r/43615/ with a slightly different 
> pattern.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp f9c48f5d938c2601cb8f826029d6969d676ab98e 
>   src/tests/resource_offers_tests.cpp 
> 0bad45dd1dabecc88fef1ab46e8ea26718070b33 
>   src/tests/slave_recovery_tests.cpp bd7b94f3f1fac6705e5bf14c6f6103b540cde56c 
> 
> Diff: https://reviews.apache.org/r/43629/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43630: Especially updated scheduler tests to use the updated MesosTest helpers.

[Bernd Mathiske]

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


Ship it!




- 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/43630/
> ---
> 
> (Updated March 2, 2016, 1:45 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
> ---
> 
> Continuation of https://reviews.apache.org/r/43615/ with re-ordering of some 
> local variables due to the order of destruction.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> 3faa8136cf57276295553910319480028f433e4c 
>   src/tests/scheduler_tests.cpp 2b1693eaf1a6106f5e7d269e4e3f6c353dd6f017 
> 
> Diff: https://reviews.apache.org/r/43630/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43615: Update test suite to use the reworked MesosTest helpers.

[Bernd Mathiske]

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




src/tests/container_logger_tests.cpp (line 301)
<https://reviews.apache.org/r/43615/#comment183944>

This indentation is OK, but prefered is this:

Try<Owned> slave = 
  StartSlave(detector.get(), containerizer.get(), flags);
  
Same elsewhere. Please update all of these.



src/tests/master_allocator_tests.cpp (line 507)
<https://reviews.apache.org/r/43615/#comment183945>

Line break after "=" is prefered.

Please update other places like this, too.



src/tests/master_allocator_tests.cpp (line 1264)
<https://reviews.apache.org/r/43615/#comment183946>

I like this arrangement of MockExecutor and TestContainerizer a lot better, 
too.



src/tests/master_tests.cpp (line 142)
<https://reviews.apache.org/r/43615/#comment183948>

This indentation is fine, but that one is easier to read:

EXPECT_EQ(slave.get()->pid.address.hostname().get(),
  offers.get()[0].hostname());



src/tests/master_tests.cpp (line 973)
<https://reviews.apache.org/r/43615/#comment183949>

EXPECT_EQ(master.get()->pid.address.ip, 
  net::IP(ntohl(masterInfo.get().ip(;



src/tests/master_tests.cpp (line 1030)
<https://reviews.apache.org/r/43615/#comment183950>

Indentation see above.



src/tests/master_tests.cpp (line 1076)
<https://reviews.apache.org/r/43615/#comment183951>

Insert blank line above, please.



src/tests/master_tests.cpp (line 1110)
<https://reviews.apache.org/r/43615/#comment183952>

It's OK to continue with the first arg on the same line in such cases.

Here and elsewhere.



src/tests/master_tests.cpp (line 1875)
<https://reviews.apache.org/r/43615/#comment183953>

The master needs to be stopped here, not below, right? Otherwise we might 
be missing part of what we want to test here.



src/tests/master_tests.cpp (line 2046)
<https://reviews.apache.org/r/43615/#comment183954>

Same as above. Aren't we stopping the master too late for our purposes?



src/tests/master_tests.cpp (line 3648)
<https://reviews.apache.org/r/43615/#comment183957>

This could go in line 3661.


- Bernd Mathiske


On March 3, 2016, 3:11 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43615/
> -------
> 
> (Updated March 3, 2016, 3:11 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 `` header where appropriate.
> * Added the namespace `using process::Owned;` where appropriate.
> * Generally replaced `Try<PID>` with `Owned`.  And 
> `Try<PID>` with `Owned`.
> * 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 
> 79f134996c4b80bf49cbb8bee28eab5e6b4f5822 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 983a6be160aefe5a32acb6111bb3c85230ec 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 5b685bfd842d0d98e8ea5ec5ddea

Re: Review Request 43614: Refactor MesosTest and remove cleanup logic.

[Bernd Mathiske]

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


Fix it, then Ship it!





src/tests/mesos.hpp (line 190)
<https://reviews.apache.org/r/43614/#comment183936>

If we were pedantic, we could make the order of the params the same in the 
comment and the signature.



src/tests/mesos.hpp (line 204)
<https://reviews.apache.org/r/43614/#comment183937>

Param order in comments vs. signature.


- 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/43614/
> ---
> 
> (Updated March 2, 2016, 1:45 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
> ---
> 
> Updates `StartMaster` and `StartSlave` test helpers to use the reworked 
> `cluster` helpers.  Removes all `MesosTest` cleanup logic and as well as the 
> helpers that accept a `MockExecutor` pointer.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 1d4f075e470a60040e17b9f011aea6202310c437 
>   src/tests/mesos.cpp e0f641c6828833de13a0a233e39ff6dc3f343d5c 
> 
> Diff: https://reviews.apache.org/r/43614/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.

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

[Bernd Mathiske]

> On March 3, 2016, 5:22 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.hpp, line 213
> > <https://reviews.apache.org/r/43613/diff/8/?file=1278081#file1278081line213>
> >
> > 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
> > <https://reviews.apache.org/r/43613/diff/8/?file=1278082#file1278082line279>
> >
> > 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
> > <https://reviews.apache.org/r/43613/diff/8/?file=1278082#file1278082line339>
> >
> > 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).
>

Re: Review Request 44336: Removed numeric suffixes where appropriate in allocator tests.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On March 3, 2016, 5:01 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44336/
> ---
> 
> (Updated March 3, 2016, 5:01 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We should not enumerate variables if the corresponding instance is the
> only one of such type in the test.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/44336/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> 
> `make check`
> 
> `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh 
> --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44335: Moved variable declarations closer to where they are used.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On March 3, 2016, 5:01 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44335/
> ---
> 
> (Updated March 3, 2016, 5:01 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/44335/diff/
> 
> 
> Testing
> ---
> 
> Tested as a chain in https://reviews.apache.org/r/44336/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44334: Cleaned up empty hashmaps from allocator tests.

[Bernd Mathiske]

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



"{}" is short, but cryptic. It is unclear what kind of entity is being passed 
here. "EMPTY" was not any better. "hashmap<SlaveID, Resources>()" at least 
revealed the type which hinted a little at the presumable purpose. A better 
identifier than "EMPTY" would beat all of the above. Maybe "NO_USED_RESOURCES"?

Could the parameter of the methods in question be supplied with a default 
value? Then no corresponding argument at all would appear in these tests.

- Bernd Mathiske


On March 3, 2016, 5 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44334/
> ---
> 
> (Updated March 3, 2016, 5 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/44334/diff/
> 
> 
> Testing
> ---
> 
> Tested as a chain in https://reviews.apache.org/r/44336/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 43629: Especially updated tests to use the updated MesosTest helpers.

[Bernd Mathiske]

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




src/tests/slave_recovery_tests.cpp (line 3342)
<https://reviews.apache.org/r/43629/#comment183697>

Why was this moved up here? Couldn't this be in line 3389/3402?


LGTM

- 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/43629/
> ---
> 
> (Updated March 2, 2016, 1:45 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
> ---
> 
> Continuation of https://reviews.apache.org/r/43615/ with a slightly different 
> pattern.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp f9c48f5d938c2601cb8f826029d6969d676ab98e 
>   src/tests/resource_offers_tests.cpp 
> 0bad45dd1dabecc88fef1ab46e8ea26718070b33 
>   src/tests/slave_recovery_tests.cpp bd7b94f3f1fac6705e5bf14c6f6103b540cde56c 
> 
> Diff: https://reviews.apache.org/r/43629/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43614: Refactor MesosTest and remove cleanup logic.

[Bernd Mathiske]

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



LGTM, pending dependencies.

- 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/43614/
> ---
> 
> (Updated March 2, 2016, 1:45 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
> ---
> 
> Updates `StartMaster` and `StartSlave` test helpers to use the reworked 
> `cluster` helpers.  Removes all `MesosTest` cleanup logic and as well as the 
> helpers that accept a `MockExecutor` pointer.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 1d4f075e470a60040e17b9f011aea6202310c437 
>   src/tests/mesos.cpp e0f641c6828833de13a0a233e39ff6dc3f343d5c 
> 
> Diff: https://reviews.apache.org/r/43614/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.

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

Re: Review Request 43615: Update test suite to use the reworked MesosTest helpers.

[Bernd Mathiske]

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



LGTM so far, but let's review this again after the changes suggested in the 
reviews this one depends on.

- Bernd Mathiske


On Feb. 29, 2016, 1:55 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43615/
> ---
> 
> (Updated Feb. 29, 2016, 1:55 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 `` header where appropriate.
> * Added the namespace `using process::Owned;` where appropriate.
> * Generally replaced `Try<PID>` with `Owned`.  And 
> `Try<PID>` with `Owned`.
> * 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 
> 6a60962b4593b3521c182c7320331743ccffd4ba 
>   src/tests/containerizer/isolator_tests.cpp 
> 7b257de2afbc66f63c47a80c1f828e3e95bd602d 
>   src/tests/containerizer/memory_pressure_tests.cpp 
> 79f134996c4b80bf49cbb8bee28eab5e6b4f5822 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 983a6be160aefe5a32acb6111bb3c85230ec 
>   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 
> 982468f851cd9d95eb6cde7c57f2d737d46a827c 
>   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_maintenance_tests.cpp 
> 3faa8136cf57276295553910319480028f433e4c 
>   src/tests/master_quota_tests.cpp 8357ec911b2a158632a708ae3adff6eabc536697 
>   src/tests/master_slave_reconciliation_tests.cpp 
> d41178eb41df519073fc0890c5716bbc9fed6ad2 
>   src/tests/master_tests.cpp 0bd8c0e42f335cad7ed858c6af5aa4f07bb37dbf 
>   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 caced732ded05a334861a53488ef6391885b2263 
>   src/tests/reconciliation_tests.cpp 97112c4d64c75a16fdd7bbefd517a039fbf55b64 
>   src/tests/registrar_zookeeper_test

Re: Review Request 43614: Refactor MesosTest and remove cleanup logic.

[Bernd Mathiske]

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




src/tests/mesos.hpp (line 133)
<https://reviews.apache.org/r/43614/#comment183412>

So this injection is allowed to be a shared_ptr and the others are not? 
This makes me wonder if it would not be simpler if all injections were 
shared_ptr, even the ones that are sometimes Owned and sometimes not (i.e. 
declared with a parallel member of type *). Then we would only need one kind 
for all cases and a lot of code would look substantially simpler. The only 
Owned ones would be those that are always Owned.


- 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/43614/
> ---
> 
> (Updated Feb. 16, 2016, 2:15 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
> ---
> 
> Updates `StartMaster` and `StartSlave` test helpers to use the reworked 
> `cluster` helpers.  Removes all `MesosTest` cleanup logic and as well as the 
> helpers that accept a `MockExecutor` pointer.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 242a11658c0a9ba4caced9b2b2bdbcb921f7fdd0 
>   src/tests/mesos.cpp e0f641c6828833de13a0a233e39ff6dc3f343d5c 
> 
> Diff: https://reviews.apache.org/r/43614/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.

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

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

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 44179: Reintroduce deleted anchor in configuration.md.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On Feb. 29, 2016, 4:36 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44179/
> ---
> 
> (Updated Feb. 29, 2016, 4:36 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This anchor was deleted by https://reviews.apache.org/r/42939 but is still 
> referenced by sandbox.md.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md c4d094aac4acb95fd6648071413a3d5160dbf381 
> 
> Diff: https://reviews.apache.org/r/44179/diff/
> 
> 
> Testing
> ---
> 
> Checked via gist.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

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

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

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

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

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 44029: Fetcher::basename should ignore query strings and fragments.

[Bernd Mathiske]

> On Feb. 26, 2016, 5:40 a.m., Bernd Mathiske wrote:
> > There is a more elaborate solution to this problem 
> > (https://reviews.apache.org/r/40054), but it requires a lot of code to 
> > implement URL parsing. Until we finalize that, I think the patch at hand 
> > gets the most urgent job done.
> 
> Jiang Yan Xu wrote:
> The concern here is that it changes the current behavior of the fetcher 
> output. Although we agree that probably most people would find the query 
> string in the filename annoying, there could be people who rely on this (and 
> with a workaround in place) and this will suddenly break things.
> 
> The explicit output filename proposed in 
> https://issues.apache.org/jira/browse/MESOS-3367 sounds like the right 
> approach.
> 
> Jiang Yan Xu wrote:
> When we have an alternative approach available to users, it's easier to 
> then come back and impose a deprecate cycle for the default behavior. What do 
> you think?
> 
> James Peach wrote:
> Yes, there's definitely compatibility concern (for example, 
> ``http://foo/bar/baz?filename.zip`` would no longer work. After chatting with 
> Yan, maybe it is sufficient to ignore the extension and just try all the 
> extractors. If it is extractable one of them will succeed. I don't think 
> there's any compatibility concerns here and we wouldn't need to do a 
> deprecation cycle.
> 
> Jiang Yan Xu wrote:
> So it occurred to me that here's an edge case:
> 
> Some executable archieves are in fact zip files, e.g., jar and pex files. 
> In the past they wouldn't get automatically decompressed due to their 
> suffixes and not that unzip couldn't decomopress them.
> 
> The original file is still going to be there but this implicates disk 
> quota, etc. Seemingly not an extremely dangerous case but probably still 
> warrants a deprecation cycle.

@jpeach, are you OK with discarding this approach and going with setting an 
explicit name instead? 
https://issues.apache.org/jira/browse/MESOS-4735


- Bernd


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


On Feb. 25, 2016, 10:32 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44029/
> ---
> 
> (Updated Feb. 25, 2016, 10:32 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4779
> https://issues.apache.org/jira/browse/MESOS-4779
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignore URL query parameters and fragments when determining this
> base name. This enables the fetcher to subsequently examine the
> file extension and extract archives correctly.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 33dfcade6beb53a5a6dbc41a8f3380f5cb30a161 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/44029/diff/
> 
> 
> Testing
> ---
> 
> ``make check`` on OS X.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44029: Fetcher::basename should ignore query strings and fragments.

[Bernd Mathiske]

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


Ship it!




There is a more elaborate solution to this problem 
(https://reviews.apache.org/r/40054), but it requires a lot of code to 
implement URL parsing. Until we finalize that, I think the patch at hand gets 
the most urgent job done.

- Bernd Mathiske


On Feb. 25, 2016, 10:32 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44029/
> ---
> 
> (Updated Feb. 25, 2016, 10:32 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4779
> https://issues.apache.org/jira/browse/MESOS-4779
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignore URL query parameters and fragments when determining this
> base name. This enables the fetcher to subsequently examine the
> file extension and extract archives correctly.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 33dfcade6beb53a5a6dbc41a8f3380f5cb30a161 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/44029/diff/
> 
> 
> Testing
> ---
> 
> ``make check`` on OS X.
> 
> 
> Thanks,
> 
> James Peach
> 
>

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

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

s/were/are/



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

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

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

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 43963: Fixed flakiness in DockerContainerizerTest.ROOT_DOCKER_Logs.

[Bernd Mathiske]

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




src/tests/containerizer/docker_containerizer_tests.cpp (line 1855)
<https://reviews.apache.org/r/43963/#comment182145>

Why the extra image? This test seems to run just fine with the other 
changes and simply "alpine".

What is the significance of "Expect" here?


- Bernd Mathiske


On Feb. 24, 2016, 9:43 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43963/
> ---
> 
> (Updated Feb. 24, 2016, 9:43 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Artem Harutyunyan, and Timothy Chen.
> 
> 
> Bugs: MESOS-4676
> https://issues.apache.org/jira/browse/MESOS-4676
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the `unbuffer` utility in front of each `echo` in the test.  Since 
> Docker appears to handle simultaneous stdout/stderr in a non-robust fashion, 
> this mitigates the amount of overlap the two streams will have in the test.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> a299c9e0744b5657984e5bb0afbe4874a266ddb6 
> 
> Diff: https://reviews.apache.org/r/43963/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> On Centos7: sudo bin/mesos-tests.sh --gtest_filter="\*ROOT_DOCKER_Logs" 
> --gtest_repeat=1200 --gtest_break_on_failure
> 
> ---
> 
> CI results: 
> sudo make check
> 
>  | OSX | CentOS 7 | CentOS 6 | Debian 8 | Ubuntu 15.10 | Ubuntu 14 | 
> Ubuntu 12 |
>  Non-SSL |  :) |:)| *|:)|   :) |:) |  
>:)|
> With-SSL |  :) |:)|&*| &|^ |:) |  
>:)|
> 
>  :) = Passed.
> 
> Known flaky tests:
>   * = DockerContainerizerTest.ROOT_DOCKER_LaunchWithPersistentVolumes
>   & = MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery
>   ^ = LimitedCpuIsolatorTest.ROOT_CGROUPS_Pids_and_Tids
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43961: Added some additional synchronization in ROOT_CGROUPS_Pids_and_Tids.

[Bernd Mathiske]

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


Fix it, then Ship it!





src/tests/containerizer/isolator_tests.cpp (line 813)
<https://reviews.apache.org/r/43961/#comment182141>

"finished exec'ing" reads like "finished executing", i.e. the process has 
terminated. But that's not what is meant here! Only after I realized this, the 
sequence of events here became clear to me. Suggestion:

// This observation ensures that the "cat" process 
// has completed its part of the exec() procedure 
// and is now executing normally."


- Bernd Mathiske


On Feb. 24, 2016, 9:59 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43961/
> ---
> 
> (Updated Feb. 24, 2016, 9:59 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Chi Zhang, Artem Harutyunyan, and 
> Jian Qiu.
> 
> 
> Bugs: MESOS-4677
> https://issues.apache.org/jira/browse/MESOS-4677
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds a set of pipes to mitigate a race between `exec`ing + updating cgroup 
> tasks and the test reading said cgroup tasks.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/isolator_tests.cpp 
> 653b037c489072f43e53dec01a811a9249dcd660 
> 
> Diff: https://reviews.apache.org/r/43961/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Centos7: sudo bin/mesos-tests.sh --gtest_filter="*ROOT_CGROUPS_Pids_and_Tids" 
> --gtest_repeat=5000 --gtest_break_on_failure
> 
> ---
> 
> CI results:
> 
> sudo make check
> 
>  | OSX | CentOS 7 | CentOS 6 | Debian 8 | Ubuntu 15.10 | Ubuntu 14 | 
> Ubuntu 12 |
>  Non-SSL |  :) |@_@   | *|X_X   |   :) |:) |  
>:)|
> With-SSL |  :) |@_@   | *| &|   :) |:) |  
>:)|
> 
>  :) = Passed.
> X_X = Configuration error with docker.
> @_@ = Ran out of disk space in some tests: 
> LinuxFilesystemIsolatorTest.ROOT_SandboxEnvironmentVariable, 
> LinuxFilesystemIsolatorTest.ROOT_MultipleContainers, 
> MesosContainerizerLaunchTest.ROOT_ChangeRootfs, 
> LinuxFilesystemIsolatorTest.ROOT_ImageInVolumeWithRootFilesystem
> 
>   * = DockerContainerizerTest.ROOT_DOCKER_LaunchWithPersistentVolumes (Known 
> issue)
>   % = DockerContainerizerTest.ROOT_DOCKER_Logs
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43850: Added waits at different points in the test to ensure milestones.

[Bernd Mathiske]

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


Ship it!





src/tests/containerizer/memory_pressure_tests.cpp (line 242)
<https://reviews.apache.org/r/43850/#comment181935>

Nice pattern. We try not to capture by reference, but here this is put to 
use to great effect.


- Bernd Mathiske


On Feb. 24, 2016, 1:10 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43850/
> ---
> 
> (Updated Feb. 24, 2016, 1:10 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Joseph Wu.
> 
> 
> Bugs: MESOS-4047
> https://issues.apache.org/jira/browse/MESOS-4047
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Sometimes MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery will fail because
> the tracker of the cgroups pressure counter is not finished by the time the
> expectations are set.
> 
> This patch ensures that different test milestones are reached; e.g. slave
> is registered, KILLED_TASK reached the scheduler, etc; before continuing
> with the test.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/memory_pressure_tests.cpp 
> 4a03af2c9c0643d964b1d76e2096341b59bf5dce 
> 
> Diff: https://reviews.apache.org/r/43850/diff/
> 
> 
> Testing
> ---
> 
> On a CentOS 6.7 virtual box machine:
> 
> ```bash
> MESOS_VERBOSE=1 sudo .libs/mesos-tests 
> --gtest_filter="MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery" 
> --gtest_repeat=1000 --gtest_break_on_failure
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 43849: Fixed typos in subprocess.cpp comments.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On Feb. 23, 2016, 9:49 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43849/
> ---
> 
> (Updated Feb. 23, 2016, 9:49 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed typos in subprocess.cpp comments.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess.cpp 
> 44ca6d0869f3dbcfda1ac01d0d6b79dc20c4267c 
> 
> Diff: https://reviews.apache.org/r/43849/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 41950: Cleaned up hierarchical allocator tests.

[Bernd Mathiske]

> On Feb. 23, 2016, 9:33 a.m., Ben Mahler wrote:
> > Apologies, did you check with Alex prior to committing? We talked about 
> > this change recently but we didn't publish the comments, sorry that it 
> > wasn't clear! I reverted it for now, since we didn't really like the empty 
> > map constants as class members. We also needed to look into whether using 
> > empty initializer lists inline worked instead of this change.

Alex is on vacation and I found this lingering. Since it looks a lot like an 
improvement, I went ahead after reading it all and testing it. No problem, we 
can progress further from either version.


- Bernd


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


On Jan. 28, 2016, 5:12 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41950/
> ---
> 
> (Updated Jan. 28, 2016, 5:12 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, Joerg Schad, and Joris 
> Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes made:
> - empty resource map promoted to a const class field;
> - removed variable numeric suffixes where appropriate;
> - added const where appropriate.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> f18e6eb10572b0f5b8bbff338384d9406f6ad62b 
> 
> Diff: https://reviews.apache.org/r/41950/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> 
> `make check` 
> 
> `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh 
> --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 43799: Removed race condition from libevent based poll implementation.

[Bernd Mathiske]

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


Fix it, then Ship it!





3rdparty/libprocess/src/libevent_poll.cpp (line 56)
<https://reviews.apache.org/r/43799/#comment181733>

According to the style guide, we are usually using trailing underscores 
here.



3rdparty/libprocess/src/libevent_poll.cpp (line 57)
<https://reviews.apache.org/r/43799/#comment181734>

Although this should work as is, it is more straight forward and the common 
(generally safer) pattern to access the shared pointer here, not the weak one. 

  _ev.get() 
  
  (respectively ev_.get(), see above)


- Bernd Mathiske


On Feb. 19, 2016, 5:09 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43799/
> ---
> 
> (Updated Feb. 19, 2016, 5:09 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-3271 and MESOS-4711
> https://issues.apache.org/jira/browse/MESOS-3271
> https://issues.apache.org/jira/browse/MESOS-4711
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Under certains circumstances, the future returned by poll is discarded right
> after the event is triggered, this causes the event callback to be called
> before the discard callback which results in an abort signal being raised
> by the libevent library.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_poll.cpp 
> 461624ca003e97be5ea9cf956d86fc294e6f1bc1 
> 
> Diff: https://reviews.apache.org/r/43799/diff/
> 
> 
> Testing
> ---
> 
> ```bash
> # On CentOS 6.7 running in virtualbox
> ../configure --enable-ssl --enable-libevent
> make -j4 check
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery" 
> --gtest_repeat=1000
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 43367: Speed up FetcherCache test cases by reduce allocation_interval.

[Bernd Mathiske]

> On Feb. 19, 2016, 1:02 a.m., Alexander Rukletsov wrote:
> > src/tests/fetcher_cache_tests.cpp, lines 189-191
> > <https://reviews.apache.org/r/43367/diff/12/?file=1254386#file1254386line189>
> >
> > Will it be more clear to explicitly advance clock in those tests? I 
> > think keeping changes local, i.e. making them only there where they are 
> > necessary, serves educational purposes. People often read tests to 
> > understand how things work, hence it's valuable not to distract them with 
> > irrelevant actions, nor to hide meaningful steps.
> 
> haosdent huang wrote:
> Do you mean move
> 
> ```
>   Future<vector> offers;
>   EXPECT_CALL(scheduler, resourceOffers(driver, _))
> .WillOnce(FutureArg<1>())
> .WillRepeatedly(DeclineOffers());
> 
>   // The default timeout in AWAIT_READY is 15 seconds,
>   // so we use that amount here.
>   // TODO(bernd-mesos): Make this a symbolic constant in "gtest.hpp"
>   // that we can reference here.
>   offers.await(Seconds(15));
>   if (!offers.isReady()) {
> return Error("Failed to wait for resource offers: " +
>(offers.isFailed() ? offers.failure() : "discarded"));
>   }
> 
>   CHECK_NE(0u, offers.get().size());
>   const Offer offer = offers.get()[0];
> ```
> 
> out and use advance clock according to different test cases?
> 
> Or pass a new param
> ```
> Try FetcherCacheTest::launchTask(
> const CommandInfo& commandInfo,
> const size_t taskIndex,
> const bool& advanceAllocation)
> ```

This change applies to all these tests, so one global change instead of many 
local ones seems OK to me. Dropping the issue.


- Bernd


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


On Feb. 18, 2016, 6:54 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43367/
> ---
> 
> (Updated Feb. 18, 2016, 6:54 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, and Bernd 
> Mathiske.
> 
> 
> Bugs: MESOS-4628
> https://issues.apache.org/jira/browse/MESOS-4628
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up FetcherCache test cases by reduce allocation_interval.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp f9c48f5d938c2601cb8f826029d6969d676ab98e 
> 
> Diff: https://reviews.apache.org/r/43367/diff/
> 
> 
> Testing
> ---
> 
> In this patch, we use 500ms instead of the default allocation_interval 1s. If 
> the test cases depends on getting offers from master multiple times, this 
> patch could reduce the offer waiting time and speed up them. 
> 
> Before
> ```
> [   OK ] FetcherCacheTest.LocalUncached (943 ms)
> [   OK ] FetcherCacheTest.LocalCached (1612 ms)
> [   OK ] FetcherCacheTest.CachedFallback (906 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (940 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (1719 ms)
> [   OK ] FetcherCacheTest.SimpleEviction (3635 ms)
> [   OK ] FetcherCacheTest.FallbackFromEviction (2518 ms)
> [   OK ] FetcherCacheTest.RemoveLRUCacheEntries (3653 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedSerialized (2632 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedConcurrent (1227 ms)
> [   OK ] FetcherCacheHttpTest.HttpMixed (1129 ms)
> 
> ```
> 
> After
> ```
> [   OK ] FetcherCacheTest.LocalUncached (979 ms)
> [   OK ] FetcherCacheTest.LocalCached (1510 ms)
> [   OK ] FetcherCacheTest.CachedFallback (853 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (816 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (1715 ms)
> [   OK ] FetcherCacheTest.SimpleEviction (2720 ms)
> [   OK ] FetcherCacheTest.FallbackFromEviction (2119 ms)
> [   OK ] FetcherCacheTest.RemoveLRUCacheEntries (2847 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedSerialized (2233 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedConcurrent (1211 ms)
> [   OK ] FetcherCacheHttpTest.HttpMixed (1106 ms)
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

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

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

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

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

See above.



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

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



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

How would a pointer manage anything?



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

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

s/lambda/extra/



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

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 43271: Modify subprocess to deal with LIBPROCESS_PORT specially.

[Bernd Mathiske]

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




3rdparty/libprocess/include/process/subprocess.hpp (line 262)
<https://reviews.apache.org/r/43271/#comment181237>

This can be a static function that does not show up in the header at all.



3rdparty/libprocess/include/process/subprocess.hpp (line 264)
<https://reviews.apache.org/r/43271/#comment181238>

s/method/function/



3rdparty/libprocess/include/process/subprocess.hpp (line 265)
<https://reviews.apache.org/r/43271/#comment181239>

s/passing//



3rdparty/libprocess/src/subprocess.cpp (line 328)
<https://reviews.apache.org/r/43271/#comment181248>

There are so many things I'd like to comment on here, please allow me to 
just go for it and implement all the changes that I recommend, then compare. 

static map<string, string> subprocess_environment(
const Option<map<string, string>>& environment)
{
  // Check whether this environment comes with a conflicting
  // `LIBPROCESS_PORT` value. Warn if so.
  if (environment.isSome() && 
environment.get().count("LIBPROCESS_PORT") > 0) { 
Option port = os::getenv("LIBPROCESS_PORT");
if (port.isSome() && port.get() == 
environment.get()["LIBPROCESS_PORT"]) {
  LOG(WARNING)
<< "Subprocess's LIBPROCESS_PORT environment variable "
<< "conflicts with its parent process's LIBPROCESS_PORT; "
<< "dropping LIBPROCESS_PORT from subprocess's environment.";
}
  }

  map<string, string> result = environment.getOrElse(os::environment());

  // Remove the libprocess port variables so that the subprocess
  // will not attempt to bind to an already occupied port.
  result.erase("LIBPROCESS_PORT");
  result.erase("LIBPROCESS_ADVERTISE_PORT");

  // NOTE: We retain the IP variables ("LIBPROCESS_IP" and
  // "LIBPROCESS_ADVERTISE_IP") because, if DNS is not available in the
  // subprocess, the absence of the IP variables will cause an error
  // when libprocess attempts a hostname lookup.

  return result;
}



3rdparty/libprocess/src/subprocess.cpp (line 446)
<https://reviews.apache.org/r/43271/#comment181249>

You can use 'subprocess_environment(environment)' here without the temp 
var, avoiding the ugly underscore naming. 

If you ewant to keep it, though, the underscore goes to the end: 
"environment_". And then you should use const ref.


- Bernd Mathiske


On Feb. 18, 2016, 11:22 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43271/
> ---
> 
> (Updated Feb. 18, 2016, 11:22 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4609
> https://issues.apache.org/jira/browse/MESOS-4609
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> * Adds a helper method for getting the current environment plus 
> considerations for libprocess.
> * Changes the default behavior of `process::subprocess` to use the above 
> helper when given `environment = None()`.
> * Adds a warning inside `process::subprocess` if `LIBPROCESS_PORT` conflicts 
> with the current process's `LIBPROCESS_PORT`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess.hpp 
> e0c306aa5cf5f393abb73768bbd287c45730f076 
>   3rdparty/libprocess/src/subprocess.cpp 
> 44ca6d0869f3dbcfda1ac01d0d6b79dc20c4267c 
> 
> Diff: https://reviews.apache.org/r/43271/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> Tests are run in the next review.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43737: Fixed typo in fetcher docs.

[Bernd Mathiske]

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


Ship it!




Thanks!

- Bernd Mathiske


On Feb. 18, 2016, 1:17 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43737/
> ---
> 
> (Updated Feb. 18, 2016, 1:17 p.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed typo in fetcher docs.
> 
> 
> Diffs
> -
> 
>   docs/fetcher.md 30f9d0f7f622db7ac960c5bf255319c2553b94ee 
> 
> Diff: https://reviews.apache.org/r/43737/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43271: Modify subprocess to deal with LIBPROCESS_PORT specially.

[Bernd Mathiske]

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




3rdparty/libprocess/src/subprocess.cpp (line 303)
<https://reviews.apache.org/r/43271/#comment180969>

I know which vars you mean here, but other readers won't know. I suggest to 
name them explicitly.



3rdparty/libprocess/src/subprocess.cpp (line 382)
<https://reviews.apache.org/r/43271/#comment180971>

Breaking out subprocess_environment but not the code below as a subroutine 
looks unbalanced. Why not factor the whole affair into a common subroutine 
together?

This routine may look like a filter function: env -> env.


- Bernd Mathiske


On Feb. 5, 2016, 3:25 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43271/
> ---
> 
> (Updated Feb. 5, 2016, 3:25 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4609
> https://issues.apache.org/jira/browse/MESOS-4609
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> * Adds a helper method for getting the current environment plus 
> considerations for libprocess.
> * Changes the default behavior of `process::subprocess` to use the above 
> helper when given `environment = None()`.
> * Adds a warning inside `process::subprocess` if `LIBPROCESS_PORT` conflicts 
> with the current process's `LIBPROCESS_PORT`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess.hpp 
> bb50cc3070245a294fa16efe44f14ae893bc5518 
>   3rdparty/libprocess/src/subprocess.cpp 
> ff477e37a9619c780bddd5a8e629fa981b729715 
> 
> Diff: https://reviews.apache.org/r/43271/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> Tests are run in the next review.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 36071: Add flow diagram for docker containerizer.

[Bernd Mathiske]

> On July 7, 2015, 4:51 a.m., Bernd Mathiske wrote:
> > 1. Inconsistent capitalization in box labels.
> > 2. You explain different paths to obtain an "executor pid" and then you 
> > checkpoint a "container pid". You lost me there.
> > 3. What happens to the tasks? Is this diagram for the executor only? If so, 
> > it is incomplete.
> 
> Bernd Mathiske wrote:
> Is this still being worked on?

@tnachen: Is this still being worked on?


- Bernd


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


On July 6, 2015, 2:37 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36071/
> ---
> 
> (Updated July 6, 2015, 2:37 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add flow diagram for docker containerizer.
> 
> 
> Diffs
> -
> 
>   docs/docker-containerizer.md 73f897780a0bb72ab092cb08186a228e3084e798 
>   docs/images/docker_containerizer_flow.jpg PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36071/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> File Attachments
> 
> 
> docker_containerizer_flow.png
>   
> https://reviews.apache.org/media/uploaded/files/2015/07/06/d888a674-17d8-4faf-ab03-f1892537a6e5__docker_containerizer_flow.png
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 43367: Speed up FetcherCache test cases by reduce allocation_interval.

[Bernd Mathiske]

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


Fix it, then Ship it!





src/tests/fetcher_cache_tests.cpp (line 190)
<https://reviews.apache.org/r/43367/#comment180968>

s/taks works, settling/tasks works and settling/


- Bernd Mathiske


On Feb. 17, 2016, 8:55 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43367/
> ---
> 
> (Updated Feb. 17, 2016, 8:55 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, and Bernd 
> Mathiske.
> 
> 
> Bugs: MESOS-4628
> https://issues.apache.org/jira/browse/MESOS-4628
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up FetcherCache test cases by reduce allocation_interval.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp f9c48f5d938c2601cb8f826029d6969d676ab98e 
> 
> Diff: https://reviews.apache.org/r/43367/diff/
> 
> 
> Testing
> ---
> 
> In this patch, we use 500ms instead of the default allocation_interval 1s. If 
> the test cases depends on getting offers from master multiple times, this 
> patch could reduce the offer waiting time and speed up them. 
> 
> Before
> ```
> [   OK ] FetcherCacheTest.LocalUncached (943 ms)
> [   OK ] FetcherCacheTest.LocalCached (1612 ms)
> [   OK ] FetcherCacheTest.CachedFallback (906 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (940 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (1719 ms)
> [   OK ] FetcherCacheTest.SimpleEviction (3635 ms)
> [   OK ] FetcherCacheTest.FallbackFromEviction (2518 ms)
> [   OK ] FetcherCacheTest.RemoveLRUCacheEntries (3653 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedSerialized (2632 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedConcurrent (1227 ms)
> [   OK ] FetcherCacheHttpTest.HttpMixed (1129 ms)
> 
> ```
> 
> After
> ```
> [   OK ] FetcherCacheTest.LocalUncached (979 ms)
> [   OK ] FetcherCacheTest.LocalCached (1510 ms)
> [   OK ] FetcherCacheTest.CachedFallback (853 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (816 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (1715 ms)
> [   OK ] FetcherCacheTest.SimpleEviction (2720 ms)
> [   OK ] FetcherCacheTest.FallbackFromEviction (2119 ms)
> [   OK ] FetcherCacheTest.RemoveLRUCacheEntries (2847 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedSerialized (2233 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedConcurrent (1211 ms)
> [   OK ] FetcherCacheHttpTest.HttpMixed (1106 ms)
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 41865: Use full width for mesos div.container

[Bernd Mathiske]

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



@mlunoe Is this change still needed, since the ticket has been closed given 
your patch?

- Bernd Mathiske


On Jan. 3, 2016, 7:08 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41865/
> ---
> 
> (Updated Jan. 3, 2016, 7:08 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Michael Lunøe.
> 
> 
> Bugs: MESOS-2585
> https://issues.apache.org/jira/browse/MESOS-2585
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use full width for mesos div.container
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/css/mesos.css 
> 5b1227e9d64757f9fc106e497f7fa3ed72112c10 
> 
> Diff: https://reviews.apache.org/r/41865/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 43608: Speed up FetcherCache test cases by disable framework checkpoint.

[Bernd Mathiske]

> On Feb. 17, 2016, 7:21 a.m., Bernd Mathiske wrote:
> > Are a couple of seconds wirth the extra code complexity? Opinions, please!
> 
> haosdent huang wrote:
> Yes, @bernd, as you see here, this patch don't bring too much effects. 
> How about let's discard it and reopen it if really necessary?

Agreed! Thanks!


- Bernd


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


On Feb. 16, 2016, 6:10 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43608/
> ---
> 
> (Updated Feb. 16, 2016, 6:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, and Bernd 
> Mathiske.
> 
> 
> Bugs: MESOS-4685
> https://issues.apache.org/jira/browse/MESOS-4685
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up FetcherCache test cases by disable framework checkpoint.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp fc89be46bc4cca81f6f3f5072539d3dae26f7c8f 
> 
> Diff: https://reviews.apache.org/r/43608/diff/
> 
> 
> Testing
> ---
> 
> When we enable checkpoint for frameworks, StatusUpdateManager would waiting 
> for write StatusUpdateRecord to file and then boardcast it. This cause 
> uncessary disk operations and bring unstable delay if our test cases don't 
> depends on recovery.  
> 
> Before
> ```
> [   OK ] FetcherCacheTest.LocalUncached (2645 ms)
> [   OK ] FetcherCacheTest.LocalCached (2653 ms)
> [   OK ] FetcherCacheTest.CachedFallback (1110 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (2620 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (2758 ms)
> [   OK ] FetcherCacheTest.SimpleEviction (5059 ms)
> [   OK ] FetcherCacheTest.FallbackFromEviction (2648 ms)
> [   OK ] FetcherCacheTest.RemoveLRUCacheEntries (3661 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedSerialized (2600 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedConcurrent (2013 ms)
> [   OK ] FetcherCacheHttpTest.HttpMixed (1746 ms)
> ```
> 
> After
> ```
> [   OK ] FetcherCacheTest.LocalUncached (2580 ms)
> [   OK ] FetcherCacheTest.LocalCached (2516 ms)
> [   OK ] FetcherCacheTest.CachedFallback (957 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (2519 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (2594 ms)
> [   OK ] FetcherCacheTest.SimpleEviction (4532 ms)
> [   OK ] FetcherCacheTest.FallbackFromEviction (2579 ms)
> [   OK ] FetcherCacheTest.RemoveLRUCacheEntries (3528 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedSerialized (2520 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedConcurrent (1534 ms)
> [   OK ] FetcherCacheHttpTest.HttpMixed (1345 ms)
> ```
> 
> And test the recovery test manually:
> ```
> ./bin/mesos-tests.sh 
> --gtest_filter="FetcherCacheHttpRecoveryTest.HttpCachedRecovery"
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

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

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

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



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

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

Re: Review Request 43367: Speed up FetcherCache test cases by reduce allocation_interval.

[Bernd Mathiske]

> On Feb. 17, 2016, 2:09 a.m., Bernd Mathiske wrote:
> > src/tests/fetcher_cache_tests.cpp, line 197
> > <https://reviews.apache.org/r/43367/diff/9/?file=1252393#file1252393line197>
> >
> > Having run multiple test suite runs, I am not convinced this has much 
> > of an effect. I got results between 20 and 28 seconds for all FetcherCache 
> > tests combined no matter what allocation interval I used: default, 500, 
> > 100. If there is an effect, it is less than the variance.
> 
> haosdent huang wrote:
> Looks weird, let me double check this.
> 
> haosdent huang wrote:
> I use `./bin/mesos-tests.sh --gtest_filter="FetcherCache*" 
> --gtest_repeat=10 2>&1 |grep total` to test. Could find use 
> allocation_interval from 1s to 200ms bring effect as the allocation_interval 
> decrease.
> 
> Use 1 s
> ```
> [==] 11 tests from 2 test cases ran. (21652 ms total)
> [==] 11 tests from 2 test cases ran. (21278 ms total)
> [==] 11 tests from 2 test cases ran. (21077 ms total)
> [==] 11 tests from 2 test cases ran. (21447 ms total)
> [==] 11 tests from 2 test cases ran. (21594 ms total)
> [==] 11 tests from 2 test cases ran. (21364 ms total)
> [==] 11 tests from 2 test cases ran. (21085 ms total)
> [==] 11 tests from 2 test cases ran. (21773 ms total)
> [==] 11 tests from 2 test cases ran. (21737 ms total)
> [==] 11 tests from 2 test cases ran. (21075 ms total)
> ```
> 
> Use 500 ms
> ```
> [==] 11 tests from 2 test cases ran. (18028 ms total)
> [==] 11 tests from 2 test cases ran. (18438 ms total)
> [==] 11 tests from 2 test cases ran. (18636 ms total)
> [==] 11 tests from 2 test cases ran. (18470 ms total)
> [==] 11 tests from 2 test cases ran. (17913 ms total)
> [==] 11 tests from 2 test cases ran. (18842 ms total)
> [==] 11 tests from 2 test cases ran. (18785 ms total)
> [==] 11 tests from 2 test cases ran. (18312 ms total)
> [==] 11 tests from 2 test cases ran. (18216 ms total)
> [==] 11 tests from 2 test cases ran. (18827 ms total)
> ```
> 
> Use 200 ms
> ```
> [==] 11 tests from 2 test cases ran. (16630 ms total)
> [==] 11 tests from 2 test cases ran. (17993 ms total)
> [==] 11 tests from 2 test cases ran. (17282 ms total)
> [==] 11 tests from 2 test cases ran. (17588 ms total)
> [==] 11 tests from 2 test cases ran. (16502 ms total)
> [==] 11 tests from 2 test cases ran. (17218 ms total)
> [==] 11 tests from 2 test cases ran. (17744 ms total)
> [==] 11 tests from 2 test cases ran. (17136 ms total)
> [==] 11 tests from 2 test cases ran. (17178 ms total)
> [==] 11 tests from 2 test cases ran. (17613 ms total)
> ```
> 
> Use 100 ms
> ```
> [==] 11 tests from 2 test cases ran. (16305 ms total)
> [==] 11 tests from 2 test cases ran. (16093 ms total)
> [==] 11 tests from 2 test cases ran. (16345 ms total)
> [==] 11 tests from 2 test cases ran. (16898 ms total)
> [==] 11 tests from 2 test cases ran. (16517 ms total)
> [==] 11 tests from 2 test cases ran. (16005 ms total)
> [==] 11 tests from 2 test cases ran. (21150 ms total)
> [==] 11 tests from 2 test cases ran. (16650 ms total)
> [==] 11 tests from 2 test cases ran. (16287 ms total)
> [==] 11 tests from 2 test cases ran. (16617 ms total)
> ```
> 
> haosdent huang wrote:
> 400 ms
> ```
> [==] 11 tests from 2 test cases ran. (17678 ms total)
> [==] 11 tests from 2 test cases ran. (17093 ms total)
> [==] 11 tests from 2 test cases ran. (17174 ms total)
> [==] 11 tests from 2 test cases ran. (17396 ms total)
> [==] 11 tests from 2 test cases ran. (18095 ms total)
> [==] 11 tests from 2 test cases ran. (17404 ms total)
> [==] 11 tests from 2 test cases ran. (17480 ms total)
> [==] 11 tests from 2 test cases ran. (17110 ms total)
> [==] 11 tests from 2 test cases ran. (17352 ms total)
> [==] 11 tests from 2 test cases ran. (17219 ms total)
> ```
> 
> 300 ms
> ```
> [==] 11 tests from 2 test cases ran. (17165 ms total)
> [==] 11 tests from 2 test cases ran. (18067 ms total)
> [==] 11 tests from 2 test cases ran. (17951 ms to

Re: Review Request 43608: Speed up FetcherCache test cases by disable framework checkpoint.

[Bernd Mathiske]

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



Are a couple of seconds wirth the extra code complexity? Opinions, please!

- Bernd Mathiske


On Feb. 16, 2016, 6:10 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43608/
> ---
> 
> (Updated Feb. 16, 2016, 6:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, and Bernd 
> Mathiske.
> 
> 
> Bugs: MESOS-4685
> https://issues.apache.org/jira/browse/MESOS-4685
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up FetcherCache test cases by disable framework checkpoint.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp fc89be46bc4cca81f6f3f5072539d3dae26f7c8f 
> 
> Diff: https://reviews.apache.org/r/43608/diff/
> 
> 
> Testing
> ---
> 
> When we enable checkpoint for frameworks, StatusUpdateManager would waiting 
> for write StatusUpdateRecord to file and then boardcast it. This cause 
> uncessary disk operations and bring unstable delay if our test cases don't 
> depends on recovery.  
> 
> Before
> ```
> [   OK ] FetcherCacheTest.LocalUncached (2645 ms)
> [   OK ] FetcherCacheTest.LocalCached (2653 ms)
> [   OK ] FetcherCacheTest.CachedFallback (1110 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (2620 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (2758 ms)
> [   OK ] FetcherCacheTest.SimpleEviction (5059 ms)
> [   OK ] FetcherCacheTest.FallbackFromEviction (2648 ms)
> [   OK ] FetcherCacheTest.RemoveLRUCacheEntries (3661 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedSerialized (2600 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedConcurrent (2013 ms)
> [   OK ] FetcherCacheHttpTest.HttpMixed (1746 ms)
> ```
> 
> After
> ```
> [   OK ] FetcherCacheTest.LocalUncached (2580 ms)
> [   OK ] FetcherCacheTest.LocalCached (2516 ms)
> [   OK ] FetcherCacheTest.CachedFallback (957 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (2519 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (2594 ms)
> [   OK ] FetcherCacheTest.SimpleEviction (4532 ms)
> [   OK ] FetcherCacheTest.FallbackFromEviction (2579 ms)
> [   OK ] FetcherCacheTest.RemoveLRUCacheEntries (3528 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedSerialized (2520 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedConcurrent (1534 ms)
> [   OK ] FetcherCacheHttpTest.HttpMixed (1345 ms)
> ```
> 
> And test the recovery test manually:
> ```
> ./bin/mesos-tests.sh 
> --gtest_filter="FetcherCacheHttpRecoveryTest.HttpCachedRecovery"
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 42842: Fixed flakiness in ContainerLoggerTest.DefaultToSandbox.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On Feb. 10, 2016, 9:49 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42842/
> ---
> 
> (Updated Feb. 10, 2016, 9:49 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Jian 
> Qiu.
> 
> 
> Bugs: MESOS-4615
> https://issues.apache.org/jira/browse/MESOS-4615
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test needs to wait for the task to finish before examining the resulting 
> files.
> 
> 
> Diffs
> -
> 
>   src/tests/container_logger_tests.cpp 
> 5fe9cce97ee83d6a9e272879ec0395b5ace4a491 
> 
> Diff: https://reviews.apache.org/r/42842/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43367: Speed up FetcherCache test cases by reduce allocation_interval.

[Bernd Mathiske]

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




src/tests/fetcher_cache_tests.cpp (line 197)
<https://reviews.apache.org/r/43367/#comment180808>

Having run multiple test suite runs, I am not convinced this has much of an 
effect. I got results between 20 and 28 seconds for all FetcherCache tests 
combined no matter what allocation interval I used: default, 500, 100. If there 
is an effect, it is less than the variance.


- Bernd Mathiske


On Feb. 17, 2016, 1:54 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43367/
> ---
> 
> (Updated Feb. 17, 2016, 1:54 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, and Bernd 
> Mathiske.
> 
> 
> Bugs: MESOS-4628
> https://issues.apache.org/jira/browse/MESOS-4628
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up FetcherCache test cases by reduce allocation_interval.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp fc89be46bc4cca81f6f3f5072539d3dae26f7c8f 
> 
> Diff: https://reviews.apache.org/r/43367/diff/
> 
> 
> Testing
> ---
> 
> In this patch, we use 500ms instead of the default allocation_interval 1s. If 
> the test cases depends on getting offers from master multiple times, this 
> patch could reduce the offer waiting time and speed up them. 
> 
> Before
> ```
> [   OK ] FetcherCacheTest.LocalUncached (943 ms)
> [   OK ] FetcherCacheTest.LocalCached (1612 ms)
> [   OK ] FetcherCacheTest.CachedFallback (906 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (940 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (1719 ms)
> [   OK ] FetcherCacheTest.SimpleEviction (3635 ms)
> [   OK ] FetcherCacheTest.FallbackFromEviction (2518 ms)
> [   OK ] FetcherCacheTest.RemoveLRUCacheEntries (3653 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedSerialized (2632 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedConcurrent (1227 ms)
> [   OK ] FetcherCacheHttpTest.HttpMixed (1129 ms)
> 
> ```
> 
> After
> ```
> [   OK ] FetcherCacheTest.LocalUncached (979 ms)
> [   OK ] FetcherCacheTest.LocalCached (1510 ms)
> [   OK ] FetcherCacheTest.CachedFallback (853 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (816 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (1715 ms)
> [   OK ] FetcherCacheTest.SimpleEviction (2720 ms)
> [   OK ] FetcherCacheTest.FallbackFromEviction (2119 ms)
> [   OK ] FetcherCacheTest.RemoveLRUCacheEntries (2847 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedSerialized (2233 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedConcurrent (1211 ms)
> [   OK ] FetcherCacheHttpTest.HttpMixed (1106 ms)
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 42696: Speed up FetcherCacheTest.Local* test by reduce loop.

[Bernd Mathiske]

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


Ship it!




Now without the checkpointing-related code which went into a separate review, 
let's ship it!

- Bernd Mathiske


On Feb. 16, 2016, 6:10 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42696/
> ---
> 
> (Updated Feb. 16, 2016, 6:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, and Bernd 
> Mathiske.
> 
> 
> Bugs: MESOS-4486
> https://issues.apache.org/jira/browse/MESOS-4486
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up FetcherCacheTest.Local* test by reduce loop.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp fc89be46bc4cca81f6f3f5072539d3dae26f7c8f 
> 
> Diff: https://reviews.apache.org/r/42696/diff/
> 
> 
> Testing
> ---
> 
> Before
> ```
> [   OK ] FetcherCacheTest.LocalUncached (2580 ms)
> [   OK ] FetcherCacheTest.LocalCached (2516 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (2519 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (2594 ms)
> ```
> 
> After
> ```
> [   OK ] FetcherCacheTest.LocalUncached (977 ms)
> [   OK ] FetcherCacheTest.LocalCached (1731 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (975 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (1713 ms)
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 43367: Speed up FetcherCache test cases by reduce allocation_interval.

[Bernd Mathiske]

> On Feb. 16, 2016, 3:02 a.m., Bernd Mathiske wrote:
> > src/tests/fetcher_cache_tests.cpp, line 422
> > <https://reviews.apache.org/r/43367/diff/4/?file=1239305#file1239305line422>
> >
> > In some fetcher cache tests, we specifically want to find out if 
> > concurrent execution of launching taks works. Settling the clock serializes 
> > this and thus disables some of desired the testing.
> > 
> > How about shortening the allocation interval and not playing with the 
> > clock?
> 
> haosdent huang wrote:
> Got it, how about change to 500ms?

Worth a try :-)


- Bernd


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


On Feb. 16, 2016, 3:42 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43367/
> -------
> 
> (Updated Feb. 16, 2016, 3:42 a.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Bugs: MESOS-4628
> https://issues.apache.org/jira/browse/MESOS-4628
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up FetcherCache test cases by advance allocation_interval.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 1cf45660691860793ac600363f7934e13a2e7ddf 
> 
> Diff: https://reviews.apache.org/r/43367/diff/
> 
> 
> Testing
> ---
> 
> Before this patch:
> ```
> [   OK ] FetcherCacheTest.LocalUncached (1065 ms)
> [   OK ] FetcherCacheTest.LocalCached (1728 ms)
> [   OK ] FetcherCacheTest.CachedFallback (1105 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (906 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (1715 ms)
> [   OK ] FetcherCacheTest.SimpleEviction (3626 ms)
> [   OK ] FetcherCacheTest.FallbackFromEviction (2649 ms)
> [   OK ] FetcherCacheTest.RemoveLRUCacheEntries (3639 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedSerialized (2608 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedConcurrent (1529 ms)
> [   OK ] FetcherCacheHttpTest.HttpMixed (1349 ms)
> ```
> 
> After this patch:
> ```
> [   OK ] FetcherCacheTest.LocalUncached (984 ms)
> [   OK ] FetcherCacheTest.LocalCached (1750 ms)
> [   OK ] FetcherCacheTest.CachedFallback (1043 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (1006 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (1714 ms)
> [   OK ] FetcherCacheTest.SimpleEviction (2723 ms)
> [   OK ] FetcherCacheTest.FallbackFromEviction (2220 ms)
> [   OK ] FetcherCacheTest.RemoveLRUCacheEntries (2736 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedSerialized (2251 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedConcurrent (1514 ms)
> [   OK ] FetcherCacheHttpTest.HttpMixed (1209 ms)
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 42696: Speed up FetcherCacheTest.Local* test cases.

[Bernd Mathiske]

> On Feb. 16, 2016, 3:02 a.m., Alexander Rukletsov wrote:
> > Could you please explain in description why disabling checkpointing speeds 
> > up the test and why it is ok to disable it for these tests?
> 
> haosdent huang wrote:
> Sure!

Not writing to disk costs less time.

If a test does not attempt recovery, then whatever would be checkpointed never 
gets used.

Only one test attempts/tests recovery so far. In this test we do apply 
checkpointing. So it does get tested, but we don't need to test the writing 
part of it in every other test.


- Bernd


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


On Feb. 9, 2016, 4:48 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42696/
> ---
> 
> (Updated Feb. 9, 2016, 4:48 p.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Bugs: MESOS-4486
> https://issues.apache.org/jira/browse/MESOS-4486
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up FetcherCacheTest.Local* test cases.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 1cf45660691860793ac600363f7934e13a2e7ddf 
> 
> Diff: https://reviews.apache.org/r/42696/diff/
> 
> 
> Testing
> ---
> 
> Before apply the patch:
> ```
> $ sudo ./bin/mesos-tests.sh --gtest_filter="FetcherCacheTest.Local*"
> [   OK ] FetcherCacheTest.LocalUncached (2580 ms)
> [   OK ] FetcherCacheTest.LocalCached (2524 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (2514 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (2551 ms)
> ```
> 
> 
> After apply the patch:
> ```
> $ sudo ./bin/mesos-tests.sh --gtest_filter="FetcherCacheTest.Local*"
> [   OK ] FetcherCacheTest.LocalUncached (873 ms)
> [   OK ] FetcherCacheTest.LocalCached (1609 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (926 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (1509 ms)
> ```
> 
> Also test the recovery test manually:
> 
> ```
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="FetcherCacheHttpRecoveryTest.HttpCachedRecovery"
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 43367: Speed up FetcherCache test cases by advance allocation_interval.

[Bernd Mathiske]

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




src/tests/fetcher_cache_tests.cpp (line 422)
<https://reviews.apache.org/r/43367/#comment180624>

In some fetcher cache tests, we specifically want to find out if concurrent 
execution of launching taks works. Settling the clock serializes this and thus 
disables some of desired the testing.

How about shortening the allocation interval and not playing with the clock?


- Bernd Mathiske


On Feb. 9, 2016, 8:17 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43367/
> ---
> 
> (Updated Feb. 9, 2016, 8:17 p.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Bugs: MESOS-4618
> https://issues.apache.org/jira/browse/MESOS-4618
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up FetcherCache test cases by advance allocation_interval.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 1cf45660691860793ac600363f7934e13a2e7ddf 
> 
> Diff: https://reviews.apache.org/r/43367/diff/
> 
> 
> Testing
> ---
> 
> Before this patch:
> ```
> [   OK ] FetcherCacheTest.LocalUncached (1065 ms)
> [   OK ] FetcherCacheTest.LocalCached (1728 ms)
> [   OK ] FetcherCacheTest.CachedFallback (1105 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (906 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (1715 ms)
> [   OK ] FetcherCacheTest.SimpleEviction (3626 ms)
> [   OK ] FetcherCacheTest.FallbackFromEviction (2649 ms)
> [   OK ] FetcherCacheTest.RemoveLRUCacheEntries (3639 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedSerialized (2608 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedConcurrent (1529 ms)
> [   OK ] FetcherCacheHttpTest.HttpMixed (1349 ms)
> ```
> 
> After this patch:
> ```
> [   OK ] FetcherCacheTest.LocalUncached (1132 ms)
> [   OK ] FetcherCacheTest.LocalCached (1693 ms)
> [   OK ] FetcherCacheTest.CachedFallback (1028 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (998 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (1341 ms)
> [   OK ] FetcherCacheTest.SimpleEviction (2056 ms)
> [   OK ] FetcherCacheTest.FallbackFromEviction (2146 ms)
> [   OK ] FetcherCacheTest.RemoveLRUCacheEntries (2576 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedSerialized (1588 ms)
> [   OK ] FetcherCacheHttpTest.HttpCachedConcurrent (1453 ms)
> [   OK ] FetcherCacheHttpTest.HttpMixed (1143 ms)
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 42696: Speed up FetcherCacheTest.Local* test cases.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On Feb. 9, 2016, 4:48 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42696/
> ---
> 
> (Updated Feb. 9, 2016, 4:48 p.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Bugs: MESOS-4486
> https://issues.apache.org/jira/browse/MESOS-4486
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up FetcherCacheTest.Local* test cases.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 1cf45660691860793ac600363f7934e13a2e7ddf 
> 
> Diff: https://reviews.apache.org/r/42696/diff/
> 
> 
> Testing
> ---
> 
> Before apply the patch:
> ```
> $ sudo ./bin/mesos-tests.sh --gtest_filter="FetcherCacheTest.Local*"
> [   OK ] FetcherCacheTest.LocalUncached (2580 ms)
> [   OK ] FetcherCacheTest.LocalCached (2524 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (2514 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (2551 ms)
> ```
> 
> 
> After apply the patch:
> ```
> $ sudo ./bin/mesos-tests.sh --gtest_filter="FetcherCacheTest.Local*"
> [   OK ] FetcherCacheTest.LocalUncached (873 ms)
> [   OK ] FetcherCacheTest.LocalCached (1609 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (926 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (1509 ms)
> ```
> 
> Also test the recovery test manually:
> 
> ```
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="FetcherCacheHttpRecoveryTest.HttpCachedRecovery"
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 43491: Added note about implicit default filter to javadoc.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On Feb. 13, 2016, 6:38 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43491/
> ---
> 
> (Updated Feb. 13, 2016, 6:38 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Bernd Mathiske, and Joerg Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make framework developers aware that the MesosSchedulerDriver will add an 
> implicit filter declining unused resources.
> 
> 
> Diffs
> -
> 
>   src/java/src/org/apache/mesos/SchedulerDriver.java 
> efe71b03eb45b295df0e5c250b753a766ed7688c 
> 
> Diff: https://reviews.apache.org/r/43491/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 43525: Fixed an unsigned<->signed comparison.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On Feb. 12, 2016, 3:47 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43525/
> ---
> 
> (Updated Feb. 12, 2016, 3:47 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/resources_tests.cpp a0e674db61e95d582c860e097efe4d85f208e1ea 
> 
> Diff: https://reviews.apache.org/r/43525/diff/
> 
> 
> Testing
> ---
> 
> make check (with GCC 5.3.1)
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Review Request 43359: Fixed the build with gcc 5.2.

[Bernd Mathiske]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

(An unsigned<->signed comparison was failing.)


Diffs
-

  src/tests/containerizer/isolator_tests.cpp 
c20f33ebd6b0b1703c513c0d4bfe4f7d3a9201c8 

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


Testing
---


Thanks,

Bernd Mathiske

Re: Review Request 43359: Fixed the build with gcc 5.2.

[Bernd Mathiske]

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

(Updated Feb. 9, 2016, 5:45 a.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Added testing info.


Repository: mesos


Description (updated)
---

An unsigned<->signed comparison was failing.


Diffs
-

  src/tests/containerizer/isolator_tests.cpp 
c20f33ebd6b0b1703c513c0d4bfe4f7d3a9201c8 

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


Testing (updated)
---

Checked that the build completes "make check" with gcc 5.2.1.


Thanks,

Bernd Mathiske

Re: Review Request 43316: Decrease countCacheEntries in FetcherCacheTest.SimpleEviction .

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On Feb. 7, 2016, 10:38 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43316/
> ---
> 
> (Updated Feb. 7, 2016, 10:38 a.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Bugs: MESOS-4618
> https://issues.apache.org/jira/browse/MESOS-4618
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Decrease countCacheEntries in FetcherCacheTest.SimpleEviction .
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 1cf45660691860793ac600363f7934e13a2e7ddf 
> 
> Diff: https://reviews.apache.org/r/43316/diff/
> 
> 
> Testing
> ---
> 
> Before
> 
> ```
> [   OK ] FetcherCacheTest.SimpleEviction (4516 ms)
> [--] 1 test from FetcherCacheTest (4516 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (4527 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> After
> 
> ```
> [   OK ] FetcherCacheTest.SimpleEviction (3602 ms)
> [--] 1 test from FetcherCacheTest (3602 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (3612 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 40731: Added a test case for floating point precision of resource allocation.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On Jan. 28, 2016, 4:45 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40731/
> ---
> 
> (Updated Jan. 28, 2016, 4:45 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Klaus Ma, Mandeep Chadha, and Neil 
> Conway.
> 
> 
> Bugs: MESOS-3552
> https://issues.apache.org/jira/browse/MESOS-3552
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test case for floating point precision of resource allocation.  This 
> patch is based on the commit submitted by : Mandeep (@mchadha) 
> https://reviews.apache.org/r/39056/
> 
> 
> Diffs
> -
> 
>   src/tests/reservation_tests.cpp d0c88560a5618ebec39fc4e0539ddb2d9ca554a2 
> 
> Diff: https://reviews.apache.org/r/40731/diff/
> 
> 
> Testing
> ---
> 
> Ran make check after adding Mandeep's test case to the GTEST framework.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 43072: Use full screen width for Mesos UI using Bootstraps (v3.3.6) container-fluid

[Bernd Mathiske]

> On Feb. 1, 2016, 6:45 p.m., Mesos ReviewBot wrote:
> > Bad patch!
> > 
> > Reviews applied: [43072]
> > 
> > Failed command: ./support/apply-review.sh -n -r 43072
> > 
> > Error:
> > 2016-02-02 02:45:28 URL:https://reviews.apache.org/r/43072/diff/raw/ 
> > [226634/226634] -> "43072.patch" [1]
> > Traceback (most recent call last):
> >   File "support/apply-reviews.py", line 342, in 
> > reviewboard()
> >   File "support/apply-reviews.py", line 321, in reviewboard
> > apply_review()
> >   File "support/apply-reviews.py", line 139, in apply_review
> > commit_patch()
> >   File "support/apply-reviews.py", line 180, in commit_patch
> > data = patch_data()
> >   File "support/apply-reviews.py", line 199, in patch_data
> > return reviewboard_data()
> >   File "support/apply-reviews.py", line 252, in reviewboard_data
> > email=user.get('email'))
> > UnicodeEncodeError: 'ascii' codec can't encode character u'\xf8' in 
> > position 11: ordinal not in range(128)
> > 
> > Full log: https://builds.apache.org/job/mesos-reviewbot/11187/console
> 
> haosdent huang wrote:
> I think we could ignore this error message.

I'd rather not ignore the error message. How can there be an out-of-range char 
in HTML and CSS files?


- Bernd


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


On Feb. 1, 2016, 3:36 p.m., Michael Lunøe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43072/
> ---
> 
> (Updated Feb. 1, 2016, 3:36 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Thomas Rampelberg.
> 
> 
> Bugs: MESOS-2585
> https://issues.apache.org/jira/browse/MESOS-2585
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The solution to our problem is solved by updating the Bootstrap CSS file, as 
> container-fluid was introduced later in Bootstrap.
> 
> I have updated to lastest stable release of Bootstrap v3.3.6. Looking through 
> the change log, the only thing that we are using in the Mesos UI and changed 
> is the "hide" class (deprecated), so instances of these have been replaced by 
> its successor ".hidden" class.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/browse.html 6b18056 
>   src/webui/master/static/css/bootstrap-3.0.3.min.css c547283 
>   src/webui/master/static/css/bootstrap-3.3.6.min.css PRE-CREATION 
>   src/webui/master/static/framework.html 9b28820 
>   src/webui/master/static/home.html d6cde1e 
>   src/webui/master/static/index.html 25caf53 
>   src/webui/master/static/slave.html bc46885 
>   src/webui/master/static/slave_executor.html 9d582d5 
>   src/webui/master/static/slave_framework.html 96d788f 
> 
> Diff: https://reviews.apache.org/r/43072/diff/
> 
> 
> Testing
> ---
> 
> - Mobile: http://cl.ly/2R2O0m1a1G3Q/Image%202016-02-01%20at%2015.21.53.png
> - 1680 x 1050px screen: 
> http://cl.ly/0K1o112u3L2I/Image%202016-02-01%20at%2015.23.37.png
> - 2880 x 1800px screen: 
> http://cl.ly/1M0L3M2X0J2i/Image%202016-02-01%20at%2015.22.50.png
> 
> Visual test in latest Chrome, Firefox, Safari and IE. No functional changes 
> introduced.
> 
> 
> Thanks,
> 
> Michael Lunøe
> 
>

Re: Review Request 43072: Use full screen width for Mesos UI using Bootstraps (v3.3.6) container-fluid

[Bernd Mathiske]

> On Feb. 1, 2016, 6:45 p.m., Mesos ReviewBot wrote:
> > Bad patch!
> > 
> > Reviews applied: [43072]
> > 
> > Failed command: ./support/apply-review.sh -n -r 43072
> > 
> > Error:
> > 2016-02-02 02:45:28 URL:https://reviews.apache.org/r/43072/diff/raw/ 
> > [226634/226634] -> "43072.patch" [1]
> > Traceback (most recent call last):
> >   File "support/apply-reviews.py", line 342, in 
> > reviewboard()
> >   File "support/apply-reviews.py", line 321, in reviewboard
> > apply_review()
> >   File "support/apply-reviews.py", line 139, in apply_review
> > commit_patch()
> >   File "support/apply-reviews.py", line 180, in commit_patch
> > data = patch_data()
> >   File "support/apply-reviews.py", line 199, in patch_data
> > return reviewboard_data()
> >   File "support/apply-reviews.py", line 252, in reviewboard_data
> > email=user.get('email'))
> > UnicodeEncodeError: 'ascii' codec can't encode character u'\xf8' in 
> > position 11: ordinal not in range(128)
> > 
> > Full log: https://builds.apache.org/job/mesos-reviewbot/11187/console
> 
> haosdent huang wrote:
> I think we could ignore this error message.
> 
> Bernd Mathiske wrote:
> I'd rather not ignore the error message. How can there be an out-of-range 
> char in HTML and CSS files?
> 
> haosdent huang wrote:
> I afraid this may caused by boostrap have some special characters like 
> `ok:before{content:"\e013"}`.

This should be an encoding using printable cahrs (<=128) only.


- Bernd


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


On Feb. 1, 2016, 3:36 p.m., Michael Lunøe wrote:
> 
> -------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43072/
> ---
> 
> (Updated Feb. 1, 2016, 3:36 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Thomas Rampelberg.
> 
> 
> Bugs: MESOS-2585
> https://issues.apache.org/jira/browse/MESOS-2585
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The solution to our problem is solved by updating the Bootstrap CSS file, as 
> container-fluid was introduced later in Bootstrap.
> 
> I have updated to lastest stable release of Bootstrap v3.3.6. Looking through 
> the change log, the only thing that we are using in the Mesos UI and changed 
> is the "hide" class (deprecated), so instances of these have been replaced by 
> its successor ".hidden" class.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/browse.html 6b18056 
>   src/webui/master/static/css/bootstrap-3.0.3.min.css c547283 
>   src/webui/master/static/css/bootstrap-3.3.6.min.css PRE-CREATION 
>   src/webui/master/static/framework.html 9b28820 
>   src/webui/master/static/home.html d6cde1e 
>   src/webui/master/static/index.html 25caf53 
>   src/webui/master/static/slave.html bc46885 
>   src/webui/master/static/slave_executor.html 9d582d5 
>   src/webui/master/static/slave_framework.html 96d788f 
> 
> Diff: https://reviews.apache.org/r/43072/diff/
> 
> 
> Testing
> ---
> 
> - Mobile: http://cl.ly/2R2O0m1a1G3Q/Image%202016-02-01%20at%2015.21.53.png
> - 1680 x 1050px screen: 
> http://cl.ly/0K1o112u3L2I/Image%202016-02-01%20at%2015.23.37.png
> - 2880 x 1800px screen: 
> http://cl.ly/1M0L3M2X0J2i/Image%202016-02-01%20at%2015.22.50.png
> 
> Visual test in latest Chrome, Firefox, Safari and IE. No functional changes 
> introduced.
> 
> 
> Thanks,
> 
> Michael Lunøe
> 
>

Review Request 42914: Listed supported file extensions in fetcher documentation.

[Bernd Mathiske]

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

Review request for mesos, Jan Schlicht and Till Toenshoff.


Bugs: MESOS-4336
https://issues.apache.org/jira/browse/MESOS-4336


Repository: mesos


Description
---

Edited fetcher.md, listing the supported file extensions when extracting an 
archive.


Diffs
-

  docs/fetcher.md cb4f3c33fb67f7ac65dbe7ebe1347d2599c43f37 

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


Testing
---

Opened file with MD renderer.


Thanks,

Bernd Mathiske

Re: Review Request 42914: Listed supported file extensions in fetcher documentation.

[Bernd Mathiske]

> On Jan. 28, 2016, 11:40 p.m., Timothy Chen wrote:
> > docs/fetcher.md, line 119
> > <https://reviews.apache.org/r/42914/diff/2/?file=1225096#file1225096line119>
> >
> > then files with a recognized extension
> 
> Bernd Mathiske wrote:
> Soryy, I do not understand what you mean.
> 
> Timothy Chen wrote:
> Sorry, I meant:
> 
> s/extensions that hint/a recognized extension that hint/g

Got it, will fix. Thanks!


- Bernd


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


On Jan. 28, 2016, 10:17 a.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42914/
> ---
> 
> (Updated Jan. 28, 2016, 10:17 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-4336
> https://issues.apache.org/jira/browse/MESOS-4336
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Edited fetcher.md, listing the supported file extensions when extracting an 
> archive.
> 
> 
> Diffs
> -
> 
>   docs/fetcher.md cb4f3c33fb67f7ac65dbe7ebe1347d2599c43f37 
> 
> Diff: https://reviews.apache.org/r/42914/diff/
> 
> 
> Testing
> ---
> 
> Opened file with MD renderer.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 42914: Listed supported file extensions in fetcher documentation.

[Bernd Mathiske]

> On Jan. 28, 2016, 11:40 p.m., Timothy Chen wrote:
> > docs/fetcher.md, line 119
> > <https://reviews.apache.org/r/42914/diff/2/?file=1225096#file1225096line119>
> >
> > then files with a recognized extension

Soryy, I do not understand what you mean.


- Bernd


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


On Jan. 28, 2016, 10:17 a.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42914/
> ---
> 
> (Updated Jan. 28, 2016, 10:17 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-4336
> https://issues.apache.org/jira/browse/MESOS-4336
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Edited fetcher.md, listing the supported file extensions when extracting an 
> archive.
> 
> 
> Diffs
> -
> 
>   docs/fetcher.md cb4f3c33fb67f7ac65dbe7ebe1347d2599c43f37 
> 
> Diff: https://reviews.apache.org/r/42914/diff/
> 
> 
> Testing
> ---
> 
> Opened file with MD renderer.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 42696: Speed up FetcherCacheTest.Local* test cases.

[Bernd Mathiske]

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




src/tests/fetcher_cache_tests.cpp (line 196)
<https://reviews.apache.org/r/42696/#comment177375>

Good idea, but then we need to set checkpointing to true for the recovery 
test below.



src/tests/fetcher_cache_tests.cpp (line 599)
<https://reviews.apache.org/r/42696/#comment177374>

const



src/tests/fetcher_cache_tests.cpp (line 713)
<https://reviews.apache.org/r/42696/#comment177376>

    const


- Bernd Mathiske


On Jan. 24, 2016, 7:01 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42696/
> ---
> 
> (Updated Jan. 24, 2016, 7:01 a.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Bugs: MESOS-4486
> https://issues.apache.org/jira/browse/MESOS-4486
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up FetcherCacheTest.Local* test cases.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 2747b72ba49c9fde04e556b649601b037517283e 
> 
> Diff: https://reviews.apache.org/r/42696/diff/
> 
> 
> Testing
> ---
> 
> Before apply the patch:
> ```
> $ sudo ./bin/mesos-tests.sh --gtest_filter="FetcherCacheTest.Local*"
> [   OK ] FetcherCacheTest.LocalUncached (2580 ms)
> [   OK ] FetcherCacheTest.LocalCached (2524 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (2514 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (2551 ms)
> ```
> 
> 
> After apply the patch:
> ```
> $ sudo ./bin/mesos-tests.sh --gtest_filter="FetcherCacheTest.Local*"
> [   OK ] FetcherCacheTest.LocalUncached (873 ms)
> [   OK ] FetcherCacheTest.LocalCached (1609 ms)
> [   OK ] FetcherCacheTest.LocalUncachedExtract (926 ms)
> [   OK ] FetcherCacheTest.LocalCachedExtract (1509 ms)
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 42477: Corrected example in quota user doc.

[Bernd Mathiske]

> On Jan. 22, 2016, 8:34 a.m., Bernd Mathiske wrote:
> > docs/quota.md, line 166
> > <https://reviews.apache.org/r/42477/diff/2/?file=1205605#file1205605line166>
> >
> > Please explain what this seemingly redundant field is doing.
> 
> Guangya Liu wrote:
> I think this is because the current Quota can only works for unreserved 
> and non revocable resources. There is indeed a document 
> https://github.com/apache/mesos/blob/master/docs/quota.md for this, make 
> sense?
> 
> Joerg Schad wrote:
> We use the default Resource protobuf message which includes an optional 
> default field  
> ```optional string role = 6 [default = "*"];``` 
> (https://github.com/apache/mesos/blob/master/include/mesos/mesos.proto#L563). 
> Note this is the status endpoint, i.e. this field is not used to specify 
> quota.
> 
> Alexander Rukletsov wrote:
> I agree with Bernd that printing out this field is misleading. Why we do 
> so in the first place? Because that's how JSON is rendered from protobuf by 
> default. Is this is a good reason not to care about it? Probably not. 
> Moreover, we have a family of `model()` functions that specify how certain 
> protobufs should be rendered in JSON (including `Resource`).
> 
> I think we can fix the current issue (this patch documents what is 
> actually being returned) and file a separate ticket for more consistent 
> rendering of `QuotaStatus`. Does it sound right, fellas?

I agree with Alex.


- Bernd


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


On Jan. 22, 2016, 7:43 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42477/
> ---
> 
> (Updated Jan. 22, 2016, 7:43 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/quota.md 208bfa06a9fc50843439bab9a041ccb557657b5d 
> 
> Diff: https://reviews.apache.org/r/42477/diff/
> 
> 
> Testing
> ---
> 
> None: Not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42674: Replaced busybox with alpine in docker tests.

[Bernd Mathiske]

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




src/tests/health_check_tests.cpp (line 351)
<https://reviews.apache.org/r/42674/#comment177079>

Instead of naked strings we should reuse a constant that we can change in 
one place.


- Bernd Mathiske


On Jan. 22, 2016, 5:26 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42674/
> ---
> 
> (Updated Jan. 22, 2016, 5:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced busybox with alpine in docker tests.
> Busybox is currently causing issues with Docker daemon with btrfs backed 
> (https://github.com/docker/docker/issues/16755), which is the default on 
> CentOS 7.
> Tested CentOS and Ubuntu 14.04 with alpine and all Docker tests passed.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 81ab3625a69644d9659d484f3c605aaa5a10b397 
>   src/tests/containerizer/docker_tests.cpp 
> 9583070d64e96f5a57db62b3fb69073ecc5b502b 
>   src/tests/health_check_tests.cpp 3606ce46bfa283ad0d5239fc25e02c5a9f8d1a53 
>   src/tests/hook_tests.cpp 152984b01069acd4cf195bfce58835f0304a97f2 
> 
> Diff: https://reviews.apache.org/r/42674/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 42674: Replaced busybox with alpine in docker tests.

[Bernd Mathiske]

> On Jan. 23, 2016, 9:40 a.m., Jie Yu wrote:
> > Can you add more context in the description? Is that because there are too 
> > many links in busybox, causing issues with docker with brfts backend snce 
> > brfts backend has a limit on links?
> > 
> > Also, can you do a sweep in the code base to make sure there's no 'busybox' 
> > anymore?
> 
> Guangya Liu wrote:
> Per Tim's above comment, he only wants to fix the test cases which are 
> pulling or using the busybox image itself but not some reference, such as 
> https://github.com/apache/mesos/blob/master/src/tests/containerizer/provisioner_docker_tests.cpp#L93
>  etc

Why not use "alpine" in the reference-only cases as well?


- Bernd


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


On Jan. 22, 2016, 5:26 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42674/
> ---
> 
> (Updated Jan. 22, 2016, 5:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced busybox with alpine in docker tests.
> Busybox is currently causing issues with Docker daemon with btrfs backed 
> (https://github.com/docker/docker/issues/16755), which is the default on 
> CentOS 7.
> Tested CentOS and Ubuntu 14.04 with alpine and all Docker tests passed.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 81ab3625a69644d9659d484f3c605aaa5a10b397 
>   src/tests/containerizer/docker_tests.cpp 
> 9583070d64e96f5a57db62b3fb69073ecc5b502b 
>   src/tests/health_check_tests.cpp 3606ce46bfa283ad0d5239fc25e02c5a9f8d1a53 
>   src/tests/hook_tests.cpp 152984b01069acd4cf195bfce58835f0304a97f2 
> 
> Diff: https://reviews.apache.org/r/42674/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 42353: Correctted typo in fetcher.md.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On Jan. 22, 2016, 4:54 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42353/
> ---
> 
> (Updated Jan. 22, 2016, 4:54 p.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Correctted typo in fetcher.md.
> 
> 
> Diffs
> -
> 
>   docs/fetcher.md d9280e1611df13f8217c9a40a02a5391c44ed267 
> 
> Diff: https://reviews.apache.org/r/42353/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 42353: Correctted typo in fetcher.md.

[Bernd Mathiske]

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



docs/fetcher.md (line 236)
<https://reviews.apache.org/r/42353/#comment176903>

Suggestions: 
s/the names/these variable names
s/all in/entirely in


- Bernd Mathiske


On Jan. 22, 2016, 5:18 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42353/
> ---
> 
> (Updated Jan. 22, 2016, 5:18 a.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Corrected typo in fetcher.md.
> 
> 
> Diffs
> -
> 
>   docs/fetcher.md d9280e1611df13f8217c9a40a02a5391c44ed267 
> 
> Diff: https://reviews.apache.org/r/42353/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 42374: Logger Module: Add test filter for tests requiring `logrotate`.

[Bernd Mathiske]

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

Ship it!


Ship It!

- Bernd Mathiske


On Jan. 19, 2016, 7:41 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42374/
> ---
> 
> (Updated Jan. 19, 2016, 7:41 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Artem 
> Harutyunyan.
> 
> 
> Bugs: MESOS-4136
> https://issues.apache.org/jira/browse/MESOS-4136
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `logrotate` binary is used by the non-default `RotatingContainerLogger` 
> module.  On some systems, `logrotate` is not provided by default.
> 
> 
> Diffs
> -
> 
>   src/tests/environment.cpp a6322f260c23796ceaa5d2080126ea9fef0b5ac6 
> 
> Diff: https://reviews.apache.org/r/42374/diff/
> 
> 
> Testing
> ---
> 
> See later reviews.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 42476: Introduced protobuf for set quota requests.

[Bernd Mathiske]

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

Ship it!


After addressing Joerg's remaining issues, ship it!

- Bernd Mathiske


On Jan. 20, 2016, 4:12 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42476/
> ---
> 
> (Updated Jan. 20, 2016, 4:12 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, Joerg Schad, and Joris 
> Van Remoortere.
> 
> 
> Bugs: MESOS-4410
> https://issues.apache.org/jira/browse/MESOS-4410
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6 
>   include/mesos/quota/quota.proto 338412ee967e14aa1957a47f4a50f2e19e4eca79 
>   src/master/quota_handler.cpp f44736cd5849d4fb22a75c1238d433a1c0c9708d 
>   src/tests/master_quota_tests.cpp e8cb074c2913cafdc6b1792896f29e53f1210c9d 
>   src/tests/role_tests.cpp 979391306e2427aaa63a5df32704913f79e20e36 
> 
> Diff: https://reviews.apache.org/r/42476/diff/
> 
> 
> Testing
> ---
> 
> make check on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41648: Used initializer list c-tor for brevity.

[Bernd Mathiske]

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

Ship it!


Ship It!

- Bernd Mathiske


On Jan. 12, 2016, 1:33 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41648/
> ---
> 
> (Updated Jan. 12, 2016, 1:33 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used initializer list c-tor for brevity.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> df8bccaf2b8cfc0cb5ca18d4867371ae7a84c12f 
> 
> Diff: https://reviews.apache.org/r/41648/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41950: Cleaned up hierarchical allocator tests.

[Bernd Mathiske]

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

Ship it!


Ship It!

- Bernd Mathiske


On Jan. 12, 2016, 1:34 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41950/
> ---
> 
> (Updated Jan. 12, 2016, 1:34 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes made:
> - empty resource map promoted to a const class field;
> - removed variable numeric suffixes where appropriate;
> - added const where appropriate.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> e044f832c2c16e53e663c6ced5452649bb0dcb59 
> 
> Diff: https://reviews.apache.org/r/41950/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.10.5
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42535: Made allocator's pause/resume idempotent.

[Bernd Mathiske]

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

Ship it!


Ship It!

- Bernd Mathiske


On Jan. 19, 2016, 6:09 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42535/
> ---
> 
> (Updated Jan. 19, 2016, 6:09 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4417
> https://issues.apache.org/jira/browse/MESOS-4417
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 101482156ffc5a4fe3cd60be222bfe609330ec3c 
>   src/master/allocator/mesos/hierarchical.cpp 
> e32ee4aa3ed9793bb5a99233e699e5cc2bdd796b 
> 
> Diff: https://reviews.apache.org/r/42535/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42476: Introduced protobuf for set quota requests.

[Bernd Mathiske]

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



docs/quota.md (line 94)
<https://reviews.apache.org/r/42476/#comment176375>

plural



include/mesos/quota/quota.proto (line 58)
<https://reviews.apache.org/r/42476/#comment176379>

"should" does not carry the right meaning here.

// The role for which to set quota guarantees.



include/mesos/quota/quota.proto (line 61)
<https://reviews.apache.org/r/42476/#comment176382>

// Resources to be guaranteed as allocatable for the above role.



include/mesos/quota/quota.proto (line 62)
<https://reviews.apache.org/r/42476/#comment176376>

plural



src/master/quota_handler.cpp (line 283)
<https://reviews.apache.org/r/42476/#comment176383>

Why not use protoRequest here?


- Bernd Mathiske


On Jan. 20, 2016, 1 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42476/
> ---
> 
> (Updated Jan. 20, 2016, 1 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, Joerg Schad, and Joris 
> Van Remoortere.
> 
> 
> Bugs: MESOS-4410
> https://issues.apache.org/jira/browse/MESOS-4410
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6 
>   include/mesos/quota/quota.proto 338412ee967e14aa1957a47f4a50f2e19e4eca79 
>   src/master/quota_handler.cpp f44736cd5849d4fb22a75c1238d433a1c0c9708d 
>   src/tests/master_quota_tests.cpp e8cb074c2913cafdc6b1792896f29e53f1210c9d 
>   src/tests/role_tests.cpp 979391306e2427aaa63a5df32704913f79e20e36 
> 
> Diff: https://reviews.apache.org/r/42476/diff/
> 
> 
> Testing
> ---
> 
> make check on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42476: Introduced protobuf for set quota requests.

[Bernd Mathiske]

> On Jan. 20, 2016, 1:14 a.m., Bernd Mathiske wrote:
> > docs/quota.md, line 94
> > <https://reviews.apache.org/r/42476/diff/3/?file=1201682#file1201682line94>
> >
> > plural
> 
> Alexander Rukletsov wrote:
> I think we use single everywhere. It means total quota guarantee and 
> shouldn't necessarily resemble the fact that there are multiple resources 
> objects backing it. Moreover, it will require us to update `QuotaInfo` and 
> `QuotaRequest` protos as well.

I would not have seen it this way if the comment describing the field in 
QuotaRequest did not make me believe this was plural. Now I see your point. 
However, in that case, I would suggest making this perspectiove clear at the 
field definition's comment. Maybe:

// The requested guarantee that these resources will be allocatable for the 
above role.


- Bernd


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


On Jan. 20, 2016, 3:58 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42476/
> ---
> 
> (Updated Jan. 20, 2016, 3:58 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, Joerg Schad, and Joris 
> Van Remoortere.
> 
> 
> Bugs: MESOS-4410
> https://issues.apache.org/jira/browse/MESOS-4410
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6 
>   include/mesos/quota/quota.proto 338412ee967e14aa1957a47f4a50f2e19e4eca79 
>   src/master/quota_handler.cpp f44736cd5849d4fb22a75c1238d433a1c0c9708d 
>   src/tests/master_quota_tests.cpp e8cb074c2913cafdc6b1792896f29e53f1210c9d 
>   src/tests/role_tests.cpp 979391306e2427aaa63a5df32704913f79e20e36 
> 
> Diff: https://reviews.apache.org/r/42476/diff/
> 
> 
> Testing
> ---
> 
> make check on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42390: Fixed fetching uris when slave is running inside a container.

[Bernd Mathiske]

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

Ship it!


Ship It!

- Bernd Mathiske


On Jan. 20, 2016, 6:12 a.m., Shuai Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42390/
> ---
> 
> (Updated Jan. 20, 2016, 6:12 a.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-4249
> https://issues.apache.org/jira/browse/MESOS-4249
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This still requires the slave work dir is a mounted volume, otherwise
> the slave has no way to fetch into the task container's sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp da19975c1f1e59fab0a96ebd9aa815bba2a35ece 
> 
> Diff: https://reviews.apache.org/r/42390/diff/
> 
> 
> Testing
> ---
> 
> - Start a master, then start the slave inside a docker container, with the 
> work_dir being a mounted volume
> - Start a simple framework, which, on receiving the first offer, launches a 
> task with a uri in the command info
> - When the task container is launched, check the file is fetched into the 
> sandbox
> ```sh
> $ docker exec  ls /mnt/mesos/sandbox
> robots.txt
> stderr
> stdout
> ```
> 
> I have collected the scripts I use in testing in [this 
> gist](https://gist.github.com/lins05/14455e92f37e91fd46ff)
> 
> 
> Thanks,
> 
> Shuai Lin
> 
>