Re: Review Request 36908: Added QuotaInfo Protobuf.

[Alexander Rukletsov]

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

Ship it!


Ship It!

- Alexander Rukletsov


On Sept. 2, 2015, 1:38 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36908/
> ---
> 
> (Updated Sept. 2, 2015, 1:38 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3164
> https://issues.apache.org/jira/browse/MESOS-3164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added QuotaInfo Protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/quota.hpp PRE-CREATION 
>   include/mesos/master/quota.proto PRE-CREATION 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
> 
> Diff: https://reviews.apache.org/r/36908/diff/
> 
> 
> Testing
> ---
> 
> make distcheck
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38160: Documented how to expedite event firing.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38161, 38160]

All tests passed.

- Mesos ReviewBot


On Oct. 2, 2015, 1:26 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38160/
> ---
> 
> (Updated Oct. 2, 2015, 1:26 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/mesos-testing-patterns.md d0d92ff569012677043a03412462cf4384096ba9 
> 
> Diff: https://reviews.apache.org/r/38160/diff/
> 
> 
> Testing
> ---
> 
> doc rendered in Markdown
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Alexander Rojas]

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

Review request for mesos, Adam B, Bernd Mathiske, and Till Toenshoff.


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


Repository: mesos


Description
---

Allows developers to provide their own parameters when loading modules instead 
of using the ones provided by the user when loading Mesos. This helps to deal 
with default modules (those used when the user doesn't provide any), and for 
testing of the modules.


Diffs
-

  src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
  src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 38161: Replaced a hard-coded number for registration backoff with a proper constant.

[Alexander Rukletsov]

> On Oct. 1, 2015, 6:08 p.m., Ben Mahler wrote:
> > src/tests/fault_tolerance_tests.cpp, lines 650-651
> > 
> >
> > I don't think we need this second sentence, since the DEFAULT expresses 
> > this pretty clearly, no?

My personal preference is to be more verbose. I think for a project newbie it's 
hard to grasp that `MesosSchedulerDriver driver2(, framework2, 
master.get(), DEFAULT_CREDENTIAL);` sets a registration backoff to a certain 
value which we use later on to advance the clock. However, I also see your 
point => removing the sentence.


> On Oct. 1, 2015, 6:08 p.m., Ben Mahler wrote:
> > src/tests/fault_tolerance_tests.cpp, lines 1547-1551
> > 
> >
> > It looks inconsistent to do this here but not in the other tests this 
> > patch touches. Now that the variable name includes DEFAULT, how about we 
> > remove the explicit setting here?

We do not use this value for `Clock::advance()` in other tests. Though it is 
indeed inconsistent, explicit setting ensures we advance for the same duration 
as used by the agent. If we remove it, `CreateSlaveFlags()` may change this 
value.


- Alexander


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


On Oct. 1, 2015, 10:07 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38161/
> ---
> 
> (Updated Oct. 1, 2015, 10:07 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prepend a constant regulating the backoff factor with DEFAULT_ prefix for 
> clarity, use it in tests where appropriate and extend comments for posterity.
> 
> 
> Diffs
> -
> 
>   src/sched/constants.hpp ac497b2abc9fbeab713b312cf37963d7de28ed50 
>   src/sched/constants.cpp 517ca5cb0dad49d775d65c9ffabc849ff85347a0 
>   src/sched/flags.hpp 4e0d56f61b6877254161112b288ae4a9c579e846 
>   src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341 
>   src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55 
>   src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 
>   src/tests/fault_tolerance_tests.cpp 
> c97bc4691f9bac4a8677e6d2247be96ee9674b57 
> 
> Diff: https://reviews.apache.org/r/38161/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38161: Replaced a hard-coded number for registration backoff with a proper constant.

[Alexander Rukletsov]

> On Oct. 1, 2015, 6:08 p.m., Ben Mahler wrote:
> > src/tests/fault_tolerance_tests.cpp, line 1540
> > 
> >
> > I could have committed the `> >` flattening already for you if it was 
> > split, also, it would be be better to do a pass on the entire file in one 
> > patch rather than stick in it alongside the rest of your change, thoughts?

Sure, see https://reviews.apache.org/r/38952/


- Alexander


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


On Oct. 1, 2015, 10:07 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38161/
> ---
> 
> (Updated Oct. 1, 2015, 10:07 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prepend a constant regulating the backoff factor with DEFAULT_ prefix for 
> clarity, use it in tests where appropriate and extend comments for posterity.
> 
> 
> Diffs
> -
> 
>   src/sched/constants.hpp ac497b2abc9fbeab713b312cf37963d7de28ed50 
>   src/sched/constants.cpp 517ca5cb0dad49d775d65c9ffabc849ff85347a0 
>   src/sched/flags.hpp 4e0d56f61b6877254161112b288ae4a9c579e846 
>   src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341 
>   src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55 
>   src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 
>   src/tests/fault_tolerance_tests.cpp 
> c97bc4691f9bac4a8677e6d2247be96ee9674b57 
> 
> Diff: https://reviews.apache.org/r/38161/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38160: Documented how to expedite event firing.

[Alexander Rukletsov]

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

(Updated Oct. 2, 2015, 1:26 p.m.)


Review request for mesos, Bernd Mathiske and Ben Mahler.


Changes
---

Rebased.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/mesos-testing-patterns.md d0d92ff569012677043a03412462cf4384096ba9 

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


Testing
---

doc rendered in Markdown


Thanks,

Alexander Rukletsov

Review Request 38952: Replaced `> >` with `>>` in fault tolerance tests.

[Alexander Rukletsov]

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

Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/tests/fault_tolerance_tests.cpp c97bc4691f9bac4a8677e6d2247be96ee9674b57 

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


Testing
---

make check (Mac OS 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 38952: Replaced `> >` with `>>` in fault tolerance tests.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [38161, 38952]

Failed command: ./support/apply-review.sh -n -r 38952

Error:
 2015-10-02 13:28:39 URL:https://reviews.apache.org/r/38952/diff/raw/ 
[13279/13279] -> "38952.patch" [1]
error: patch failed: src/tests/fault_tolerance_tests.cpp:589
error: src/tests/fault_tolerance_tests.cpp: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On Oct. 2, 2015, 1:26 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38952/
> ---
> 
> (Updated Oct. 2, 2015, 1:26 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/fault_tolerance_tests.cpp 
> c97bc4691f9bac4a8677e6d2247be96ee9674b57 
> 
> Diff: https://reviews.apache.org/r/38952/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38950: Http Authenticators can be loaded as modules from mesos.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37714, 37996, 37997, 37998, 37999, 38000, 38094, 38627, 38950]

All tests passed.

- Mesos ReviewBot


On Oct. 2, 2015, 9:37 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38950/
> ---
> 
> (Updated Oct. 2, 2015, 9:37 a.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3232
> https://issues.apache.org/jira/browse/MESOS-3232
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds support for modularization of HTTP Authenticators. 
> 
> It includes an example of how to do it with the Basic HTTP Authenticator.
> 
> 
> Diffs
> -
> 
>   include/mesos/authentication/http/basic_authenticator_factory.hpp 
> PRE-CREATION 
>   include/mesos/module/http_authenticator.hpp PRE-CREATION 
>   src/Makefile.am 8aa456611dd5405336dd7b0c19ba4a942ea1c805 
>   src/authentication/http/basic_authenticator_factory.cpp PRE-CREATION 
>   src/examples/test_http_authenticator_module.cpp PRE-CREATION 
>   src/master/constants.hpp 51d477444c4d1a2c9dd2f32164ebffbb1fd5c8c2 
>   src/master/constants.cpp 2b66b27783f18930010ee912e9977ea1647eba09 
>   src/module/manager.cpp f9a0643a70bc9de1484599629041650493842c69 
>   src/tests/http_authentication_tests.cpp PRE-CREATION 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
>   src/tests/module.cpp edab0b37dcf0bd8e15d439726354039c1bbcd51f 
> 
> Diff: https://reviews.apache.org/r/38950/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Review Request 38950: Http Authenticators can be loaded as modules from mesos.

[Alexander Rojas]

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

Review request for mesos, Adam B, Bernd Mathiske, and Till Toenshoff.


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


Repository: mesos


Description
---

Adds support for modularization of HTTP Authenticators. 

It includes an example of how to do it with the Basic HTTP Authenticator.


Diffs
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
PRE-CREATION 
  include/mesos/module/http_authenticator.hpp PRE-CREATION 
  src/Makefile.am 8aa456611dd5405336dd7b0c19ba4a942ea1c805 
  src/authentication/http/basic_authenticator_factory.cpp PRE-CREATION 
  src/examples/test_http_authenticator_module.cpp PRE-CREATION 
  src/master/constants.hpp 51d477444c4d1a2c9dd2f32164ebffbb1fd5c8c2 
  src/master/constants.cpp 2b66b27783f18930010ee912e9977ea1647eba09 
  src/module/manager.cpp f9a0643a70bc9de1484599629041650493842c69 
  src/tests/http_authentication_tests.cpp PRE-CREATION 
  src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
  src/tests/module.cpp edab0b37dcf0bd8e15d439726354039c1bbcd51f 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 38952: Replaced `> >` with `>>` in fault tolerance tests.

[Alexander Rukletsov]

> On Oct. 2, 2015, 3:01 p.m., Neil Conway wrote:
> > I agree this is an improvement, but rather than doing this piecemeal, why 
> > not do it once across the whole codebase?
> > 
> > Might also be worth adding to the style guide.

We have discussed that before and the agreement was not to do a sweep, but 
rather incrementally change when we touch appropriate files. See comments in 
https://reviews.apache.org/r/38161/ for reasoning for this patch.


- Alexander


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


On Oct. 2, 2015, 1:26 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38952/
> ---
> 
> (Updated Oct. 2, 2015, 1:26 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/fault_tolerance_tests.cpp 
> c97bc4691f9bac4a8677e6d2247be96ee9674b57 
> 
> Diff: https://reviews.apache.org/r/38952/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38160: Documented how to expedite event firing.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38161, 38952, 38160]

All tests passed.

- Mesos ReviewBot


On Oct. 2, 2015, 3:13 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38160/
> ---
> 
> (Updated Oct. 2, 2015, 3:13 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/mesos-testing-patterns.md d0d92ff569012677043a03412462cf4384096ba9 
> 
> Diff: https://reviews.apache.org/r/38160/diff/
> 
> 
> Testing
> ---
> 
> doc rendered in Markdown
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38160: Documented how to expedite event firing.

[Alexander Rukletsov]

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

(Updated Oct. 2, 2015, 3:13 p.m.)


Review request for mesos, Bernd Mathiske and Ben Mahler.


Changes
---

Rebased.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/mesos-testing-patterns.md d0d92ff569012677043a03412462cf4384096ba9 

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


Testing
---

doc rendered in Markdown


Thanks,

Alexander Rukletsov

Re: Review Request 38161: Replaced a hard-coded number for registration backoff with a proper constant.

[Alexander Rukletsov]

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

(Updated Oct. 2, 2015, 3:13 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Rebased.


Repository: mesos


Description
---

Prepend a constant regulating the backoff factor with DEFAULT_ prefix for 
clarity, use it in tests where appropriate and extend comments for posterity.


Diffs (updated)
-

  src/sched/constants.hpp ac497b2abc9fbeab713b312cf37963d7de28ed50 
  src/sched/constants.cpp 517ca5cb0dad49d775d65c9ffabc849ff85347a0 
  src/sched/flags.hpp 4e0d56f61b6877254161112b288ae4a9c579e846 
  src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341 
  src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55 
  src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 
  src/tests/fault_tolerance_tests.cpp c97bc4691f9bac4a8677e6d2247be96ee9674b57 

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


Testing
---

make check (Mac OS 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 38952: Replaced `> >` with `>>` in fault tolerance tests.

[Neil Conway]

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


I agree this is an improvement, but rather than doing this piecemeal, why not 
do it once across the whole codebase?

Might also be worth adding to the style guide.

- Neil Conway


On Oct. 2, 2015, 1:26 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38952/
> ---
> 
> (Updated Oct. 2, 2015, 1:26 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/fault_tolerance_tests.cpp 
> c97bc4691f9bac4a8677e6d2247be96ee9674b57 
> 
> Diff: https://reviews.apache.org/r/38952/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38952: Replaced `> >` with `>>` in fault tolerance tests.

[Alexander Rukletsov]

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

(Updated Oct. 2, 2015, 3:13 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Rebased.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/fault_tolerance_tests.cpp c97bc4691f9bac4a8677e6d2247be96ee9674b57 

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


Testing
---

make check (Mac OS 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 38580: Added docker registry RemotePuller

[Timothy Chen]

> On Oct. 1, 2015, 6:53 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioner/docker/remote_puller.cpp, line 227
> > 
> >
> > I thought you wanted to move this to somewhere shared? We can create a 
> > base puller class and move this there.
> 
> Jojy Varghese wrote:
> Thought about it a little more and realized that the functionality of 
> "untar a tarball into a dierctory" should belong in a common place like 
> libprocess. Its not a function of puller but maybe a Tar class.

Sure, are you going to do that? I think it's easier to put it in a shared place 
for now since it's going to take longer to merge to libprocess IMO.


- Timothy


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


On Oct. 2, 2015, 5:18 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> ---
> 
> (Updated Oct. 2, 2015, 5:18 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Integrated remote puller with store. Tests will follow.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8aa456611dd5405336dd7b0c19ba4a942ea1c805 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp 
> 4574e8a04663482625d7b54f765741f221ec13e0 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp 
> 4a0b7d11f013941084571f2d89d835a4668a3d8b 
>   src/slave/containerizer/provisioner/docker/puller.hpp 
> 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 
>   src/slave/containerizer/provisioner/docker/puller.cpp 
> cb05324689ffa26ce830b513e2d71b55517da3cb 
>   src/slave/containerizer/provisioner/docker/registry_client.hpp 
> 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185 
>   src/slave/containerizer/provisioner/docker/registry_client.cpp 
> c2040b48ea43fdb29766994c244273d3fa9ee3cd 
>   src/slave/containerizer/provisioner/docker/remote_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp 
> cbb67686d45513f0395a0cf1bc5c43cb4935adae 
>   src/slave/flags.hpp f76f0f6a0c7a01c7f061a225d7f6ef52be0ee7b5 
>   src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> d895eb9d0723e52cff8b21ef2deeaef1911d019c 
> 
> Diff: https://reviews.apache.org/r/38580/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38960: Add QuotaInfo Protobuf to CMake.

[Alex Clemmer]

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

Ship it!


Ship It!

- Alex Clemmer


On Oct. 2, 2015, 4:51 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38960/
> ---
> 
> (Updated Oct. 2, 2015, 4:51 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, Alex Clemmer, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> QuotaInfo was introduced here: https://reviews.apache.org/r/36908/
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 891f951f3107ece29b7923b7a3cc414e2ea56983 
> 
> Diff: https://reviews.apache.org/r/38960/diff/
> 
> 
> Testing
> ---
> 
> ```
> cmake .. # (OSX)
> make -j8 | grep quota
> ```
> 
> Confirmed that quota protobuf was built.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 38961: Simplified resources initialization.

[Alexander Rukletsov]

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

Review request for mesos and Bernd Mathiske.


Repository: mesos


Description
---

Simplified resources initialization.


Diffs
-

  src/tests/fetcher_cache_tests.cpp 31ca14621962f8851920b733ba0dec9f1afe8e79 

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


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 38580: Added docker registry RemotePuller

[Timothy Chen]

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



src/slave/containerizer/provisioner/docker/remote_puller.cpp (line 143)


I think this is going to crash when user passes in a invalid sec value, 
since you check for error but you do a get in the if statement.

You should break it into two checks and do different actions.

Also remove the unnecessary parenthesis around timeoutSecs.isError()



src/slave/containerizer/provisioner/docker/remote_puller.cpp (line 179)


Align << on both lines.



src/slave/containerizer/provisioner/docker/remote_puller.cpp (line 191)


Fix all the indentation with LOGs please.



src/slave/containerizer/provisioner/docker/remote_puller.cpp (line 218)


You're missing the discard case here.
If timeout happens, you discard the future and I believe it will call the 
following callback here.

Therefore this is also going to crash if it times out.

I suggest we really write tests around these as I don't think this will 
work at all.


- Timothy Chen


On Oct. 2, 2015, 5:18 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> ---
> 
> (Updated Oct. 2, 2015, 5:18 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Integrated remote puller with store. Tests will follow.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8aa456611dd5405336dd7b0c19ba4a942ea1c805 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp 
> 4574e8a04663482625d7b54f765741f221ec13e0 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp 
> 4a0b7d11f013941084571f2d89d835a4668a3d8b 
>   src/slave/containerizer/provisioner/docker/puller.hpp 
> 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 
>   src/slave/containerizer/provisioner/docker/puller.cpp 
> cb05324689ffa26ce830b513e2d71b55517da3cb 
>   src/slave/containerizer/provisioner/docker/registry_client.hpp 
> 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185 
>   src/slave/containerizer/provisioner/docker/registry_client.cpp 
> c2040b48ea43fdb29766994c244273d3fa9ee3cd 
>   src/slave/containerizer/provisioner/docker/remote_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp 
> cbb67686d45513f0395a0cf1bc5c43cb4935adae 
>   src/slave/flags.hpp f76f0f6a0c7a01c7f061a225d7f6ef52be0ee7b5 
>   src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> d895eb9d0723e52cff8b21ef2deeaef1911d019c 
> 
> Diff: https://reviews.apache.org/r/38580/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38960: Add QuotaInfo Protobuf to CMake.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38960]

All tests passed.

- Mesos ReviewBot


On Oct. 2, 2015, 4:51 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38960/
> ---
> 
> (Updated Oct. 2, 2015, 4:51 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, Alex Clemmer, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> QuotaInfo was introduced here: https://reviews.apache.org/r/36908/
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 891f951f3107ece29b7923b7a3cc414e2ea56983 
> 
> Diff: https://reviews.apache.org/r/38960/diff/
> 
> 
> Testing
> ---
> 
> ```
> cmake .. # (OSX)
> make -j8 | grep quota
> ```
> 
> Confirmed that quota protobuf was built.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 38161: Replaced a hard-coded number for registration backoff with a proper constant.

[Alexander Rukletsov]

> On Oct. 1, 2015, 6:08 p.m., Ben Mahler wrote:
> > src/tests/fault_tolerance_tests.cpp, lines 1547-1551
> > 
> >
> > It looks inconsistent to do this here but not in the other tests this 
> > patch touches. Now that the variable name includes DEFAULT, how about we 
> > remove the explicit setting here?
> 
> Alexander Rukletsov wrote:
> We do not use this value for `Clock::advance()` in other tests. Though it 
> is indeed inconsistent, explicit setting ensures we advance for the same 
> duration as used by the agent. If we remove it, `CreateSlaveFlags()` may 
> change this value.
> 
> Ben Mahler wrote:
> Really? I see two other instances of Clock::advance with 
> DEFAULT_REGISTRATION_BACKOFF_FACTOR passed in, looking at the diff alone.

Okay, these two do not count, they are being introduced now : ). The difference 
between those and this one is that AFAIK we cannot change Scheduler flags from 
the tests, but we can do it with the agent flags.


- Alexander


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


On Oct. 2, 2015, 3:13 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38161/
> ---
> 
> (Updated Oct. 2, 2015, 3:13 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prepend a constant regulating the backoff factor with DEFAULT_ prefix for 
> clarity, use it in tests where appropriate and extend comments for posterity.
> 
> 
> Diffs
> -
> 
>   src/sched/constants.hpp ac497b2abc9fbeab713b312cf37963d7de28ed50 
>   src/sched/constants.cpp 517ca5cb0dad49d775d65c9ffabc849ff85347a0 
>   src/sched/flags.hpp 4e0d56f61b6877254161112b288ae4a9c579e846 
>   src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341 
>   src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55 
>   src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 
>   src/tests/fault_tolerance_tests.cpp 
> c97bc4691f9bac4a8677e6d2247be96ee9674b57 
> 
> Diff: https://reviews.apache.org/r/38161/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 38963: Added initial draft of networking user-doc.

[Kapil Arya]

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

Review request for mesos, Connor Doyle and Niklas Nielsen.


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


Repository: mesos


Description
---

Added initial draft of networking user-doc.


Diffs
-

  docs/images/networking-architecture.png PRE-CREATION 
  docs/networking.md PRE-CREATION 

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


Testing
---

Markdown rendering at 
https://github.com/karya0/mesos/blob/net-user-doc/docs/networking.md


Thanks,

Kapil Arya

Re: Review Request 38814: add test cases for sha512 digest verifier

[Gilbert Song]

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

(Updated Oct. 2, 2015, 11:29 a.m.)


Review request for mesos, Jojy Varghese and Timothy Chen.


Repository: mesos


Description
---

add test cases for sha512 digest verifier


Diffs (updated)
-

  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

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


Testing
---

make check (ubuntu 14.04)


Thanks,

Gilbert Song

Re: Review Request 38416: Allow HTTP response codes to be checked with code.

[Timothy Chen]

> On Sept. 25, 2015, 10:52 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/http.hpp, lines 59-60
> > 
> >
> > How about just status()?
> > 
> > We can also avoid Option here by just returning the number value with 
> > no message? That seems cleaner?
> 
> Timothy Chen wrote:
> Not sure how this is useful if it only returns the number value?
> 
> Ben Mahler wrote:
> Well, you have unconditional gets which can crash the program:
> 
> ```
>   Response(StatusCode _code)
> : type(NONE), code(_code)
>   {
> status = statusString((uint16_t)code).get(); // XXX
>   }
> 
>   explicit Response(
>   const std::string& _body,
>   StatusCode _code)
> : type(BODY),
>   body(_body),
>   code(_code)
>   {
> headers["Content-Length"] = stringify(body.size());
> status = statusString((uint16_t)code).get(); // XXX
>   }
> ```
> 
> Also the tests have unconditional gets, which is unfortunate. Rather than 
> updating all the call sites to deal with None, I would suggest following a 
> similar approach to strerror (which always returns a string, if I pass  I 
> get back "Unknown error: ".
> 
> The other reason is that since the strings coming out from this method 
> include the number (" "), a better answer for unknown 
> statuses is to only include the number (""), since this is already 
> captures that  is omitted. Thoughts?
> 
> Timothy Chen wrote:
> Ah ok I didn't get the default error message part. I think if we're going 
> to keep the map and also use it to get status strings, I think I'll keep the 
> old behavior where we have statuses map which is statically initialized, and 
> just use that to check and get status strings for now. Since we have to check 
> the status code in the map anyway to set the response->flag.

I meant to say I didn't get the default error message part before you left the 
comment :)


- Timothy


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


On Sept. 25, 2015, 10:38 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38416/
> ---
> 
> (Updated Sept. 25, 2015, 10:38 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, Jie Yu, Jojy Varghese, 
> and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3429
> https://issues.apache.org/jira/browse/MESOS-3429
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow HTTP response codes to be checked with code.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> fbd6cf7967173495875a8ea90ed28ade88b982a2 
>   3rdparty/libprocess/src/decoder.hpp 
> 67a5135f302153e376e8dfe8db82aa0b15449389 
>   3rdparty/libprocess/src/http.cpp 9ad613a16c379b6d76a9a0f8d6160fe23a182fd4 
>   3rdparty/libprocess/src/process.cpp 
> 4afa30569b4d235637b49a624602e6b199c32e0e 
>   3rdparty/libprocess/src/tests/benchmarks.cpp 
> 97c81b8e61a75771e5bf7d46505cec4e0c0f404a 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> d0b9399d38fa284466a012a21080b1d9007af98b 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 1023500ac2e3c55be955c4686e7f720eba6b4cc9 
> 
> Diff: https://reviews.apache.org/r/38416/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Review Request 38960: Add QuotaInfo Protobuf to CMake.

[Joseph Wu]

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

Review request for mesos, Alexander Rukletsov, Bernd Mathiske, Alex Clemmer, 
and Till Toenshoff.


Repository: mesos


Description
---

QuotaInfo was introduced here: https://reviews.apache.org/r/36908/


Diffs
-

  src/CMakeLists.txt 891f951f3107ece29b7923b7a3cc414e2ea56983 

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


Testing
---

```
cmake .. # (OSX)
make -j8 | grep quota
```

Confirmed that quota protobuf was built.


Thanks,

Joseph Wu

Re: Review Request 38580: Added docker registry RemotePuller

[Jojy Varghese]

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

(Updated Oct. 2, 2015, 5:18 p.m.)


Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.


Changes
---

addressed review comments.


Repository: mesos


Description
---

Integrated remote puller with store. Tests will follow.


Diffs (updated)
-

  src/Makefile.am 8aa456611dd5405336dd7b0c19ba4a942ea1c805 
  src/slave/containerizer/provisioner/docker/local_puller.hpp 
4574e8a04663482625d7b54f765741f221ec13e0 
  src/slave/containerizer/provisioner/docker/local_puller.cpp 
4a0b7d11f013941084571f2d89d835a4668a3d8b 
  src/slave/containerizer/provisioner/docker/puller.hpp 
105b4e75439c2ad4c08e2fd364f288f1d39b9b59 
  src/slave/containerizer/provisioner/docker/puller.cpp 
cb05324689ffa26ce830b513e2d71b55517da3cb 
  src/slave/containerizer/provisioner/docker/registry_client.hpp 
9d5d15455192e2d75fe5cd7fa8755fb8cc67e185 
  src/slave/containerizer/provisioner/docker/registry_client.cpp 
c2040b48ea43fdb29766994c244273d3fa9ee3cd 
  src/slave/containerizer/provisioner/docker/remote_puller.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/store.cpp 
cbb67686d45513f0395a0cf1bc5c43cb4935adae 
  src/slave/flags.hpp f76f0f6a0c7a01c7f061a225d7f6ef52be0ee7b5 
  src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 
  src/tests/containerizer/provisioner_docker_tests.cpp 
d895eb9d0723e52cff8b21ef2deeaef1911d019c 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38416: Allow HTTP response codes to be checked with code.

[Ben Mahler]

> On Sept. 25, 2015, 10:52 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/http.hpp, lines 59-60
> > 
> >
> > How about just status()?
> > 
> > We can also avoid Option here by just returning the number value with 
> > no message? That seems cleaner?
> 
> Timothy Chen wrote:
> Not sure how this is useful if it only returns the number value?

Well, you have unconditional gets which can crash the program:

```
  Response(StatusCode _code)
: type(NONE), code(_code)
  {
status = statusString((uint16_t)code).get(); // XXX
  }

  explicit Response(
  const std::string& _body,
  StatusCode _code)
: type(BODY),
  body(_body),
  code(_code)
  {
headers["Content-Length"] = stringify(body.size());
status = statusString((uint16_t)code).get(); // XXX
  }
```

Also the tests have unconditional gets, which is unfortunate. Rather than 
updating all the call sites to deal with None, I would suggest following a 
similar approach to strerror (which always returns a string, if I pass  I 
get back "Unknown error: ".

The other reason is that since the strings coming out from this method include 
the number (" "), a better answer for unknown statuses is to 
only include the number (""), since this is already captures that 
 is omitted. Thoughts?


- Ben


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


On Sept. 25, 2015, 10:38 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38416/
> ---
> 
> (Updated Sept. 25, 2015, 10:38 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, Jie Yu, Jojy Varghese, 
> and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3429
> https://issues.apache.org/jira/browse/MESOS-3429
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow HTTP response codes to be checked with code.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> fbd6cf7967173495875a8ea90ed28ade88b982a2 
>   3rdparty/libprocess/src/decoder.hpp 
> 67a5135f302153e376e8dfe8db82aa0b15449389 
>   3rdparty/libprocess/src/http.cpp 9ad613a16c379b6d76a9a0f8d6160fe23a182fd4 
>   3rdparty/libprocess/src/process.cpp 
> 4afa30569b4d235637b49a624602e6b199c32e0e 
>   3rdparty/libprocess/src/tests/benchmarks.cpp 
> 97c81b8e61a75771e5bf7d46505cec4e0c0f404a 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> d0b9399d38fa284466a012a21080b1d9007af98b 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 1023500ac2e3c55be955c4686e7f720eba6b4cc9 
> 
> Diff: https://reviews.apache.org/r/38416/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38416: Allow HTTP response codes to be checked with code.

[Timothy Chen]

> On Sept. 25, 2015, 10:52 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/http.hpp, lines 59-60
> > 
> >
> > How about just status()?
> > 
> > We can also avoid Option here by just returning the number value with 
> > no message? That seems cleaner?
> 
> Timothy Chen wrote:
> Not sure how this is useful if it only returns the number value?
> 
> Ben Mahler wrote:
> Well, you have unconditional gets which can crash the program:
> 
> ```
>   Response(StatusCode _code)
> : type(NONE), code(_code)
>   {
> status = statusString((uint16_t)code).get(); // XXX
>   }
> 
>   explicit Response(
>   const std::string& _body,
>   StatusCode _code)
> : type(BODY),
>   body(_body),
>   code(_code)
>   {
> headers["Content-Length"] = stringify(body.size());
> status = statusString((uint16_t)code).get(); // XXX
>   }
> ```
> 
> Also the tests have unconditional gets, which is unfortunate. Rather than 
> updating all the call sites to deal with None, I would suggest following a 
> similar approach to strerror (which always returns a string, if I pass  I 
> get back "Unknown error: ".
> 
> The other reason is that since the strings coming out from this method 
> include the number (" "), a better answer for unknown 
> statuses is to only include the number (""), since this is already 
> captures that  is omitted. Thoughts?

Ah ok I didn't get the default error message part. I think if we're going to 
keep the map and also use it to get status strings, I think I'll keep the old 
behavior where we have statuses map which is statically initialized, and just 
use that to check and get status strings for now. Since we have to check the 
status code in the map anyway to set the response->flag.


- Timothy


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


On Sept. 25, 2015, 10:38 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38416/
> ---
> 
> (Updated Sept. 25, 2015, 10:38 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, Jie Yu, Jojy Varghese, 
> and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3429
> https://issues.apache.org/jira/browse/MESOS-3429
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow HTTP response codes to be checked with code.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> fbd6cf7967173495875a8ea90ed28ade88b982a2 
>   3rdparty/libprocess/src/decoder.hpp 
> 67a5135f302153e376e8dfe8db82aa0b15449389 
>   3rdparty/libprocess/src/http.cpp 9ad613a16c379b6d76a9a0f8d6160fe23a182fd4 
>   3rdparty/libprocess/src/process.cpp 
> 4afa30569b4d235637b49a624602e6b199c32e0e 
>   3rdparty/libprocess/src/tests/benchmarks.cpp 
> 97c81b8e61a75771e5bf7d46505cec4e0c0f404a 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> d0b9399d38fa284466a012a21080b1d9007af98b 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 1023500ac2e3c55be955c4686e7f720eba6b4cc9 
> 
> Diff: https://reviews.apache.org/r/38416/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Isabel Jimenez]

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

(Updated Oct. 2, 2015, 6:42 p.m.)


Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.


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


Repository: mesos


Description
---

Added validation for Call protobuf message in Agent /api/v1/executor endpoint.


Diffs (updated)
-

  src/CMakeLists.txt 891f951 
  src/Makefile.am 8aa4566 
  src/slave/http.cpp f9cf7bb 
  src/slave/validation.hpp PRE-CREATION 
  src/slave/validation.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Isabel Jimenez

Re: Review Request 38960: Add QuotaInfo Protobuf to CMake.

[Gilbert Song]

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

Ship it!


Ship It!

- Gilbert Song


On Oct. 2, 2015, 9:51 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38960/
> ---
> 
> (Updated Oct. 2, 2015, 9:51 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, Alex Clemmer, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> QuotaInfo was introduced here: https://reviews.apache.org/r/36908/
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 891f951f3107ece29b7923b7a3cc414e2ea56983 
> 
> Diff: https://reviews.apache.org/r/38960/diff/
> 
> 
> Testing
> ---
> 
> ```
> cmake .. # (OSX)
> make -j8 | grep quota
> ```
> 
> Confirmed that quota protobuf was built.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 38963: Added initial draft of networking user-doc.

[Joseph Wu]

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


A few things you're missing (haven't read the doc yet):

* A link to this from `home.md`.
* You need to generate this diff manually (via `git diff --binary`) and post 
it.  The image won't be included otherwise.  (ReviewBot is going to say "bad 
patch" soon.)
* Make sure you render this using 
https://github.com/mesosphere/mesos-website-container  (Note that the image 
will *not* show up, until https://issues.apache.org/jira/browse/MESOS-3183 is 
resolved).

- Joseph Wu


On Oct. 2, 2015, 11:08 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38963/
> ---
> 
> (Updated Oct. 2, 2015, 11:08 a.m.)
> 
> 
> Review request for mesos, Connor Doyle and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3561
> https://issues.apache.org/jira/browse/MESOS-3561
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial draft of networking user-doc.
> 
> 
> Diffs
> -
> 
>   docs/images/networking-architecture.png PRE-CREATION 
>   docs/networking.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38963/diff/
> 
> 
> Testing
> ---
> 
> Markdown rendering at 
> https://github.com/karya0/mesos/blob/net-user-doc/docs/networking.md
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 38580: Added docker registry RemotePuller

[Timothy Chen]

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



src/slave/containerizer/provisioner/docker/registry_client.cpp (line 171)


Fix the indentation here, we missed it last time.


- Timothy Chen


On Oct. 2, 2015, 5:18 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> ---
> 
> (Updated Oct. 2, 2015, 5:18 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Integrated remote puller with store. Tests will follow.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8aa456611dd5405336dd7b0c19ba4a942ea1c805 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp 
> 4574e8a04663482625d7b54f765741f221ec13e0 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp 
> 4a0b7d11f013941084571f2d89d835a4668a3d8b 
>   src/slave/containerizer/provisioner/docker/puller.hpp 
> 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 
>   src/slave/containerizer/provisioner/docker/puller.cpp 
> cb05324689ffa26ce830b513e2d71b55517da3cb 
>   src/slave/containerizer/provisioner/docker/registry_client.hpp 
> 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185 
>   src/slave/containerizer/provisioner/docker/registry_client.cpp 
> c2040b48ea43fdb29766994c244273d3fa9ee3cd 
>   src/slave/containerizer/provisioner/docker/remote_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp 
> cbb67686d45513f0395a0cf1bc5c43cb4935adae 
>   src/slave/flags.hpp f76f0f6a0c7a01c7f061a225d7f6ef52be0ee7b5 
>   src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> d895eb9d0723e52cff8b21ef2deeaef1911d019c 
> 
> Diff: https://reviews.apache.org/r/38580/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38161: Replaced a hard-coded number for registration backoff with a proper constant.

[Ben Mahler]

> On Oct. 1, 2015, 6:08 p.m., Ben Mahler wrote:
> > src/tests/fault_tolerance_tests.cpp, lines 1547-1551
> > 
> >
> > It looks inconsistent to do this here but not in the other tests this 
> > patch touches. Now that the variable name includes DEFAULT, how about we 
> > remove the explicit setting here?
> 
> Alexander Rukletsov wrote:
> We do not use this value for `Clock::advance()` in other tests. Though it 
> is indeed inconsistent, explicit setting ensures we advance for the same 
> duration as used by the agent. If we remove it, `CreateSlaveFlags()` may 
> change this value.

Really? I see two other instances of Clock::advance with 
DEFAULT_REGISTRATION_BACKOFF_FACTOR passed in, looking at the diff alone.


- Ben


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


On Oct. 2, 2015, 3:13 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38161/
> ---
> 
> (Updated Oct. 2, 2015, 3:13 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prepend a constant regulating the backoff factor with DEFAULT_ prefix for 
> clarity, use it in tests where appropriate and extend comments for posterity.
> 
> 
> Diffs
> -
> 
>   src/sched/constants.hpp ac497b2abc9fbeab713b312cf37963d7de28ed50 
>   src/sched/constants.cpp 517ca5cb0dad49d775d65c9ffabc849ff85347a0 
>   src/sched/flags.hpp 4e0d56f61b6877254161112b288ae4a9c579e846 
>   src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341 
>   src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55 
>   src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 
>   src/tests/fault_tolerance_tests.cpp 
> c97bc4691f9bac4a8677e6d2247be96ee9674b57 
> 
> Diff: https://reviews.apache.org/r/38161/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38960: Add QuotaInfo Protobuf to CMake.

[Alexander Rukletsov]

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

Ship it!


Good catch, thanks Joseph!

- Alexander Rukletsov


On Oct. 2, 2015, 4:51 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38960/
> ---
> 
> (Updated Oct. 2, 2015, 4:51 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, Alex Clemmer, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> QuotaInfo was introduced here: https://reviews.apache.org/r/36908/
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 891f951f3107ece29b7923b7a3cc414e2ea56983 
> 
> Diff: https://reviews.apache.org/r/38960/diff/
> 
> 
> Testing
> ---
> 
> ```
> cmake .. # (OSX)
> make -j8 | grep quota
> ```
> 
> Confirmed that quota protobuf was built.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 38580: Added docker registry RemotePuller

[Jojy Varghese]

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

(Updated Oct. 2, 2015, 6:41 p.m.)


Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.


Changes
---

review comments addressed.


Repository: mesos


Description
---

Integrated remote puller with store. Tests will follow.


Diffs (updated)
-

  src/Makefile.am 8aa456611dd5405336dd7b0c19ba4a942ea1c805 
  src/slave/containerizer/provisioner/docker/local_puller.hpp 
4574e8a04663482625d7b54f765741f221ec13e0 
  src/slave/containerizer/provisioner/docker/local_puller.cpp 
4a0b7d11f013941084571f2d89d835a4668a3d8b 
  src/slave/containerizer/provisioner/docker/puller.hpp 
105b4e75439c2ad4c08e2fd364f288f1d39b9b59 
  src/slave/containerizer/provisioner/docker/puller.cpp 
cb05324689ffa26ce830b513e2d71b55517da3cb 
  src/slave/containerizer/provisioner/docker/registry_client.hpp 
9d5d15455192e2d75fe5cd7fa8755fb8cc67e185 
  src/slave/containerizer/provisioner/docker/registry_client.cpp 
c2040b48ea43fdb29766994c244273d3fa9ee3cd 
  src/slave/containerizer/provisioner/docker/remote_puller.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/store.cpp 
cbb67686d45513f0395a0cf1bc5c43cb4935adae 
  src/slave/flags.hpp f76f0f6a0c7a01c7f061a225d7f6ef52be0ee7b5 
  src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 
  src/tests/containerizer/provisioner_docker_tests.cpp 
d895eb9d0723e52cff8b21ef2deeaef1911d019c 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38901: Serialize Docker Image Spec as Protobuf

[Gilbert Song]

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

(Updated Oct. 2, 2015, 1:10 p.m.)


Review request for mesos, Jojy Varghese and Timothy Chen.


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


Repository: mesos


Description
---

Serialize Docker Image Spec as Protobuf


Diffs (updated)
-

  src/CMakeLists.txt 891f951f3107ece29b7923b7a3cc414e2ea56983 
  src/Makefile.am f060998bb08cdb071db5a2e85dfbad805dab45e9 
  src/slave/containerizer/provisioner/docker/message.proto 
bbac2e6c1f40a7ca3f9227baca56a44cd43f58c6 
  src/slave/containerizer/provisioner/docker/spec.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/spec.cpp PRE-CREATION 
  src/tests/containerizer/provisioner_docker_tests.cpp 
d895eb9d0723e52cff8b21ef2deeaef1911d019c 

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


Testing
---

make check (ubuntu 14.04 + clang++-3.6)


Thanks,

Gilbert Song

Re: Review Request 38844: Added unit tests for Call validation in Agent

[Isabel Jimenez]

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

(Updated Oct. 2, 2015, 6:53 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


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


Repository: mesos


Description
---

Unit tests for Call validation in Agent.


Diffs (updated)
-

  src/tests/executor_http_api_tests.cpp c2c05f4 

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


Testing
---

make check


Thanks,

Isabel Jimenez

Re: Review Request 38961: Simplified resources initialization.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38961]

All tests passed.

- Mesos ReviewBot


On Oct. 2, 2015, 5:10 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38961/
> ---
> 
> (Updated Oct. 2, 2015, 5:10 p.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Simplified resources initialization.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 31ca14621962f8851920b733ba0dec9f1afe8e79 
> 
> Diff: https://reviews.apache.org/r/38961/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38816: add test case for docker remotePuller

[Gilbert Song]

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

(Updated Oct. 2, 2015, 11:59 a.m.)


Review request for mesos, Jojy Varghese and Timothy Chen.


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


Repository: mesos


Description
---

add test case for docker remotePuller


Diffs (updated)
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
d895eb9d0723e52cff8b21ef2deeaef1911d019c 

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


Testing
---

make check (ubuntu 14.04) 

libevent and ssl enabled in configuration (e.g., '../configure --disable-java 
--disable-python --enable-libevent --enable-ssl --enable-debug')


Thanks,

Gilbert Song

Re: Review Request 38900: Update command executor to support rootfs.

[Jie Yu]

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



src/launcher/executor.cpp (line 97)


Not yours, but can you use `_override` for the parameter?



src/launcher/executor.cpp (lines 186 - 195)


First, I would like all the preparation to be done before the fork (because 
after the fork, we technically cannot do any async signal unsafe work).

Second, I don't think you need to search for the `rootVolume`. You can just 
simply assume it exists and fail if it doesn't. So the logic should be:
```
Option rootfs;

if (sandboxDirectory is some) {
  // This is the case where the user specifies a
  // rootfs for the command.
  if (current user is not root) {
error message
abort();
  }
  
  if (.rootfs does not exist) {
error message
abort();
  }
  
  rootfs = path::join(...);
  sandbox = path::join(...);
  
  if (sandbox does not exist) {
mkdir sandbox
  }
  
  mount the sandbox
}

...
fork()
...

// In the child.
if (rootfs.isSome()) {
  chroot(rootfs)
  chdir(sandboxDirectory)
  su(user)
}
```



src/launcher/executor.cpp (lines 709 - 710)


Please mention that these flags are only meaningful if rootfs is used for 
the user command.



src/slave/slave.cpp (lines 3227 - 3234)


I don't think you need to do this check for mac (it's a silent ignore 
anyway). The filesystem isolator is going to reject the task during launch on 
Mac.



src/slave/slave.cpp (lines 3316 - 3325)


Is it possible to use the non-shell version so that you don't need to worry 
about escaping?


- Jie Yu


On Oct. 2, 2015, 12:16 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38900/
> ---
> 
> (Updated Oct. 2, 2015, 12:16 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3428
> https://issues.apache.org/jira/browse/MESOS-3428
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update command executor to support rootfs.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
>   src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341 
>   src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 4c1419290645ad4c44360a81618a6cea7ad190df 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b904b2d88e9b62fa4ba312c4569a4d89b0dc6052 
>   src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c 
> 
> Diff: https://reviews.apache.org/r/38900/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38814: add test cases for sha512 digest verifier

[Jojy Varghese]

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



3rdparty/libprocess/src/tests/digest_tests.cpp (line 130)


You might have to take care of the else case also.



3rdparty/libprocess/src/tests/digest_tests.cpp (line 158)


You will have to guard the call to "digest" with 
DigestTypeTraits::is_implemented.


- Jojy Varghese


On Oct. 2, 2015, 6:29 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38814/
> ---
> 
> (Updated Oct. 2, 2015, 6:29 p.m.)
> 
> 
> Review request for mesos, Jojy Varghese and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> add test cases for sha512 digest verifier
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38814/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu 14.04)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 38844: Added unit tests for Call validation in Agent

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38618, 38577, 38844]

All tests passed.

- Mesos ReviewBot


On Oct. 2, 2015, 6:53 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38844/
> ---
> 
> (Updated Oct. 2, 2015, 6:53 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Unit tests for Call validation in Agent.
> 
> 
> Diffs
> -
> 
>   src/tests/executor_http_api_tests.cpp c2c05f4 
> 
> Diff: https://reviews.apache.org/r/38844/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38963: Added initial draft of networking user-doc.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [38963]

Failed command: ./support/apply-review.sh -n -r 38963

Error:
 2015-10-02 19:19:14 URL:https://reviews.apache.org/r/38963/diff/raw/ 
[10822/10822] -> "38963.patch" [1]
error: missing binary patch data for 'docs/images/networking-architecture.png'
error: binary patch does not apply to 'docs/images/networking-architecture.png'
error: docs/images/networking-architecture.png: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On Oct. 2, 2015, 6:08 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38963/
> ---
> 
> (Updated Oct. 2, 2015, 6:08 p.m.)
> 
> 
> Review request for mesos, Connor Doyle and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3561
> https://issues.apache.org/jira/browse/MESOS-3561
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial draft of networking user-doc.
> 
> 
> Diffs
> -
> 
>   docs/images/networking-architecture.png PRE-CREATION 
>   docs/networking.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38963/diff/
> 
> 
> Testing
> ---
> 
> Markdown rendering at 
> https://github.com/karya0/mesos/blob/net-user-doc/docs/networking.md
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Review Request 38967: Added functionality to generate Java V1 Protobufs and insert them into the existing JAR for now

[Anand Mazumdar]

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

Review request for mesos and Joris Van Remoortere.


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


Repository: mesos


Description
---

For now , till we find out a way on how we go about distributing the V1 
protobufs. This fail-safe change dumps them into the existing MESOS JAR(Java). 
This would save anyone the trouble of generating them before using the new API.


Diffs
-

  src/Makefile.am 8aa456611dd5405336dd7b0c19ba4a942ea1c805 

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


Testing
---

Verified Java JAR has the required protobufs.


Thanks,

Anand Mazumdar

Re: Review Request 38963: Added initial draft of networking user-doc.

[Niklas Nielsen]

> On Oct. 2, 2015, 11:31 a.m., Joseph Wu wrote:
> > A few things you're missing (haven't read the doc yet):
> > 
> > * A link to this from `home.md`.
> > * You need to generate this diff manually (via `git diff --binary`) and 
> > post it.  The image won't be included otherwise.  (ReviewBot is going to 
> > say "bad patch" soon.)
> > * Make sure you render this using 
> > https://github.com/mesosphere/mesos-website-container  (Note that the image 
> > will *not* show up, until https://issues.apache.org/jira/browse/MESOS-3183 
> > is resolved).

+1 :) Thanks Joseph


- Niklas


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


On Oct. 2, 2015, 11:08 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38963/
> ---
> 
> (Updated Oct. 2, 2015, 11:08 a.m.)
> 
> 
> Review request for mesos, Connor Doyle and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3561
> https://issues.apache.org/jira/browse/MESOS-3561
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial draft of networking user-doc.
> 
> 
> Diffs
> -
> 
>   docs/images/networking-architecture.png PRE-CREATION 
>   docs/networking.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38963/diff/
> 
> 
> Testing
> ---
> 
> Markdown rendering at 
> https://github.com/karya0/mesos/blob/net-user-doc/docs/networking.md
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 38967: Added functionality to generate Java V1 Protobufs and insert them into the existing JAR for now

[Isabel Jimenez]

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



src/Makefile.am (line 225)


Could you also add a fix for v1 python proto generation?


- Isabel Jimenez


On Oct. 2, 2015, 10:27 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38967/
> ---
> 
> (Updated Oct. 2, 2015, 10:27 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3575
> https://issues.apache.org/jira/browse/MESOS-3575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For now , till we find out a way on how we go about distributing the V1 
> protobufs. This fail-safe change dumps them into the existing MESOS 
> JAR(Java). This would save anyone the trouble of generating them before using 
> the new API.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8aa456611dd5405336dd7b0c19ba4a942ea1c805 
> 
> Diff: https://reviews.apache.org/r/38967/diff/
> 
> 
> Testing
> ---
> 
> Verified Java JAR has the required protobufs.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 38844: Added unit tests for Call validation in Agent

[Anand Mazumdar]

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

Ship it!


LGTM

- Anand Mazumdar


On Oct. 2, 2015, 6:53 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38844/
> ---
> 
> (Updated Oct. 2, 2015, 6:53 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Unit tests for Call validation in Agent.
> 
> 
> Diffs
> -
> 
>   src/tests/executor_http_api_tests.cpp c2c05f4 
> 
> Diff: https://reviews.apache.org/r/38844/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38579: Refactored registry client

[Ben Mahler]

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


Thanks for taking this on Jojy! This is going to be a great exercise in sending 
out small patches so we land things quickly :)

First thing is that this review depends on the addition of new functionality in 
https://reviews.apache.org/r/38443/ . It's easier for the reviewers to help you 
land your cleanups without it being blocked on the new functionality, can you 
remove the dependency?

I wasn't able to do a complete pass on everything since there is a lot of code 
here. But to get things started, I've done an initial pass and made a number of 
comments below. Let's start by addressing them and using small independent 
patches for each cleanup, that way we can make progress without having to clean 
everything up all in one patch.

Much appreciated!!


src/slave/containerizer/provisioner/docker/registry_client.hpp (lines 36 - 40)


Hm.. any reason the registry client is nested within the containerizer?

A registry client seems like a general abstraction, should it live up 
alongside the `Docker` abstraction within src/docker?



src/slave/containerizer/provisioner/docker/registry_client.hpp (line 46)


Can we avoid the redundant naming here? We currently have 
`docker::registry::RegistryClient`, how about `docker::registry::Client` or 
`docker::RegistryClient`?



src/slave/containerizer/provisioner/docker/registry_client.hpp (lines 53 - 54)


Hm.. can you fix the style on your comments? There should be a space after 
the `//`.



src/slave/containerizer/provisioner/docker/registry_client.hpp (line 64)


Why ManifestResponse instead of just Manifest..?



src/slave/containerizer/provisioner/docker/registry_client.hpp (line 67)


How about: s/fsLayerInfoList/layers/



src/slave/containerizer/provisioner/docker/registry_client.hpp (line 71)


Authentication or authorization? Can you avoid the abbreviation? This is 
what we've done for the rest of our authentication/authorization code.



src/slave/containerizer/provisioner/docker/registry_client.hpp (lines 75 - 86)


As it stands these comments don't seem to be adding any value over what the 
code expresses, should we remove them? Or is there anything that you think they 
should call out that the reader can't see from the variable names?



src/slave/containerizer/provisioner/docker/registry_client.hpp (line 100)


authentication or authorization, or both? Can you do a pass for this 
abbreviation?



src/slave/containerizer/provisioner/docker/registry_client.hpp (line 147)


We've been trying to eliminate static non-PODs, could you replace this with 
a static function?



src/slave/containerizer/provisioner/docker/registry_client.cpp (lines 62 - 65)


Hm.. why did you need this additional static create? Looking at the 
implementation it seems as though you only need `RegistryClient::create`.



src/slave/containerizer/provisioner/docker/registry_client.cpp (line 106)


How about s/getManifestResponse/getManifest/



src/slave/containerizer/provisioner/docker/registry_client.cpp (lines 122 - 130)


Generally our pattern for process wrappers is to have the wrapper 
constructor allocate the process. Here it is getting injected. Can you refactor 
this to make it consistent? Ditto for TokenManager.

This means avoiding RegistryClientProcess::create and creating the token 
manager in here, then passing everything we need into the RegistryClient 
constructor, which is a simple pass through to the RegistryClientProcess 
constructor.

Looking at the TokenManager code, I'm confused why TokenManager::create 
exists, and why there is a Try. It looks like token manager construction never 
returns an error?

If we clean up the token manager creation code in the same way as I'm 
suggesting here, we can make this a lot simpler as well. :)



src/slave/containerizer/provisioner/docker/registry_client.cpp (line 193)


We try to avoid abbreviations for names like this, can you do a pass to 
clean them up?



src/slave/containerizer/provisioner/docker/registry_client.cpp (line 231)

Re: Review Request 38941: Moved structs outside RegistryClient

[Ben Mahler]

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


Thanks for the small patch! I would have liked to ship this but I can't due to 
the dependencies :(


src/slave/containerizer/provisioner/docker/registry_client.hpp (line 62)


Why ManifestResponse instead of just Manifest?


- Ben Mahler


On Oct. 2, 2015, 4 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38941/
> ---
> 
> (Updated Oct. 2, 2015, 4 a.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved:
>   - ManifestResponse
>   - FilesystemLayerInfo
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/provisioner/docker/registry_client.hpp 
> 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185 
>   src/slave/containerizer/provisioner/docker/registry_client.cpp 
> c2040b48ea43fdb29766994c244273d3fa9ee3cd 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> d895eb9d0723e52cff8b21ef2deeaef1911d019c 
> 
> Diff: https://reviews.apache.org/r/38941/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38608: Added an http::Connection for connection re-use and pipelining.

[Ben Mahler]

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

(Updated Oct. 2, 2015, 11:49 p.m.)


Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.


Changes
---

Close the socket upon send error.


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


Repository: mesos


Description
---

In order to support connection re-use and pipelining of requests on a shared 
connection, this introduces the notion of an http::Connection.


Diffs (updated)
-

  3rdparty/libprocess/include/process/http.hpp 
ba3f0bc7df33795e332c374fbad04106b9d56416 
  3rdparty/libprocess/src/http.cpp d9925333f25491f92495bf11315237f54a0a2015 
  3rdparty/libprocess/src/tests/http_tests.cpp 
c380f356548cf9f5491044bccabcd9c66ad5f55a 

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


Testing
---

Added tests.


Thanks,

Ben Mahler

Re: Review Request 38608: Added an http::Connection for connection re-use and pipelining.

[Ben Mahler]

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

(Updated Oct. 3, 2015, 12:04 a.m.)


Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.


Changes
---

Fixed a compilation issue on OS X.


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


Repository: mesos


Description
---

In order to support connection re-use and pipelining of requests on a shared 
connection, this introduces the notion of an http::Connection.


Diffs (updated)
-

  3rdparty/libprocess/include/process/http.hpp 
ba3f0bc7df33795e332c374fbad04106b9d56416 
  3rdparty/libprocess/src/http.cpp d9925333f25491f92495bf11315237f54a0a2015 
  3rdparty/libprocess/src/tests/http_tests.cpp 
c380f356548cf9f5491044bccabcd9c66ad5f55a 

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


Testing
---

Added tests.


Thanks,

Ben Mahler

Re: Review Request 38967: Added functionality to generate Java V1 Protobufs and insert them into the existing JAR for now

[Anand Mazumdar]

> On Oct. 2, 2015, 10:34 p.m., Isabel Jimenez wrote:
> > src/Makefile.am, line 225
> > 
> >
> > Could you also add a fix for v1 python proto generation?

Checkout my comment here : https://issues.apache.org/jira/browse/MESOS-3575 

This patch only takes care of Java for now.


- Anand


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


On Oct. 2, 2015, 10:27 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38967/
> ---
> 
> (Updated Oct. 2, 2015, 10:27 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3575
> https://issues.apache.org/jira/browse/MESOS-3575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For now , till we find out a way on how we go about distributing the V1 
> protobufs. This fail-safe change dumps them into the existing MESOS 
> JAR(Java). This would save anyone the trouble of generating them before using 
> the new API.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8aa456611dd5405336dd7b0c19ba4a942ea1c805 
> 
> Diff: https://reviews.apache.org/r/38967/diff/
> 
> 
> Testing
> ---
> 
> Verified Java JAR has the required protobufs.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 38901: Serialize Docker Image Spec as Protobuf

[Timothy Chen]

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



src/slave/containerizer/provisioner/docker/spec.hpp (line 37)


End all comments with a period.



src/tests/containerizer/provisioner_docker_tests.cpp (line 321)


No need to abort, just use ASSERT_SOME



src/tests/containerizer/provisioner_docker_tests.cpp (line 357)


How about call it invalid instead of insufficient



src/tests/containerizer/provisioner_docker_tests.cpp (line 361)


What's invalid about this JSON?



src/tests/containerizer/provisioner_docker_tests.cpp (line 368)


ditto


- Timothy Chen


On Oct. 2, 2015, 8:10 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38901/
> ---
> 
> (Updated Oct. 2, 2015, 8:10 p.m.)
> 
> 
> Review request for mesos, Jojy Varghese and Timothy Chen.
> 
> 
> Bugs: MESOS-2972
> https://issues.apache.org/jira/browse/MESOS-2972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Serialize Docker Image Spec as Protobuf
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 891f951f3107ece29b7923b7a3cc414e2ea56983 
>   src/Makefile.am f060998bb08cdb071db5a2e85dfbad805dab45e9 
>   src/slave/containerizer/provisioner/docker/message.proto 
> bbac2e6c1f40a7ca3f9227baca56a44cd43f58c6 
>   src/slave/containerizer/provisioner/docker/spec.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/spec.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> d895eb9d0723e52cff8b21ef2deeaef1911d019c 
> 
> Diff: https://reviews.apache.org/r/38901/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu 14.04 + clang++-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 38967: Added functionality to generate Java V1 Protobufs and insert them into the existing JAR for now

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38967]

All tests passed.

- Mesos ReviewBot


On Oct. 2, 2015, 10:27 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38967/
> ---
> 
> (Updated Oct. 2, 2015, 10:27 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3575
> https://issues.apache.org/jira/browse/MESOS-3575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For now , till we find out a way on how we go about distributing the V1 
> protobufs. This fail-safe change dumps them into the existing MESOS 
> JAR(Java). This would save anyone the trouble of generating them before using 
> the new API.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8aa456611dd5405336dd7b0c19ba4a942ea1c805 
> 
> Diff: https://reviews.apache.org/r/38967/diff/
> 
> 
> Testing
> ---
> 
> Verified Java JAR has the required protobufs.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 38416: Allow HTTP response codes to be checked with code.

[Timothy Chen]

> On Sept. 25, 2015, 10:52 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/http.hpp, lines 59-60
> > 
> >
> > How about just status()?
> > 
> > We can also avoid Option here by just returning the number value with 
> > no message? That seems cleaner?

Not sure how this is useful if it only returns the number value?


- Timothy


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


On Sept. 25, 2015, 10:38 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38416/
> ---
> 
> (Updated Sept. 25, 2015, 10:38 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, Jie Yu, Jojy Varghese, 
> and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3429
> https://issues.apache.org/jira/browse/MESOS-3429
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow HTTP response codes to be checked with code.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> fbd6cf7967173495875a8ea90ed28ade88b982a2 
>   3rdparty/libprocess/src/decoder.hpp 
> 67a5135f302153e376e8dfe8db82aa0b15449389 
>   3rdparty/libprocess/src/http.cpp 9ad613a16c379b6d76a9a0f8d6160fe23a182fd4 
>   3rdparty/libprocess/src/process.cpp 
> 4afa30569b4d235637b49a624602e6b199c32e0e 
>   3rdparty/libprocess/src/tests/benchmarks.cpp 
> 97c81b8e61a75771e5bf7d46505cec4e0c0f404a 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> d0b9399d38fa284466a012a21080b1d9007af98b 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 1023500ac2e3c55be955c4686e7f720eba6b4cc9 
> 
> Diff: https://reviews.apache.org/r/38416/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

> On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote:
> > src/slave/containerizer/provisioner/docker/registry_client.hpp, lines 75-86
> > 
> >
> > As it stands these comments don't seem to be adding any value over what 
> > the code expresses, should we remove them? Or is there anything that you 
> > think they should call out that the reader can't see from the variable 
> > names?

They only serve doxygen. I thought all new code(header files) has to be 
commented for generating doxygen based docs.


> On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote:
> > src/slave/containerizer/provisioner/docker/registry_client.hpp, line 148
> > 
> >
> > We've been trying to eliminate static non-PODs, could you replace this 
> > with a static function?

I have got rid of this constant in the subsequent patch 
(https://reviews.apache.org/r/38580)


> On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote:
> > src/slave/containerizer/provisioner/docker/registry_client.cpp, lines 63-66
> > 
> >
> > Hm.. why did you need this additional static create? Looking at the 
> > implementation it seems as though you only need `RegistryClient::create`.

Not sure I follow. This create is the factory for the process class. If there 
is an error creating the Process class, this should help in catching that. Isnt 
it?


> On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote:
> > src/slave/containerizer/provisioner/docker/registry_client.cpp, line 197
> > 
> >
> > We try to avoid abbreviations for names like this, can you do a pass to 
> > clean them up?

I have taken liberty when using commonly used variable names. creds, params etc 
are example of that 
(http://lxr.free-electrons.com/source/include/linux/sched.h). I can change it 
if you feel strongly about it. Also, in the context of Docker its always 
authentication.


> On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote:
> > src/slave/containerizer/provisioner/docker/registry_client.cpp, line 594
> > 
> >
> > Strange that size_t is used here which I'm guessing is why you stuck 
> > the post-decrement in the loop condition. That's a bit tricky, can you just 
> > use an rbegin iterator here?

i would have but i need the index number.


> On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote:
> > src/slave/containerizer/provisioner/docker/registry_client.cpp, lines 
> > 548-549
> > 
> >
> > Why is this a member function? Can it be a standalone function for 
> > converting a response into a manifest?

should i leave it as a lambda then?


> On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote:
> > src/slave/containerizer/provisioner/docker/registry_client.cpp, lines 
> > 716-718
> > 
> >
> > Why only this validation? Should 'path' be a 'Path'?

No since this is not a filepath but a url path


> On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote:
> > src/slave/containerizer/provisioner/docker/registry_client.cpp, lines 
> > 724-741
> > 
> >
> > Is it acceptable to hold the whole blob in memory like this?? How big 
> > can these blobs be?

Thats a good point. Ideally we should have a buffered socker reader. Would 
appreciate if you could point me to an example of buffered reader from http.


- Jojy


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


On Oct. 2, 2015, 12:24 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38579/
> ---
> 
> (Updated Oct. 2, 2015, 12:24 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Moved ManifestResponse struct from RegistryClient to namespace.
> - Cleanup
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/provisioner/docker/registry_client.hpp 
> 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185 
>   src/slave/containerizer/provisioner/docker/registry_client.cpp 
> c2040b48ea43fdb29766994c244273d3fa9ee3cd 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> d895eb9d0723e52cff8b21ef2deeaef1911d019c 
> 
> Diff: https://reviews.apache.org/r/38579/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
>