Re: Review Request 39258: Add filesystem isolator with command executor test.

[Timothy Chen]

> On Nov. 3, 2015, 1:30 a.m., Jie Yu wrote:
> > src/tests/containerizer/filesystem_isolator_tests.cpp, line 286
> > 
> >
> > any reason you use 'copy'. Can we do bind to speed up tests?
> 
> Timothy Chen wrote:
> it failed when the filesystem isolator tries to mkdir sandbox
> 
> Timothy Chen wrote:
> i think alternatifely i can also let Rootfs clasd create a sandbox mount 
> point too

I tried to use BindBackend, but it then failed to create mount point for the 
old root in the /tmp directory of the new root in fs.cpp as Bind simply made 
the whole filesystem read-only. I think I'll use Copy for now.


- Timothy


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


On Nov. 2, 2015, 7:05 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39258/
> ---
> 
> (Updated Nov. 2, 2015, 7:05 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add filesystem isolator with command executor test.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 39008f620183d242407fea5377bfceffc57b 
>   src/tests/containerizer/provisioner.hpp 
> 507e1413470ef4f36ead657203073115d5324bef 
> 
> Diff: https://reviews.apache.org/r/39258/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 39276: Fixed a bug in which under certains circumstances HTTP 1.1 Pipelining is not respected.

[Alexander Rojas]

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

(Updated Nov. 4, 2015, 12:22 p.m.)


Review request for mesos, Anand Mazumdar, Bernd Mathiske, and Till Toenshoff.


Changes
---

Rebasing.


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


Repository: mesos


Description
---

When using the same socket to send multiple HTTP requests to different actors. 
If the actor responsible for handling the first request is stuck handling 
another event while a subsequent request can reply immediatly, the order of the 
responses is altered violating HTTP Pipelining.

This patch fixes that problem enforcing that every response is sent in the 
order the corresponding request arrived. It also adds a test to reproduce the 
issue and verify the fix works.


Diffs (updated)
-

  3rdparty/libprocess/include/process/event.hpp 
16ddbd77afa6efdf6bad201aa497ee102aa863ae 
  3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313 
  3rdparty/libprocess/src/tests/http_tests.cpp 
7eb4ef187b2cb358c370d0381db65b8e18668bab 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 37999: Implemented http::AuthenticatorManager

[Alexander Rojas]

> On Oct. 21, 2015, 1:03 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/include/process/authenticator.hpp, line 52
> > 
> >
> > Duplicate text.

I used a separate line because Doxygen uses the first line as a brief summary 
(It is also in our [style 
guide](https://github.com/apache/mesos/blob/c3940cd4da29eb6539096c6ec6dcab1a70993c42/docs/doxygen-style-guide.md#source-code-documentation-syntax)).
 However, given that I haven't seen the rendered result. I will remove this 
line.


> On Oct. 21, 2015, 1:03 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/include/process/authenticator.hpp, line 107
> > 
> >
> > s/are/is (contents is singular)
> > 
> > s/a HTTP/an HTTP 
> > 
> > (Please check all other places. If "a" is followed by "H" in an 
> > abbreviation, which is therefore pronounced "aytsh", this constitutes an 
> > "a" followed by a vowel sound, so it becomes "an)

Sorry, _contents_ is 
[plural](http://english.stackexchange.com/questions/13556/content-or-contents). 
Though there are different meanings of the word _content_.


> On Oct. 21, 2015, 1:03 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/src/process.cpp, line 793
> > 
> >
> > Suggestion: break this up into two calls.
> > 
> > removeAllAuthenticators()
> > removeAuthenticator()

I will let you decide this one with till, because he suggested the exact 
opposite (which has also been suggested by other reviewers), in his Sept. 10, 
2015, 8:06 a.m. review (see above).


- Alexander


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


On Nov. 4, 2015, 12:25 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> ---
> 
> (Updated Nov. 4, 2015, 12:25 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3231
> https://issues.apache.org/jira/browse/MESOS-3231
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduces the authenticator manager, which is a class which handles the 
> actual authentication procedure during the execution of 
> `ProcessManager::handle()` and it also takes care of the life cycle of 
> instances of http::Authenticator.
> 
> No tests are added at this point since no public API is generated, the goal 
> of this patch is to implement the manager and verify nothing breaks 
> afterwards. Authenticator manager tests proper come in a latter patch.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/event.hpp 
> 16ddbd77afa6efdf6bad201aa497ee102aa863ae 
>   3rdparty/libprocess/include/process/http.hpp 
> 90c9be122ee0c402b806d70fc818e3c03b15101a 
>   3rdparty/libprocess/src/process.cpp 
> a94712b9ac3b60fb047b3a5a4d84a56fa4d02313 
> 
> Diff: https://reviews.apache.org/r/37999/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 37999: Implemented http::AuthenticatorManager

[Alexander Rojas]

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

(Updated Nov. 4, 2015, 12:25 p.m.)


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


Changes
---

Addresses Bernd's review issues.


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


Repository: mesos


Description
---

Introduces the authenticator manager, which is a class which handles the actual 
authentication procedure during the execution of `ProcessManager::handle()` and 
it also takes care of the life cycle of instances of http::Authenticator.

No tests are added at this point since no public API is generated, the goal of 
this patch is to implement the manager and verify nothing breaks afterwards. 
Authenticator manager tests proper come in a latter patch.


Diffs (updated)
-

  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION 
  3rdparty/libprocess/include/process/event.hpp 
16ddbd77afa6efdf6bad201aa497ee102aa863ae 
  3rdparty/libprocess/include/process/http.hpp 
90c9be122ee0c402b806d70fc818e3c03b15101a 
  3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 37999: Implemented http::AuthenticatorManager

[Alexander Rojas]

> On Oct. 21, 2015, 1:46 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/src/process.cpp, line 885
> > 
> >
> > Why are we doing this? Why isn't it a failure if there is no success 
> > and also no challenge? Please comment in the source code on this.
> > 
> > My take is that if these authenticators would have liked to pose a 
> > challenge, they would already have done so.

Change the comment to:

```c++
// At this point on of the following is true:
// 1. Authentication succeeded, in which case the principal was
//already returned and this code is not executed.
// 2. It is a multi-step authentication protocol and challenges is
//not empty, because they are filled with steps.
// 3. Authentication failed for any reason, e.g. credentials were not
//provided in which case the authentication is reset to the first
//step.
```

Which makes clear why is this done here.


> On Oct. 21, 2015, 1:46 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/src/process.cpp, line 886
> > 
> >
> > s/with those/those with

Comment changed completely.


- Alexander


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


On Nov. 4, 2015, 12:25 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> ---
> 
> (Updated Nov. 4, 2015, 12:25 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3231
> https://issues.apache.org/jira/browse/MESOS-3231
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduces the authenticator manager, which is a class which handles the 
> actual authentication procedure during the execution of 
> `ProcessManager::handle()` and it also takes care of the life cycle of 
> instances of http::Authenticator.
> 
> No tests are added at this point since no public API is generated, the goal 
> of this patch is to implement the manager and verify nothing breaks 
> afterwards. Authenticator manager tests proper come in a latter patch.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/event.hpp 
> 16ddbd77afa6efdf6bad201aa497ee102aa863ae 
>   3rdparty/libprocess/include/process/http.hpp 
> 90c9be122ee0c402b806d70fc818e3c03b15101a 
>   3rdparty/libprocess/src/process.cpp 
> a94712b9ac3b60fb047b3a5a4d84a56fa4d02313 
> 
> Diff: https://reviews.apache.org/r/37999/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38000: Added an API for libprocess users to interact with http::AuthenticatorManager

[Alexander Rojas]

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

(Updated Nov. 4, 2015, 12:27 p.m.)


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


Changes
---

Adresses issues from Bernd's review.
Rebasing.


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


Repository: mesos


Description
---

Adds functions which allows libprocess users to add handlers which require 
authentication as well as functions to install and remove authenticators.

Includes tests.


Diffs (updated)
-

  3rdparty/libprocess/include/process/process.hpp 
8b086f296c80a43be2edaf496a04dadf0c64251a 
  3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313 
  3rdparty/libprocess/src/tests/http_tests.cpp 
7eb4ef187b2cb358c370d0381db65b8e18668bab 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 38000: Added an API for libprocess users to interact with http::AuthenticatorManager

[Alexander Rojas]

> On Oct. 21, 2015, 2:12 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/include/process/process.hpp, line 83
> > 
> >
> > Why aren't we using shared pointers here?

Last time I asked, we don't use std smart pointers on public stout and 
libprocess API's but we are allowed to use them in internal code.


- Alexander


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


On Nov. 4, 2015, 12:27 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38000/
> ---
> 
> (Updated Nov. 4, 2015, 12:27 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3233
> https://issues.apache.org/jira/browse/MESOS-3233
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds functions which allows libprocess users to add handlers which require 
> authentication as well as functions to install and remove authenticators.
> 
> Includes tests.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/process.hpp 
> 8b086f296c80a43be2edaf496a04dadf0c64251a 
>   3rdparty/libprocess/src/process.cpp 
> a94712b9ac3b60fb047b3a5a4d84a56fa4d02313 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 7eb4ef187b2cb358c370d0381db65b8e18668bab 
> 
> Diff: https://reviews.apache.org/r/38000/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38094: Added implementation of Http Basic authentication scheme.

[Alexander Rojas]

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

(Updated Nov. 4, 2015, 12:29 p.m.)


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


Changes
---

Rebasing.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 404a7438e7cd53d9295fe6025b5af5fb83df939b 
  3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION 
  3rdparty/libprocess/src/CMakeLists.txt 
fb9bd04832779ac43151f2feb3dfbf58cb996434 
  3rdparty/libprocess/src/authenticator.cpp PRE-CREATION 
  3rdparty/libprocess/src/tests/http_tests.cpp 
7eb4ef187b2cb358c370d0381db65b8e18668bab 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 39707: Minor code refactor for fetcher.cpp.

[Bernd Mathiske]

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



src/launcher/fetcher.cpp (line 297)


Or, avoiding a string constant: 

CHECK_FALSE(item.cache_filename().empty());


- Bernd Mathiske


On Oct. 27, 2015, 6:45 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39707/
> ---
> 
> (Updated Oct. 27, 2015, 6:45 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made some small refactoring as I was reading through the fetcher code to make 
> the groking easy. No functional changes.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 8fb6c83981a141df9c0a8a6f8267230bef64f218 
> 
> Diff: https://reviews.apache.org/r/39707/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

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.

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 39043: Added support for HTTP Authentication in Mesos.

[Alexander Rojas]

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

(Updated Nov. 4, 2015, 12:33 p.m.)


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


Changes
---

Addresses issues in Jian Qiu's review.


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


Repository: mesos


Description
---

1. Adds a flag to load an HTTP Authenticator module from the flags.
2. If provided, uses the credentials file to initialize the default HTTP 
Authenticator.
3. Updates the existing endpoints which implement their own basic HTTP 
authenticator with the libprocess one.
4. Updates one test which expected the wrong results since now credentials are 
checked before the body of the request.


Diffs (updated)
-

  src/master/constants.hpp 51d477444c4d1a2c9dd2f32164ebffbb1fd5c8c2 
  src/master/constants.cpp 2b66b27783f18930010ee912e9977ea1647eba09 
  src/master/flags.hpp 5fd5d502697b2edc22ae98a5a8e361bf62bf8bb6 
  src/master/flags.cpp 0285ce70cefca09e81ef7137968d024e297fec87 
  src/master/http.cpp 9d20346460b4f9659aacba3fcb950ab564ba9d0d 
  src/master/master.hpp b19f23be135d191ce119cf87fd25c4d6746faed3 
  src/master/master.cpp cbae27e7a4059a72bc69e152ec8adaf4ef725965 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 39799: Allow hdfs URLs in the HDFS wrapper API.

[Bernd Mathiske]

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

Ship it!



src/hdfs/hdfs.hpp (line 205)


s/a/an


- Bernd Mathiske


On Nov. 3, 2015, 7:21 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39799/
> ---
> 
> (Updated Nov. 3, 2015, 7:21 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Timothy Chen.
> 
> 
> Bugs: MESOS-3602
> https://issues.apache.org/jira/browse/MESOS-3602
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow hdfs URLs in the HDFS wrapper API.
> 
> 
> Diffs
> -
> 
>   src/hdfs/hdfs.hpp 18f17231b92b84d0b0e4e15837d0e44ce8758cdf 
> 
> Diff: https://reviews.apache.org/r/39799/diff/
> 
> 
> Testing
> ---
> 
> make check. Manual testing with a framework that uses the fetcher with HDFS 
> urls.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 39043: Added support for HTTP Authentication in Mesos.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [39276, 37998, 39472, 37999, 38000, 38094, 38627, 38950]

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

Error:
 2015-11-04 11:50:44 URL:https://reviews.apache.org/r/38950/diff/raw/ 
[18307/18307] -> "38950.patch" [1]
error: patch failed: src/Makefile.am:476
error: src/Makefile.am: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On Nov. 4, 2015, 11:33 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39043/
> ---
> 
> (Updated Nov. 4, 2015, 11:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Isabel Jimenez, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3756
> https://issues.apache.org/jira/browse/MESOS-3756
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> 1. Adds a flag to load an HTTP Authenticator module from the flags.
> 2. If provided, uses the credentials file to initialize the default HTTP 
> Authenticator.
> 3. Updates the existing endpoints which implement their own basic HTTP 
> authenticator with the libprocess one.
> 4. Updates one test which expected the wrong results since now credentials 
> are checked before the body of the request.
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp 51d477444c4d1a2c9dd2f32164ebffbb1fd5c8c2 
>   src/master/constants.cpp 2b66b27783f18930010ee912e9977ea1647eba09 
>   src/master/flags.hpp 5fd5d502697b2edc22ae98a5a8e361bf62bf8bb6 
>   src/master/flags.cpp 0285ce70cefca09e81ef7137968d024e297fec87 
>   src/master/http.cpp 9d20346460b4f9659aacba3fcb950ab564ba9d0d 
>   src/master/master.hpp b19f23be135d191ce119cf87fd25c4d6746faed3 
>   src/master/master.cpp cbae27e7a4059a72bc69e152ec8adaf4ef725965 
> 
> Diff: https://reviews.apache.org/r/39043/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: 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/
---

(Updated Nov. 4, 2015, 1:20 p.m.)


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


Changes
---

Rebase.


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


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 (updated)
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
PRE-CREATION 
  include/mesos/module/http_authenticator.hpp PRE-CREATION 
  src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289 
  src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db 
  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 39043: Added support for HTTP Authentication in Mesos.

[Alexander Rojas]

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

(Updated Nov. 4, 2015, 1:21 p.m.)


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


Changes
---

Forcing buildbot rebuild.


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


Repository: mesos


Description
---

1. Adds a flag to load an HTTP Authenticator module from the flags.
2. If provided, uses the credentials file to initialize the default HTTP 
Authenticator.
3. Updates the existing endpoints which implement their own basic HTTP 
authenticator with the libprocess one.
4. Updates one test which expected the wrong results since now credentials are 
checked before the body of the request.


Diffs (updated)
-

  src/master/constants.hpp 51d477444c4d1a2c9dd2f32164ebffbb1fd5c8c2 
  src/master/constants.cpp 2b66b27783f18930010ee912e9977ea1647eba09 
  src/master/flags.hpp 5fd5d502697b2edc22ae98a5a8e361bf62bf8bb6 
  src/master/flags.cpp 0285ce70cefca09e81ef7137968d024e297fec87 
  src/master/http.cpp 9d20346460b4f9659aacba3fcb950ab564ba9d0d 
  src/master/master.hpp b19f23be135d191ce119cf87fd25c4d6746faed3 
  src/master/master.cpp cbae27e7a4059a72bc69e152ec8adaf4ef725965 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 39005: stout: Added thread-safe replacement for strerror.

[Bernd Mathiske]

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



3rdparty/libprocess/3rdparty/stout/include/stout/os/strerror.hpp (line 36)


How is this relevant?



3rdparty/libprocess/3rdparty/stout/include/stout/os/strerror.hpp (line 53)


How would str_error run into errors "itself"? We don't seem to be handling 
any such erros here. But if there are any, shouldn't we? And if there aren't 
any, would the whole point be moot then?



3rdparty/libprocess/3rdparty/stout/include/stout/os/strerror.hpp (line 64)


a) This is an obscure reference. Pleae provide a pointer. b) Do you mean 
"similar to" or "attributed to" instead of "due to"? The latter indicates that 
you need to program this way because of something in heimdal's fcache code 
affecting strerror_r. That seems implausible.


- Bernd Mathiske


On Nov. 3, 2015, 2:52 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39005/
> ---
> 
> (Updated Nov. 3, 2015, 2:52 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3551
> https://issues.apache.org/jira/browse/MESOS-3551
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit also adds test cases for os::strerror (from stout) which can
> only be committed with a libprocess commit -- the corresponding Makefile
> lives there.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 
> 76e1674e08bbe65a4fdf86731823a61f231d6d12 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 67b142bbc2d80f40a1e893cc5813d58dd2aa8381 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/strerror.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
> 26e1377ee625748b7fdbec327fef9ac602535f83 
>   3rdparty/libprocess/3rdparty/stout/tests/os/strerror_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39005/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 39043: Added support for HTTP Authentication in Mesos.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39276, 37998, 39472, 37999, 38000, 38094, 38627, 38950, 39043]

All tests passed.

- Mesos ReviewBot


On Nov. 4, 2015, 12:21 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39043/
> ---
> 
> (Updated Nov. 4, 2015, 12:21 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Isabel Jimenez, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3756
> https://issues.apache.org/jira/browse/MESOS-3756
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> 1. Adds a flag to load an HTTP Authenticator module from the flags.
> 2. If provided, uses the credentials file to initialize the default HTTP 
> Authenticator.
> 3. Updates the existing endpoints which implement their own basic HTTP 
> authenticator with the libprocess one.
> 4. Updates one test which expected the wrong results since now credentials 
> are checked before the body of the request.
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp 51d477444c4d1a2c9dd2f32164ebffbb1fd5c8c2 
>   src/master/constants.cpp 2b66b27783f18930010ee912e9977ea1647eba09 
>   src/master/flags.hpp 5fd5d502697b2edc22ae98a5a8e361bf62bf8bb6 
>   src/master/flags.cpp 0285ce70cefca09e81ef7137968d024e297fec87 
>   src/master/http.cpp 9d20346460b4f9659aacba3fcb950ab564ba9d0d 
>   src/master/master.hpp b19f23be135d191ce119cf87fd25c4d6746faed3 
>   src/master/master.cpp cbae27e7a4059a72bc69e152ec8adaf4ef725965 
> 
> Diff: https://reviews.apache.org/r/39043/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

[Klaus Ma]

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

(Updated Nov. 4, 2015, 9:19 p.m.)


Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.


Changes
---

rebase the code


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


Repository: mesos


Description
---

Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
converts a `google::protobuf::Message` into a `JSON::Object`.
We should add the support for `google::protobuf::RepeatedPtrField` by 
introducing overloaded functions.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp f0e7870 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 

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


Testing
---

cd 3rdparty/libprocess/3rdparty/stout
./boostrap
./configure
make


Thanks,

Klaus Ma

Re: Review Request 38335: Add JSON::protobuf for google::protobuf::RepeatedPtrField

[Klaus Ma]

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

(Updated Nov. 4, 2015, 9:21 p.m.)


Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.


Changes
---

rebase the code & address comments


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


Repository: mesos


Description
---

Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
converts a `google::protobuf::Message` into a `JSON::Object`.
We should add the support for `google::protobuf::RepeatedPtrField` by 
introducing overloaded functions.


Diffs (updated)
-

  src/common/http.cpp f56d8a1 
  src/docker/executor.cpp d4c05c2 
  src/examples/persistent_volume_framework.cpp 176ac3d 
  src/launcher/executor.cpp 50b3c6e 
  src/master/contender.cpp c641305 
  src/master/http.cpp 9d20346 
  src/master/maintenance.cpp 5fe9358 
  src/master/registrar.cpp 1117232 
  src/slave/containerizer/fetcher.cpp e0d02d5 
  src/slave/containerizer/mesos/containerizer.cpp 9fd69c1 
  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 1911ba6 
  src/slave/monitor.cpp aa6e958 
  src/tests/containerizer/launch_tests.cpp de655ec 
  src/tests/containerizer/port_mapping_tests.cpp ae2c0e6 
  src/tests/fault_tolerance_tests.cpp f78a291 
  src/tests/master_contender_detector_tests.cpp 1da7f91 
  src/tests/master_maintenance_tests.cpp e89ce3b 
  src/tests/master_tests.cpp 8564405 
  src/tests/mesos.cpp ab2d85b 
  src/tests/monitor_tests.cpp 583e711 
  src/tests/reservation_endpoints_tests.cpp f5f9c48 
  src/tests/resources_tests.cpp 6584fc6 
  src/tests/scheduler_http_api_tests.cpp b6f6e91 
  src/tests/script.cpp bcc1fab 
  src/tests/slave_tests.cpp 91dbdba 
  src/usage/main.cpp 86fd796 

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


Testing
---

make
make check


Thanks,

Klaus Ma

Re: Review Request 39860: Quota: Replaced "slave" with "agent" in allocator logs.

[Alexander Rukletsov]

> On Nov. 2, 2015, 4:11 p.m., Vinod Kone wrote:
> > What's the plan for slave to agent rename in the logs? Having just the 
> > allocator output agent when the rest of the code base outputs slave will be 
> > confusing IMO.
> 
> Alexander Rukletsov wrote:
> I think we change as we go. I was introducing new log messages and 
> decided not to use a deprecated term "slave", but rather change whole 
> allocator output for consistency.

After the second though, I think it's OK to be consistent in the allocator but 
not with the master. An allocator can be a module, which means allocator logs 
may differ drastically from master logs. Does it make sense?


- Alexander


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


On Nov. 2, 2015, 3:19 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39860/
> ---
> 
> (Updated Nov. 2, 2015, 3:19 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3718
> https://issues.apache.org/jira/browse/MESOS-3718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
> 
> Diff: https://reviews.apache.org/r/39860/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

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

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 十月 21, 2015, 12:52 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
> 
> (Updated 十月 21, 2015, 12:52 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3072
> https://issues.apache.org/jira/browse/MESOS-3072
> 
> 
> 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/examples/example_module_impl.cpp 
> db015cea130701a4e0a6fcb890c79fbb0c02c1ce 
>   src/examples/test_module.hpp 0514963b6100a6e9a834f2b9670cebc310918fb8 
>   src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
>   src/tests/module_tests.cpp 60497aac3200ab9a679a81a593b5bf0d02fd4b50 
> 
> Diff: https://reviews.apache.org/r/38627/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38335: Add JSON::protobuf for google::protobuf::RepeatedPtrField

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38342, 38335]

All tests passed.

- Mesos ReviewBot


On Nov. 4, 2015, 1:21 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38335/
> ---
> 
> (Updated Nov. 4, 2015, 1:21 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp f56d8a1 
>   src/docker/executor.cpp d4c05c2 
>   src/examples/persistent_volume_framework.cpp 176ac3d 
>   src/launcher/executor.cpp 50b3c6e 
>   src/master/contender.cpp c641305 
>   src/master/http.cpp 9d20346 
>   src/master/maintenance.cpp 5fe9358 
>   src/master/registrar.cpp 1117232 
>   src/slave/containerizer/fetcher.cpp e0d02d5 
>   src/slave/containerizer/mesos/containerizer.cpp 9fd69c1 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 1911ba6 
>   src/slave/monitor.cpp aa6e958 
>   src/tests/containerizer/launch_tests.cpp de655ec 
>   src/tests/containerizer/port_mapping_tests.cpp ae2c0e6 
>   src/tests/fault_tolerance_tests.cpp f78a291 
>   src/tests/master_contender_detector_tests.cpp 1da7f91 
>   src/tests/master_maintenance_tests.cpp e89ce3b 
>   src/tests/master_tests.cpp 8564405 
>   src/tests/mesos.cpp ab2d85b 
>   src/tests/monitor_tests.cpp 583e711 
>   src/tests/reservation_endpoints_tests.cpp f5f9c48 
>   src/tests/resources_tests.cpp 6584fc6 
>   src/tests/scheduler_http_api_tests.cpp b6f6e91 
>   src/tests/script.cpp bcc1fab 
>   src/tests/slave_tests.cpp 91dbdba 
>   src/usage/main.cpp 86fd796 
> 
> Diff: https://reviews.apache.org/r/38335/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38218: Quota: Extended the Allocator interface with quota-related methods.

[Bernd Mathiske]

> On Oct. 25, 2015, 5:46 a.m., Qian Zhang wrote:
> > include/mesos/master/allocator.hpp, line 357
> > 
> >
> > Why do we assume quota for the given role is not set prior to the call? 
> > I thought we support setting quota for a role many time, i.e., the 
> > subsequent call will overwrite the quota set by the previous call, right?
> 
> Alexander Rukletsov wrote:
> Nope. "Overwriting" or updating quota is not straightforward and will be 
> done in the next iterations, which means there will be introduced an 
> `updateQuota()` method.

IMHO having a setQuota call that assumes no prior quota is superfluous once we 
have implemented updateQuota, which will be able to deal with any situation. 
But it is OK for this first interation to get there. Let's revisit this later.


- Bernd


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


On Oct. 23, 2015, 9:38 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38218/
> ---
> 
> (Updated Oct. 23, 2015, 9:38 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3716
> https://issues.apache.org/jira/browse/MESOS-3716
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp dbceb53a3accd32762d09785ecae06667c3cb611 
>   src/master/allocator/mesos/allocator.hpp 
> c5375aa89b210e46c41ac7d68d119749de15d2f5 
>   src/master/allocator/mesos/hierarchical.hpp 
> cfd937ba306273c24fb5337dfeb1a15e1545169b 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
>   src/tests/mesos.hpp 3e58b454c75a2ab9f8b4a29785fa823afefd0c8a 
> 
> Diff: https://reviews.apache.org/r/38218/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 39005: stout: Added thread-safe replacement for strerror.

[Benjamin Bannier]

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

(Updated Nov. 4, 2015, 2:37 p.m.)


Review request for mesos, Bernd Mathiske, Ben Mahler, and Till Toenshoff.


Changes
---

Address review comments.


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


Repository: mesos


Description
---

This commit also adds test cases for os::strerror (from stout) which can
only be committed with a libprocess commit -- the corresponding Makefile
lives there.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/Makefile.am 
76e1674e08bbe65a4fdf86731823a61f231d6d12 
  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
67b142bbc2d80f40a1e893cc5813d58dd2aa8381 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/strerror.hpp PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
26e1377ee625748b7fdbec327fef9ac602535f83 
  3rdparty/libprocess/3rdparty/stout/tests/os/strerror_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Benjamin Bannier

Re: Review Request 39005: stout: Added thread-safe replacement for strerror.

[Benjamin Bannier]

> On Nov. 4, 2015, 12:48 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/strerror.hpp, line 36
> > 
> >
> > How is this relevant?

Agreed, remove reference (which might go stale).


> On Nov. 4, 2015, 12:48 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/strerror.hpp, line 64
> > 
> >
> > a) This is an obscure reference. Pleae provide a pointer. b) Do you 
> > mean "similar to" or "attributed to" instead of "due to"? The latter 
> > indicates that you need to program this way because of something in 
> > heimdal's fcache code affecting strerror_r. That seems implausible.

Removed the reference which was both obscure and not very helpful, and in 
danger of going stale.


> On Nov. 4, 2015, 12:48 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/strerror.hpp, line 53
> > 
> >
> > How would str_error run into errors "itself"? We don't seem to be 
> > handling any such erros here. But if there are any, shouldn't we? And if 
> > there aren't any, would the whole point be moot then?

Error scenarios would be (a) an invalid errnum, or (b) a buffer to small to 
contain the error message from `strerror_r`.

For (a) `strerror_r` will still return some error message string so we do not 
need to do anything.

For (b) we would receive a truncated error message string. The buffer size we 
use "should be big enough" for any conceivable error message. I added a note to 
the docstring so users are aware of this limitation.

In both cases we would receive some string representation of the given `errnum`.


I also expanded the comment on the problems https://reviews.apache.org/r/39005/#review105058
---


On Nov. 4, 2015, 2:37 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39005/
> ---
> 
> (Updated Nov. 4, 2015, 2:37 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3551
> https://issues.apache.org/jira/browse/MESOS-3551
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit also adds test cases for os::strerror (from stout) which can
> only be committed with a libprocess commit -- the corresponding Makefile
> lives there.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 
> 76e1674e08bbe65a4fdf86731823a61f231d6d12 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 67b142bbc2d80f40a1e893cc5813d58dd2aa8381 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/strerror.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
> 26e1377ee625748b7fdbec327fef9ac602535f83 
>   3rdparty/libprocess/3rdparty/stout/tests/os/strerror_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39005/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 38110: Quota: Checked sanity of quota set requests.

[Qian Zhang]

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



src/master/quota_handler.cpp (line 151)


So for this TODO, in future, we want to count dynamically reserved 
resources in ```roleTotal```, right? But in the code below, I see the 
dynamically reserved resources have already been counted in 
```availableOnAgent```, so if we count it here as well, will it be double 
counted?



src/master/quota_handler.cpp (line 161)


Do we want to check if ```missingResources``` is empty first? Maybe the 
quota is already satisfied at this point.



src/master/quota_handler.cpp (line 164)


Do we want to filter out the slave which is not active (slave->active == 
false)?


- Qian Zhang


On Nov. 3, 2015, 11:46 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38110/
> ---
> 
> (Updated Nov. 3, 2015, 11:46 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Michael Park.
> 
> 
> Bugs: MESOS-3074
> https://issues.apache.org/jira/browse/MESOS-3074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Performs a check whether a quota request is reasonable and can be satisfied 
> at the moment. A precise answer is impossible here, but a sanity check is 
> still helpful, because it allows us to filter knowingly unsatisfiable 
> requests.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3aa7017287506938d77bbfb1e06f510101009ee3 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38110/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 39018: Added JSON parsing for Resources.

[Greg Mann]

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

(Updated Nov. 4, 2015, 3:31 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.


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


Repository: mesos


Description
---

This includes code changes necessary for JSON parsing of Resources. 
Documentation changes will be posted soon in another review 
(https://reviews.apache.org/r/39102/).


Diffs
-

  include/mesos/resources.hpp 6c3a065945eb56dc88df9c977e5ca11d4cbcbf61 
  include/mesos/v1/resources.hpp fe8925ac851b74d1b37919f00afc7ed816f47ea5 
  src/common/resources.cpp 601388c35a1bff37c58e753d1870d53b8d0af2d1 
  src/tests/resources_tests.cpp 6584fc6c39e6ffe9f8085576677dcc669f127697 
  src/v1/resources.cpp dc868903472f8f3a1ddc56092e3f8f81d953ce39 

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


Testing (updated)
---

New code was added to `ResourcesTest.ParsingFromJSON`, and two new tests were 
added: `ResourcesTest.ParsingFromJSONWithRoles` and 
`ResourcesTest.ParsingFromJSONError`. These attempt to test common error 
scenarios that might show up in user-supplied JSON.

`make check` was used to confirm that all tests pass.

The original resources parsing format is used throughout many other tests 
(check `src/tests/sorter_tests.cpp`, for example), so it's clear that the 
original parsing continues to work correctly.


Thanks,

Greg Mann

Re: Review Request 39018: Added JSON parsing for Resources.

[Greg Mann]

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

(Updated Nov. 4, 2015, 4:21 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.


Changes
---

Expanded comment in Resources::find().


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


Repository: mesos


Description
---

This includes code changes necessary for JSON parsing of Resources. 
Documentation changes will be posted soon in another review 
(https://reviews.apache.org/r/39102/).


Diffs (updated)
-

  include/mesos/resources.hpp 6c3a065945eb56dc88df9c977e5ca11d4cbcbf61 
  include/mesos/v1/resources.hpp fe8925ac851b74d1b37919f00afc7ed816f47ea5 
  src/common/resources.cpp 601388c35a1bff37c58e753d1870d53b8d0af2d1 
  src/tests/resources_tests.cpp 6584fc6c39e6ffe9f8085576677dcc669f127697 
  src/v1/resources.cpp dc868903472f8f3a1ddc56092e3f8f81d953ce39 

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


Testing
---

New code was added to `ResourcesTest.ParsingFromJSON`, and two new tests were 
added: `ResourcesTest.ParsingFromJSONWithRoles` and 
`ResourcesTest.ParsingFromJSONError`. These attempt to test common error 
scenarios that might show up in user-supplied JSON.

`make check` was used to confirm that all tests pass.

The original resources parsing format is used throughout many other tests 
(check `src/tests/sorter_tests.cpp`, for example), so it's clear that the 
original parsing continues to work correctly.


Thanks,

Greg Mann

Re: Review Request 39018: Added JSON parsing for Resources.

[Greg Mann]

> On Nov. 4, 2015, 2:54 a.m., Guangya Liu wrote:
> > src/v1/resources.cpp, line 1190
> > 
> >
> > remove this

Rather than removing it, I thought it might be nice to expand this comment to 
make it more useful. Let me know what you think!


- Greg


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


On Nov. 4, 2015, 3:31 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> ---
> 
> (Updated Nov. 4, 2015, 3:31 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-2467
> https://issues.apache.org/jira/browse/MESOS-2467
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes code changes necessary for JSON parsing of Resources. 
> Documentation changes will be posted soon in another review 
> (https://reviews.apache.org/r/39102/).
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 6c3a065945eb56dc88df9c977e5ca11d4cbcbf61 
>   include/mesos/v1/resources.hpp fe8925ac851b74d1b37919f00afc7ed816f47ea5 
>   src/common/resources.cpp 601388c35a1bff37c58e753d1870d53b8d0af2d1 
>   src/tests/resources_tests.cpp 6584fc6c39e6ffe9f8085576677dcc669f127697 
>   src/v1/resources.cpp dc868903472f8f3a1ddc56092e3f8f81d953ce39 
> 
> Diff: https://reviews.apache.org/r/39018/diff/
> 
> 
> Testing
> ---
> 
> New code was added to `ResourcesTest.ParsingFromJSON`, and two new tests were 
> added: `ResourcesTest.ParsingFromJSONWithRoles` and 
> `ResourcesTest.ParsingFromJSONError`. These attempt to test common error 
> scenarios that might show up in user-supplied JSON.
> 
> `make check` was used to confirm that all tests pass.
> 
> The original resources parsing format is used throughout many other tests 
> (check `src/tests/sorter_tests.cpp`, for example), so it's clear that the 
> original parsing continues to work correctly.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39873: Fixed master to properly handle status updates when multiple of them are enqueued on the slave.

[Vinod Kone]

> On Nov. 4, 2015, 2:43 a.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 6004-6013
> > 
> >
> > Just looking at the patch that introduced this bug, why are we removing 
> > the out-of-order update prevention? Don't see any mention of this in the 
> > stuff around https://issues.apache.org/jira/browse/MESOS-2864.

There are couple reasons for this

1) it's hard to figure out if an update is an out of order update. for example, 
if TASK_STARTING, TASK_RUNNING and TASK_FINISHED are all enqued on the slave, 
the master might receive non-terminal (TASK_RUNNING) and terminal 
(TASK_FINISHED) after it transitioned the task to terminal state (TASK_STARTING 
update w/ latest state as TASK_FINISHED).

2) when updateTask() is called, typically an update is being forwarded to the 
scheduler. so while it might be ok to not transition the (latest) state of the 
task, it is important to set the task.status_update_state and 
task.status_update_uuid correctly. so we shouldn't short-circuit the function 
without setting these.

makes sense?


> On Nov. 4, 2015, 2:43 a.m., Ben Mahler wrote:
> > src/master/master.cpp, line 6039
> > 
> >
> > Why not have the same logic here to be defensive? Or do you intend to 
> > guard against it? Just seems weird to allow it in one of the cases but not 
> > the other.

i didn't do it in the master's case because it would be a bug in the code if 
the master is trying to call updateTask() for an already terminated task! we 
have to do it for updates from slave because we don't control the executor's 
updates. i could make it a CHECK for the master case though.


- Vinod


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


On Nov. 2, 2015, 10:34 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39873/
> ---
> 
> (Updated Nov. 2, 2015, 10:34 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2864
> https://issues.apache.org/jira/browse/MESOS-2864
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The master now doesn't change the latest state of a task if it has already 
> terminated. But it still updates the status update state and uuid.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 2bc5a97a5b50c8a8a9902c47b2e9e3b5216d97ea 
> 
> Diff: https://reviews.apache.org/r/39873/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Ran the "RecoverCompletedExecutor" test 100 times as it was failing most of 
> the time without this fix.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Review Request 39939: Make docker_socket option support different protocols.

[haosdent huang]

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

Review request for mesos and Timothy Chen.


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


Repository: mesos


Description
---

Make docker_socket option support different protocols.


Diffs
-

  docs/configuration.md 195814cf918e018d8287113299163415b94ab09f 
  src/docker/docker.hpp 527b3e48edf7e84095a8d9ff4fd1a848c2d529f0 
  src/docker/docker.cpp 4ebca660834492f99815a17e04cef6116654dedb 
  src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8 

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


Testing
---

make check


Thanks,

haosdent huang

Review Request 39938: Document OS X SIGPIPE delivery.

[James Peach]

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

Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

Document OS X SIGPIPE delivery.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/tests/os/signals_tests.cpp 
4bb79fdc161483cfc2b7a7a15e5c3ce62ceb493d 

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


Testing
---

make check


Thanks,

James Peach

Review Request 39941: SIGPIPE is ignored in libprocess so stop handling it.

[James Peach]

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

Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

SIGPIPE is ignored in libprocess so stop handling it.


Diffs
-

  src/logging/logging.cpp fb798670d9ac79c75ad39905614fbfe1ea25fba6 
  src/tests/main.cpp 970a8b06e1c04cb6545353c8d36f5707c153d49b 

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


Testing
---

make check.
Run IOTest.Write in a loop on OS X.


Thanks,

James Peach

Review Request 39940: Globally ignore SIGPIPE in libprocess.

[James Peach]

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

Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

Reliably turning SIGPIPE into EPIPE while allowing the main executable
to still own SIGPIPE handling is complex and hard to make reliable
across platforms. Globally ignore SIPIPE when we start up libprocess.


Diffs
-

  3rdparty/libprocess/src/io.cpp 26686e1a96484e3f09d41a7292f38b7579ce9c48 
  3rdparty/libprocess/src/libevent.cpp 7b4299df0e07f4f365ac1cdb24917c804cc72cdc 
  3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313 
  3rdparty/libprocess/src/tests/main.cpp 
4a7b9b68731ba1ac489109f20af2c1eec0bdc84f 

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


Testing
---

make check.
Run IOTest.Write in a loop on OS X.


Thanks,

James Peach

Re: Review Request 39938: Document OS X SIGPIPE delivery.

[haosdent huang]

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

Ship it!


Ship It!

- haosdent huang


On Nov. 4, 2015, 5:15 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39938/
> ---
> 
> (Updated Nov. 4, 2015, 5:15 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2079
> https://issues.apache.org/jira/browse/MESOS-2079
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document OS X SIGPIPE delivery.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/tests/os/signals_tests.cpp 
> 4bb79fdc161483cfc2b7a7a15e5c3ce62ceb493d 
> 
> Diff: https://reviews.apache.org/r/39938/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 39940: Globally ignore SIGPIPE in libprocess.

[haosdent huang]

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

Ship it!


Ship It!

- haosdent huang


On Nov. 4, 2015, 5:16 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39940/
> ---
> 
> (Updated Nov. 4, 2015, 5:16 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2079
> https://issues.apache.org/jira/browse/MESOS-2079
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reliably turning SIGPIPE into EPIPE while allowing the main executable
> to still own SIGPIPE handling is complex and hard to make reliable
> across platforms. Globally ignore SIPIPE when we start up libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/io.cpp 26686e1a96484e3f09d41a7292f38b7579ce9c48 
>   3rdparty/libprocess/src/libevent.cpp 
> 7b4299df0e07f4f365ac1cdb24917c804cc72cdc 
>   3rdparty/libprocess/src/process.cpp 
> a94712b9ac3b60fb047b3a5a4d84a56fa4d02313 
>   3rdparty/libprocess/src/tests/main.cpp 
> 4a7b9b68731ba1ac489109f20af2c1eec0bdc84f 
> 
> Diff: https://reviews.apache.org/r/39940/diff/
> 
> 
> Testing
> ---
> 
> make check.
> Run IOTest.Write in a loop on OS X.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 39941: SIGPIPE is ignored in libprocess so stop handling it.

[haosdent huang]

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

Ship it!


Ship It!

- haosdent huang


On Nov. 4, 2015, 5:16 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39941/
> ---
> 
> (Updated Nov. 4, 2015, 5:16 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2079
> https://issues.apache.org/jira/browse/MESOS-2079
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SIGPIPE is ignored in libprocess so stop handling it.
> 
> 
> Diffs
> -
> 
>   src/logging/logging.cpp fb798670d9ac79c75ad39905614fbfe1ea25fba6 
>   src/tests/main.cpp 970a8b06e1c04cb6545353c8d36f5707c153d49b 
> 
> Diff: https://reviews.apache.org/r/39941/diff/
> 
> 
> Testing
> ---
> 
> make check.
> Run IOTest.Write in a loop on OS X.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 39102: Added documentation for JSON resources.

[Greg Mann]

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

(Updated Nov. 4, 2015, 5:26 p.m.)


Review request for mesos, Adam B and Neil Conway.


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


Repository: mesos


Description
---

Added documentation for JSON resources.


Diffs (updated)
-

  docs/attributes-resources.md f712d094f14426515dabde45f98d6c1ae36c3447 
  docs/configuration.md 195814cf918e018d8287113299163415b94ab09f 

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


Testing
---

Viewed the relevant documentation sections ('Attributes and Resources' & 
'Configuration') using the mesos-website-container: 
https://github.com/mesosphere/mesos-website-container


Thanks,

Greg Mann

Re: Review Request 39453: Added HTTP docs to libprocess README.md.

[Greg Mann]

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

(Updated Nov. 4, 2015, 5:28 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Hindman, and Neil Conway.


Repository: mesos


Description
---

Added HTTP docs to libprocess README.md.


Diffs (updated)
-

  3rdparty/libprocess/README.md 769b79f9a9f816e618dfde62b653e3194386908d 

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


Testing
---

https://gist.github.com/greggomann/b5b35a164a006bec9357


Thanks,

Greg Mann

Re: Review Request 39707: Minor code refactor for fetcher.cpp.

[Vinod Kone]

> On Nov. 4, 2015, 11:30 a.m., Bernd Mathiske wrote:
> > src/launcher/fetcher.cpp, line 297
> > 
> >
> > Or, avoiding a string constant: 
> > 
> > CHECK_FALSE(item.cache_filename().empty());

i don't see a CHECK_FALSE macro in glog?


- Vinod


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


On Oct. 28, 2015, 1:45 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39707/
> ---
> 
> (Updated Oct. 28, 2015, 1:45 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made some small refactoring as I was reading through the fetcher code to make 
> the groking easy. No functional changes.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 8fb6c83981a141df9c0a8a6f8267230bef64f218 
> 
> Diff: https://reviews.apache.org/r/39707/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 39707: Minor code refactor for fetcher.cpp.

[Bernd Mathiske]

> On Nov. 4, 2015, 3:30 a.m., Bernd Mathiske wrote:
> > src/launcher/fetcher.cpp, line 297
> > 
> >
> > Or, avoiding a string constant: 
> > 
> > CHECK_FALSE(item.cache_filename().empty());
> 
> Vinod Kone wrote:
> i don't see a CHECK_FALSE macro in glog?

How about CHECK(!item.cache_filename().empty()) then?
Not a biggy.


- Bernd


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


On Oct. 27, 2015, 6:45 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39707/
> ---
> 
> (Updated Oct. 27, 2015, 6:45 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made some small refactoring as I was reading through the fetcher code to make 
> the groking easy. No functional changes.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 8fb6c83981a141df9c0a8a6f8267230bef64f218 
> 
> Diff: https://reviews.apache.org/r/39707/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 39707: Minor code refactor for fetcher.cpp.

[Bernd Mathiske]

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

Ship it!


Ship It!

- Bernd Mathiske


On Oct. 27, 2015, 6:45 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39707/
> ---
> 
> (Updated Oct. 27, 2015, 6:45 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made some small refactoring as I was reading through the fetcher code to make 
> the groking easy. No functional changes.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 8fb6c83981a141df9c0a8a6f8267230bef64f218 
> 
> Diff: https://reviews.apache.org/r/39707/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Review Request 39944: Add prefix option for os::environment.

[haosdent huang]

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

Review request for mesos, Ben Mahler, Jojy Varghese, and Timothy Chen.


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


Repository: mesos


Description
---

Add prefix option for os::environment.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp 
91d82a8fae27c002458cad0bbdc45b312d2ec70d 

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


Testing
---


Thanks,

haosdent huang

Review Request 39945: Pass SSL related environment variables to executor.

[haosdent huang]

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

Review request for mesos, Ben Mahler, Jojy Varghese, and Timothy Chen.


Repository: mesos


Description
---

Pass SSL related environment variables to executor.


Diffs
-

  src/slave/containerizer/containerizer.cpp 
06753365e2ec7cb59edd1ed6ecfe1a794498ee9b 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 39939: Make docker_socket option support different protocols.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39939]

All tests passed.

- Mesos ReviewBot


On Nov. 4, 2015, 5:13 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39939/
> ---
> 
> (Updated Nov. 4, 2015, 5:13 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-3821
> https://issues.apache.org/jira/browse/MESOS-3821
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make docker_socket option support different protocols.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 195814cf918e018d8287113299163415b94ab09f 
>   src/docker/docker.hpp 527b3e48edf7e84095a8d9ff4fd1a848c2d529f0 
>   src/docker/docker.cpp 4ebca660834492f99815a17e04cef6116654dedb 
>   src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8 
> 
> Diff: https://reviews.apache.org/r/39939/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

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

(Updated Nov. 4, 2015, 6:07 p.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

moved timeout semantics to the caller.


Repository: mesos


Description
---

- Cleanup and broke big methods into smaller chunks.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
ad57d8592c5f9df5115b575855ed2e99a0597359 
  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
  src/tests/containerizer/provisioner_docker_tests.cpp 
8d90894410cd834edf49a2814d1b616718798fe8 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 39015: RegistryClient refactor: expanded abbreviated names.

[Jojy Varghese]

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

(Updated Nov. 4, 2015, 6:08 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

rebased


Repository: mesos


Description
---

RegistryClient refactor: expanded abbreviated names.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
ad57d8592c5f9df5115b575855ed2e99a0597359 
  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 39112: RegistryClient refactor: fixed variable names

[Jojy Varghese]

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

(Updated Nov. 4, 2015, 6:08 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

rebased


Repository: mesos


Description
---

RegistryClient refactor: fixed variable names. This patch will serve as 
catch-all for any variable name related changes in the refactor.


Diffs (updated)
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
8d90894410cd834edf49a2814d1b616718798fe8 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 39053: RegistryClient refactor: priv method const'ness

[Jojy Varghese]

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

(Updated Nov. 4, 2015, 6:08 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

rebased


Repository: mesos


Description
---

RegistryClient refactor: priv method const'ness


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 

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


Testing
---

make check.


Thanks,

Jojy Varghese

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 Nov. 4, 2015, 6:09 p.m.)


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


Changes
---

added discard semantics


Repository: mesos


Description
---

Integrated remote puller with store. Tests will follow.


Diffs (updated)
-

  src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289 
  src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db 
  src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
87d80026939a326bd1169f46906e36d6ef19f546 
  src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
f314f20d989ed0125e15d2a14d0f8e56c56976d5 
  src/slave/containerizer/mesos/provisioner/docker/puller.hpp 
8010b8aa75a2fc2e4b537f915f9dca028e407b63 
  src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2 
  src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
ad57d8592c5f9df5115b575855ed2e99a0597359 
  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
  src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp 
PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
bb02d650e16d45fcf337a7954f7a26143fb2c69f 
  src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73 
  src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8 
  src/tests/containerizer/provisioner_docker_tests.cpp 
8d90894410cd834edf49a2814d1b616718798fe8 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 39803: Windows: Implemented stout/os/stat.hpp`.

[Michael Hopcroft]

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



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp
 (line 26)


Good comment!



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp
 (line 32)


I wonder if there is any good way of protecting ourselves from breaking 
changes associated with _REPARSE_DATA_BUFFER. Perhaps you should consider 
setting WINVER in windows.hpp. Then you could add a static_assert on the value 
of WINVER in this header.


http://stackoverflow.com/questions/10112051/c-compile-time-macros-to-detect-windows-os



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp
 (line 33)


Consider adding one-line comments for each field.

https://msdn.microsoft.com/en-us/library/cc232007.aspx



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp
 (line 45)


One line comment for this field is important since the length is in bytes, 
not WCHARS. Same for line 55.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp
 (line 49)


Are you sure these buffers of size 1 are correct? I'm assuming that the 
remainder of the buffer follows the _REPARSE_DATA_BUFFER in the block of memory.

I would add a comment explaining this.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp
 (line 51)


Recommend putting a blank line before this comment to that it is absolutely 
clear which struct it refers to.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp
 (line 71)


Who actually uses this struct. The reason I ask is that it has wstrings 
inside instead of strings. Will the user be forced to do the conversion? 
Perhaps you should be doing the conversion for them here.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp
 (line 73)


Be aware the std::wstring can throw exceptions. If stout has a strong 
no-exception guarantee, you should review the code paths to make sure that this 
exception is caught somewhere inside of stout.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp
 (line 75)


If this struct is part of the API, it should have better documentation. One 
cannot know what flags is without reading the implementation in this file.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp
 (line 84)


Should check for return value of  INVALID_FILE_ATTRIBUTES or add an 
explanatory comment if the code on line 85 correctly handles this case.

In general, your logic should not make any assumptions on what 
INVALID_FILE_ATTRIBUTES actually equals.

--
Oh - I see - you check for INVALID_FILE_ATTRIBUTES on line 89. In general 
line 85 is bad form because reparseBitSet is only valid when attributes != 
INVALID_FILE_ATTRIBUTES. Recommend writing code so that reparseBitSet is valid 
during its entire lifetime.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp
 (line 87)


And get rid of this comment. It assumes that INVALID_FILE_ATTRIBUTES == -1, 
which may not always be true. This comment shouldn't be necessary if you rework 
the logic per my comment on line 84.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp
 (line 104)


Divide by sizeof(WCHAR). Same goes for lines 106, 111, and 113.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp
 (line 108)


I prefer "data.SymbolicLinkReparseBuffer.PathBuffer + printNameStartIndex", 
but some might consider this a religious or stylistic difference. My opinion is 
that you shouldn't dereference the pointer ([]) if you are then going to take 
its address.

Same goes for line 115.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp
 (line 145)


"or a file" not "of a file"



3rdparty/libprocess/3rdparty/stout/include/stout/i

Re: Review Request 39250: Puller refactor: moved untar to a common place

[Jojy Varghese]

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

(Updated Nov. 4, 2015, 6:09 p.m.)


Review request for mesos and Timothy Chen.


Changes
---

rebased.


Repository: mesos


Description
---

Puller refactor: moved untar to a common place


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
f314f20d989ed0125e15d2a14d0f8e56c56976d5 
  src/slave/containerizer/mesos/provisioner/docker/puller.hpp 
8010b8aa75a2fc2e4b537f915f9dca028e407b63 
  src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2 
  src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp 
PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 39944: Add prefix option for os::environment.

[Jojy Varghese]

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



3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp (line 40)


I thought the intent here was to check for prefix and not anywhere in the 
string.


- Jojy Varghese


On Nov. 4, 2015, 6:02 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39944/
> ---
> 
> (Updated Nov. 4, 2015, 6:02 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add prefix option for os::environment.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp 
> 91d82a8fae27c002458cad0bbdc45b312d2ec70d 
> 
> Diff: https://reviews.apache.org/r/39944/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39944: Add prefix option for os::environment.

[Jojy Varghese]

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


I would suggest you add tests for this.

- Jojy Varghese


On Nov. 4, 2015, 6:02 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39944/
> ---
> 
> (Updated Nov. 4, 2015, 6:02 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add prefix option for os::environment.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp 
> 91d82a8fae27c002458cad0bbdc45b312d2ec70d 
> 
> Diff: https://reviews.apache.org/r/39944/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39945: Pass SSL related environment variables to executor.

[Jojy Varghese]

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



src/slave/containerizer/containerizer.cpp (line 259)


Wondering if we can pass in a prefix set to the os::environment. This way, 
we get LIBPROCESS_IP, SSL etc all in at the same time.


- Jojy Varghese


On Nov. 4, 2015, 6:03 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39945/
> ---
> 
> (Updated Nov. 4, 2015, 6:03 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jojy Varghese, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pass SSL related environment variables to executor.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.cpp 
> 06753365e2ec7cb59edd1ed6ecfe1a794498ee9b 
> 
> Diff: https://reviews.apache.org/r/39945/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39944: Add prefix option for os::environment.

[Jojy Varghese]

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



3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp (line 25)


Do you intend to copy the string each time? Or a const string& will do?

As mentioned in the other review, maybe passing a set of prefixes or 
regular expressions would be useful.


- Jojy Varghese


On Nov. 4, 2015, 6:02 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39944/
> ---
> 
> (Updated Nov. 4, 2015, 6:02 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add prefix option for os::environment.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp 
> 91d82a8fae27c002458cad0bbdc45b312d2ec70d 
> 
> Diff: https://reviews.apache.org/r/39944/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Review Request 39946: Moved definition of ctors and dtors for mock classes out of header file.

[Neil Conway]

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

Review request for mesos and Joris Van Remoortere.


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


Repository: mesos


Description
---

As discussed in MESOS-3827, this can improve compilation time of classes that
use GMock. On my system, this improves the compilation time of the unit tests by
about 8%.

Note that we can't use this technique with TestAllocator, because it is
templatized.


Diffs
-

  src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db 
  src/tests/containerizer/launcher.hpp 5d34bab789bdafe71d94e8ec263710c50b83e180 
  src/tests/containerizer/launcher.cpp PRE-CREATION 
  src/tests/mesos.hpp f731ac3284a5793b6bf510d3a5b742cbe0938217 
  src/tests/mesos.cpp ab2d85b091d121113931e4190a5b496901dcd7a5 

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


Testing
---


Thanks,

Neil Conway

Review Request 39947: Refactored mock allocator into a separate header file.

[Neil Conway]

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

Review request for mesos and Joris Van Remoortere.


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


Repository: mesos


Description
---

Refactored mock allocator into a separate header file.


Diffs
-

  src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db 
  src/tests/allocator.hpp PRE-CREATION 
  src/tests/hierarchical_allocator_tests.cpp 
505b9de3d8d888c296f6103c80fe9f0ef1c2ca16 
  src/tests/master_allocator_tests.cpp 1fe3757f224281c312008e9010a95559cfff3dcf 
  src/tests/mesos.hpp f731ac3284a5793b6bf510d3a5b742cbe0938217 
  src/tests/reservation_endpoints_tests.cpp 
f5f9c4834779a3a84d12d9be77e674e2d6088b5d 
  src/tests/reservation_tests.cpp 6b7c43c8b5c64618249dbee926383242320c111e 
  src/tests/resource_offers_tests.cpp af40a072bf5221cda42147e6f2c09d020a7f63f2 
  src/tests/slave_recovery_tests.cpp a50960c156da6adcab869a65ab9286adda188d2c 

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


Testing
---


Thanks,

Neil Conway

Review Request 39948: Remove some undocumented, commented-out code within libprocess.

[Joseph Wu]

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

Review request for mesos, Benjamin Hindman and Joris Van Remoortere.


Repository: mesos


Description
---

These are remnants of commented-out code, dating back to when libprocess was 
originally open-sourced.


Diffs
-

  3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313 

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


Testing
---

None.  Comment change only.


Thanks,

Joseph Wu

Review Request 39949: Document and simplify libprocess initialization synchronization logic.

[Joseph Wu]

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

Review request for mesos, Benjamin Hindman and Joris Van Remoortere.


Repository: mesos


Description
---

The initialization synchronization logic contains three conditions, which check:
1) Was `initialize` called and is it done?
2) Was `initialize` called and is it not done?
3) Are you the first to call `initialize`?

Condition (3) uses `compare_exchange_strong` between `initialized` and `false`. 
 This returns `true` (and sets `initialized` to true) iff the caller is the 
first to reach that expression.

The second simultaneous caller of `initialize` will either satisify condition 
(2) or (3) and then wait on `initializing`.  For the second caller, (2) and (3) 
are identical because `compare_exchange_strong` between `true` and `false` will 
always return false, thereby putting the second caller into the waiting loop.


Diffs
-

  3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313 

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


Testing
---

`make check`


Thanks,

Joseph Wu

Re: Review Request 39944: Add prefix option for os::environment.

[Timothy Chen]

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



3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp (line 25)


Why is this needed?
And also we prefer Option rather than an empty string default.


- Timothy Chen


On Nov. 4, 2015, 6:02 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39944/
> ---
> 
> (Updated Nov. 4, 2015, 6:02 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add prefix option for os::environment.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp 
> 91d82a8fae27c002458cad0bbdc45b312d2ec70d 
> 
> Diff: https://reviews.apache.org/r/39944/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39949: Document and simplify libprocess initialization synchronization logic.

[Neil Conway]

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


Can we test that more thoroughly than just "make check"? e.g., if there's a 
unit test that tries to enter this logic with multiple threads at once, running 
that with gtest_repeat=1000 would be nice.


3rdparty/libprocess/src/process.cpp (line 742)


I wonder if it would be an improvement to rename these variables to reflect 
what they are used for more clearly.

For example: maybe call them "initialize_started" and "initialize_complete" 
(and change the second so that it goes from false -> true instead of true -> 
false).



3rdparty/libprocess/src/process.cpp (line 751)


process::initialize.



3rdparty/libprocess/src/process.cpp (line 759)


Can you clarify what "the first ones" means here?


Can

- Neil Conway


On Nov. 4, 2015, 6:58 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39949/
> ---
> 
> (Updated Nov. 4, 2015, 6:58 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The initialization synchronization logic contains three conditions, which 
> check:
> 1) Was `initialize` called and is it done?
> 2) Was `initialize` called and is it not done?
> 3) Are you the first to call `initialize`?
> 
> Condition (3) uses `compare_exchange_strong` between `initialized` and 
> `false`.  This returns `true` (and sets `initialized` to true) iff the caller 
> is the first to reach that expression.
> 
> The second simultaneous caller of `initialize` will either satisify condition 
> (2) or (3) and then wait on `initializing`.  For the second caller, (2) and 
> (3) are identical because `compare_exchange_strong` between `true` and 
> `false` will always return false, thereby putting the second caller into the 
> waiting loop.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> a94712b9ac3b60fb047b3a5a4d84a56fa4d02313 
> 
> Diff: https://reviews.apache.org/r/39949/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39941: SIGPIPE is ignored in libprocess so stop handling it.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39938, 39940, 39941]

All tests passed.

- Mesos ReviewBot


On Nov. 4, 2015, 5:16 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39941/
> ---
> 
> (Updated Nov. 4, 2015, 5:16 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2079
> https://issues.apache.org/jira/browse/MESOS-2079
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SIGPIPE is ignored in libprocess so stop handling it.
> 
> 
> Diffs
> -
> 
>   src/logging/logging.cpp fb798670d9ac79c75ad39905614fbfe1ea25fba6 
>   src/tests/main.cpp 970a8b06e1c04cb6545353c8d36f5707c153d49b 
> 
> Diff: https://reviews.apache.org/r/39941/diff/
> 
> 
> Testing
> ---
> 
> make check.
> Run IOTest.Write in a loop on OS X.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 39949: Document and simplify libprocess initialization synchronization logic.

[Joseph Wu]

> On Nov. 4, 2015, 11:25 a.m., Neil Conway wrote:
> > Can we test that more thoroughly than just "make check"? e.g., if there's a 
> > unit test that tries to enter this logic with multiple threads at once, 
> > running that with gtest_repeat=1000 would be nice.

There are tons of methods in libprocess that call `process::initialize` as a 
side-effect, but at the same time, the libprocess test suite starts out with an 
essentially race-free init call (See: 
https://github.com/apache/mesos/blob/master/3rdparty/libprocess/src/tests/main.cpp#L52).
  So any `--gtest_repeat` or `--gtest_shuffle` won't actually test the init 
code.  (The master and agent also call init once on startup.)

I'm not sure how valuable it will be to, say, to spawn a bunch of threads that 
call `process::initialize`.  Do you have any suggestions?


> On Nov. 4, 2015, 11:25 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/src/process.cpp, line 742
> > 
> >
> > I wonder if it would be an improvement to rename these variables to 
> > reflect what they are used for more clearly.
> > 
> > For example: maybe call them "initialize_started" and 
> > "initialize_complete" (and change the second so that it goes from false -> 
> > true instead of true -> false).

That sounds very reasonable.  I'll rename (and see if anyone objects :).


- Joseph


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


On Nov. 4, 2015, 10:58 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39949/
> ---
> 
> (Updated Nov. 4, 2015, 10:58 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The initialization synchronization logic contains three conditions, which 
> check:
> 1) Was `initialize` called and is it done?
> 2) Was `initialize` called and is it not done?
> 3) Are you the first to call `initialize`?
> 
> Condition (3) uses `compare_exchange_strong` between `initialized` and 
> `false`.  This returns `true` (and sets `initialized` to true) iff the caller 
> is the first to reach that expression.
> 
> The second simultaneous caller of `initialize` will either satisify condition 
> (2) or (3) and then wait on `initializing`.  For the second caller, (2) and 
> (3) are identical because `compare_exchange_strong` between `true` and 
> `false` will always return false, thereby putting the second caller into the 
> waiting loop.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> a94712b9ac3b60fb047b3a5a4d84a56fa4d02313 
> 
> Diff: https://reviews.apache.org/r/39949/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39949: Document and simplify libprocess initialization synchronization logic.

[Neil Conway]

> On Nov. 4, 2015, 7:25 p.m., Neil Conway wrote:
> > Can we test that more thoroughly than just "make check"? e.g., if there's a 
> > unit test that tries to enter this logic with multiple threads at once, 
> > running that with gtest_repeat=1000 would be nice.
> 
> Joseph Wu wrote:
> There are tons of methods in libprocess that call `process::initialize` 
> as a side-effect, but at the same time, the libprocess test suite starts out 
> with an essentially race-free init call (See: 
> https://github.com/apache/mesos/blob/master/3rdparty/libprocess/src/tests/main.cpp#L52).
>   So any `--gtest_repeat` or `--gtest_shuffle` won't actually test the init 
> code.  (The master and agent also call init once on startup.)
> 
> I'm not sure how valuable it will be to, say, to spawn a bunch of threads 
> that call `process::initialize`.  Do you have any suggestions?

You could hackup tests/main.cpp::main() to test concurrent calls to 
process::initialize() -- i.e., 

```if (getenv("TEST_LIBPROCESS_INIT")) { for (int i = 0; i < 100; i++) { /* 
spawn thread */ process::initialize(); } } }```


- Neil


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


On Nov. 4, 2015, 6:58 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39949/
> ---
> 
> (Updated Nov. 4, 2015, 6:58 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The initialization synchronization logic contains three conditions, which 
> check:
> 1) Was `initialize` called and is it done?
> 2) Was `initialize` called and is it not done?
> 3) Are you the first to call `initialize`?
> 
> Condition (3) uses `compare_exchange_strong` between `initialized` and 
> `false`.  This returns `true` (and sets `initialized` to true) iff the caller 
> is the first to reach that expression.
> 
> The second simultaneous caller of `initialize` will either satisify condition 
> (2) or (3) and then wait on `initializing`.  For the second caller, (2) and 
> (3) are identical because `compare_exchange_strong` between `true` and 
> `false` will always return false, thereby putting the second caller into the 
> waiting loop.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> a94712b9ac3b60fb047b3a5a4d84a56fa4d02313 
> 
> Diff: https://reviews.apache.org/r/39949/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39949: Document and simplify libprocess initialization synchronization logic.

[Joseph Wu]

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

(Updated Nov. 4, 2015, 12:24 p.m.)


Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Neil 
Conway.


Changes
---

Renamed variables to more closely reflect their purpose.  Negated 
`initializing` to `initialize_complete`.
Did some manual tests.


Repository: mesos


Description (updated)
---

* Renamed `initialized` to `initialize_started`.
* Renamed `initializing` to `initialize_complete`.
* Removed the (2) condition, described below: 

The initialization synchronization logic contains three conditions, which check:
1) Was `initialize` called and is it done?
2) Was `initialize` called and is it not done?
3) Are you the first to call `initialize`?

Condition (3) uses `compare_exchange_strong` between `initialized` and `false`. 
 This returns `true` (and sets `initialized` to true) iff the caller is the 
first to reach that expression.

The second simultaneous caller of `initialize` will either satisify condition 
(2) or (3) and then wait on `initializing`.  For the second caller, (2) and (3) 
are identical because `compare_exchange_strong` between `true` and `false` will 
always return false, thereby putting the second caller into the waiting loop.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313 

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


Testing (updated)
---

`make check`

Replaced `process::initialize();` in `3rdparty/libprocess/src/tests/main.cpp` 
with:
```

  const size_t numThreads = 50;

  std::thread* runningThreads[numThreads];

  // Create additional threads.
  for (size_t i = 0; i < numThreads; i++) {
runningThreads[i] = new std::thread([]() {
  process::initialize();
});
  }

  for (size_t i = 0; i < numThreads; i++) {
runningThreads[i]->join();
delete runningThreads[i];
  }
```
(Also added `#include ` to the header).

Rebuilt `libprocess-tests` with the modification and ran it a few times.
`3rdparty/libprocess/libprocess-tests`


Thanks,

Joseph Wu

Re: Review Request 39949: Document and simplify libprocess initialization synchronization logic.

[Joseph Wu]

> On Nov. 4, 2015, 11:25 a.m., Neil Conway wrote:
> > Can we test that more thoroughly than just "make check"? e.g., if there's a 
> > unit test that tries to enter this logic with multiple threads at once, 
> > running that with gtest_repeat=1000 would be nice.
> 
> Joseph Wu wrote:
> There are tons of methods in libprocess that call `process::initialize` 
> as a side-effect, but at the same time, the libprocess test suite starts out 
> with an essentially race-free init call (See: 
> https://github.com/apache/mesos/blob/master/3rdparty/libprocess/src/tests/main.cpp#L52).
>   So any `--gtest_repeat` or `--gtest_shuffle` won't actually test the init 
> code.  (The master and agent also call init once on startup.)
> 
> I'm not sure how valuable it will be to, say, to spawn a bunch of threads 
> that call `process::initialize`.  Do you have any suggestions?
> 
> Neil Conway wrote:
> You could hackup tests/main.cpp::main() to test concurrent calls to 
> process::initialize() -- i.e., 
> 
> ```if (getenv("TEST_LIBPROCESS_INIT")) { for (int i = 0; i < 100; i++) { 
> /* spawn thread */ process::initialize(); } } }```

Done.  Nothing funky seemed to happen.  (And if I add a print statement in the 
body of the init, it only gets printed once, as expected.)


- Joseph


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


On Nov. 4, 2015, 12:24 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39949/
> ---
> 
> (Updated Nov. 4, 2015, 12:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Neil 
> Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> * Renamed `initialized` to `initialize_started`.
> * Renamed `initializing` to `initialize_complete`.
> * Removed the (2) condition, described below: 
> 
> The initialization synchronization logic contains three conditions, which 
> check:
> 1) Was `initialize` called and is it done?
> 2) Was `initialize` called and is it not done?
> 3) Are you the first to call `initialize`?
> 
> Condition (3) uses `compare_exchange_strong` between `initialized` and 
> `false`.  This returns `true` (and sets `initialized` to true) iff the caller 
> is the first to reach that expression.
> 
> The second simultaneous caller of `initialize` will either satisify condition 
> (2) or (3) and then wait on `initializing`.  For the second caller, (2) and 
> (3) are identical because `compare_exchange_strong` between `true` and 
> `false` will always return false, thereby putting the second caller into the 
> waiting loop.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> a94712b9ac3b60fb047b3a5a4d84a56fa4d02313 
> 
> Diff: https://reviews.apache.org/r/39949/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Replaced `process::initialize();` in `3rdparty/libprocess/src/tests/main.cpp` 
> with:
> ```
> 
>   const size_t numThreads = 50;
> 
>   std::thread* runningThreads[numThreads];
> 
>   // Create additional threads.
>   for (size_t i = 0; i < numThreads; i++) {
> runningThreads[i] = new std::thread([]() {
>   process::initialize();
> });
>   }
> 
>   for (size_t i = 0; i < numThreads; i++) {
> runningThreads[i]->join();
> delete runningThreads[i];
>   }
> ```
> (Also added `#include ` to the header).
> 
> Rebuilt `libprocess-tests` with the modification and ran it a few times.
> `3rdparty/libprocess/libprocess-tests`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39949: Document and simplify libprocess initialization synchronization logic.

[Neil Conway]

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

Ship it!


Ship It!

- Neil Conway


On Nov. 4, 2015, 8:24 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39949/
> ---
> 
> (Updated Nov. 4, 2015, 8:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Neil 
> Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> * Renamed `initialized` to `initialize_started`.
> * Renamed `initializing` to `initialize_complete`.
> * Removed the (2) condition, described below: 
> 
> The initialization synchronization logic contains three conditions, which 
> check:
> 1) Was `initialize` called and is it done?
> 2) Was `initialize` called and is it not done?
> 3) Are you the first to call `initialize`?
> 
> Condition (3) uses `compare_exchange_strong` between `initialized` and 
> `false`.  This returns `true` (and sets `initialized` to true) iff the caller 
> is the first to reach that expression.
> 
> The second simultaneous caller of `initialize` will either satisify condition 
> (2) or (3) and then wait on `initializing`.  For the second caller, (2) and 
> (3) are identical because `compare_exchange_strong` between `true` and 
> `false` will always return false, thereby putting the second caller into the 
> waiting loop.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> a94712b9ac3b60fb047b3a5a4d84a56fa4d02313 
> 
> Diff: https://reviews.apache.org/r/39949/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Replaced `process::initialize();` in `3rdparty/libprocess/src/tests/main.cpp` 
> with:
> ```
> 
>   const size_t numThreads = 50;
> 
>   std::thread* runningThreads[numThreads];
> 
>   // Create additional threads.
>   for (size_t i = 0; i < numThreads; i++) {
> runningThreads[i] = new std::thread([]() {
>   process::initialize();
> });
>   }
> 
>   for (size_t i = 0; i < numThreads; i++) {
> runningThreads[i]->join();
> delete runningThreads[i];
>   }
> ```
> (Also added `#include ` to the header).
> 
> Rebuilt `libprocess-tests` with the modification and ran it a few times.
> `3rdparty/libprocess/libprocess-tests`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39948: Remove some undocumented, commented-out code within libprocess.

[Neil Conway]

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

Ship it!


Ship It!

- Neil Conway


On Nov. 4, 2015, 6:50 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39948/
> ---
> 
> (Updated Nov. 4, 2015, 6:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These are remnants of commented-out code, dating back to when libprocess was 
> originally open-sourced.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> a94712b9ac3b60fb047b3a5a4d84a56fa4d02313 
> 
> Diff: https://reviews.apache.org/r/39948/diff/
> 
> 
> Testing
> ---
> 
> None.  Comment change only.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 39954: Fixed spelling in comments.

[Jojy Varghese]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Fixed spelling in comments.


Diffs
-

  support/docker_build.sh  

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


Testing
---


Thanks,

Jojy Varghese

Review Request 39953: Renamed jenkins_build.sh to docker_build.sh.

[Jojy Varghese]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

This matches the functionality of the script.  We intend to use the build script
as a common scipt for both mesos-reviewbot and mesos.


Diffs
-

  support/jenkins_build.sh  

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


Testing
---

locally ran the script


Thanks,

Jojy Varghese

Re: Review Request 39954: Fixed spelling in comments.

[Jojy Varghese]

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

(Updated Nov. 4, 2015, 8:44 p.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Fixed spelling in comments.


Diffs
-

  support/docker_build.sh  

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


Testing (updated)
---

locally ran the script


Thanks,

Jojy Varghese

Re: Review Request 39914: Changed verify_review to use docker_build.sh.

[Jojy Varghese]

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

(Updated Nov. 4, 2015, 8:47 p.m.)


Review request for mesos, Cody Maloney and Vinod Kone.


Changes
---

after discussion with vinod.


Summary (updated)
-

Changed verify_review to use docker_build.sh.


Repository: mesos


Description (updated)
---

SSL based tests are not currently verified on reviewbot. This change would make 
reviewbot environment to be same as mesos builds on jenkins.


Diffs (updated)
-

  support/verify_reviews.py 0d322a19af719aba8846fbc705bf8ec7647304a3 

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


Testing (updated)
---

ran script locally


Thanks,

Jojy Varghese

Re: Review Request 39719: Fixed marking mounts as slave in ubuntu.

[Timothy Chen]

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

(Updated Nov. 4, 2015, 8:57 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Fixed marking mounts as slave in ubuntu. More details please see MESOS-3806


Diffs (updated)
-

  src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289 
  src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
a126cd6ee15adb8d92e85ab562e998e3647f999a 
  src/slave/containerizer/mesos/main.cpp 
0e1793128b56ab2e0aa2d263383c9de47ffe25d1 
  src/slave/containerizer/mesos/mount.hpp PRE-CREATION 
  src/slave/containerizer/mesos/mount.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 39866: Added containerInfo support for tasks in mesos containerizer.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Nov. 4, 2015, 8:58 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39866/
> ---
> 
> (Updated Nov. 4, 2015, 8:58 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added containerInfo support for tasks in mesos containerizer.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 4aad8a3be43b331efc6b8157b2fae090df16c1b4 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 9fd69c1738e2300dbb843d259727010e24523cff 
> 
> Diff: https://reviews.apache.org/r/39866/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 39866: Added containerInfo support for tasks in mesos containerizer.

[Timothy Chen]

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

(Updated Nov. 4, 2015, 8:58 p.m.)


Review request for mesos, Jie Yu and Jojy Varghese.


Repository: mesos


Description
---

Added containerInfo support for tasks in mesos containerizer.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
4aad8a3be43b331efc6b8157b2fae090df16c1b4 
  src/slave/containerizer/mesos/containerizer.cpp 
9fd69c1738e2300dbb843d259727010e24523cff 

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


Testing
---

make check


Thanks,

Timothy Chen

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

[Timothy Chen]

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

(Updated Nov. 4, 2015, 8:59 p.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 (updated)
-

  src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
  src/slave/constants.hpp de6b58a93346c618a9214032d891c1004203ca56 
  src/slave/constants.cpp b69471b2d57aad0c254ef3bb7dce9405abeab93a 
  src/slave/slave.hpp e6fa66b40c7f17c500056b7d6f95d7e795a16ca0 
  src/slave/slave.cpp 5f9b52b41eaab0c24965f28e192074340e00bde5 
  src/tests/slave_tests.cpp 91dbdba56c7d3a374e56be92d88c0b367c7a2e1c 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 39719: Fixed marking mounts as slave in ubuntu.

[Jie Yu]

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

Ship it!



src/slave/containerizer/mesos/mount.cpp (line 72)


One extra blank line above.


- Jie Yu


On Nov. 4, 2015, 8:57 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39719/
> ---
> 
> (Updated Nov. 4, 2015, 8:57 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3806
> https://issues.apache.org/jira/browse/MESOS-3806
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed marking mounts as slave in ubuntu. More details please see MESOS-3806
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289 
>   src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> a126cd6ee15adb8d92e85ab562e998e3647f999a 
>   src/slave/containerizer/mesos/main.cpp 
> 0e1793128b56ab2e0aa2d263383c9de47ffe25d1 
>   src/slave/containerizer/mesos/mount.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/mount.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39719/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 39953: Renamed jenkins_build.sh to docker_build.sh.

[Vinod Kone]

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

Ship it!


Ship It!

- Vinod Kone


On Nov. 4, 2015, 8:43 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39953/
> ---
> 
> (Updated Nov. 4, 2015, 8:43 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This matches the functionality of the script.  We intend to use the build 
> script
> as a common scipt for both mesos-reviewbot and mesos.
> 
> 
> Diffs
> -
> 
>   support/jenkins_build.sh  
> 
> Diff: https://reviews.apache.org/r/39953/diff/
> 
> 
> Testing
> ---
> 
> locally ran the script
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39869: Added provisioner TestStore.

[Timothy Chen]

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

(Updated Nov. 4, 2015, 9:03 p.m.)


Review request for mesos, Jie Yu and Jojy Varghese.


Repository: mesos


Description
---

Added provisioner TestStore.


Diffs (updated)
-

  src/tests/containerizer/store.hpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 39868: Moved ProvisionerProcess to header for testing.

[Timothy Chen]

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

(Updated Nov. 4, 2015, 9:03 p.m.)


Review request for mesos, Jie Yu and Jojy Varghese.


Repository: mesos


Description
---

Moved ProvisionerProcess to header for testing.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/provisioner.hpp 
912fc5abadb1219fc4baec1a751010522bc7a7d0 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
e99c1c9876b836be5d3efcef7408b5ed01d8984e 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 39258: Add filesystem isolator with command executor test.

[Timothy Chen]

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

(Updated Nov. 4, 2015, 9:04 p.m.)


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


Repository: mesos


Description
---

Add filesystem isolator with command executor test.


Diffs (updated)
-

  src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
39008f620183d242407fea5377bfceffc57b 
  src/tests/containerizer/provisioner.hpp 
507e1413470ef4f36ead657203073115d5324bef 
  src/tests/containerizer/rootfs.hpp 56a205f7c18318ab9e99f4bb8cb969ba3302baea 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 39954: Fixed spelling in comments.

[Vinod Kone]

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


the diff is empty?

- Vinod Kone


On Nov. 4, 2015, 8:44 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39954/
> ---
> 
> (Updated Nov. 4, 2015, 8:44 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed spelling in comments.
> 
> 
> Diffs
> -
> 
>   support/docker_build.sh  
> 
> Diff: https://reviews.apache.org/r/39954/diff/
> 
> 
> Testing
> ---
> 
> locally ran the script
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39914: Changed verify_review to use docker_build.sh.

[Vinod Kone]

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



support/verify_reviews.py (line 122)


have you verified this change locally?

the docker_build.sh script expects certain environment variables to be set.


- Vinod Kone


On Nov. 4, 2015, 8:47 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39914/
> ---
> 
> (Updated Nov. 4, 2015, 8:47 p.m.)
> 
> 
> Review request for mesos, Cody Maloney and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SSL based tests are not currently verified on reviewbot. This change would 
> make reviewbot environment to be same as mesos builds on jenkins.
> 
> 
> Diffs
> -
> 
>   support/verify_reviews.py 0d322a19af719aba8846fbc705bf8ec7647304a3 
> 
> Diff: https://reviews.apache.org/r/39914/diff/
> 
> 
> Testing
> ---
> 
> ran script locally
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39914: Changed verify_review to use docker_build.sh.

[Jojy Varghese]

> On Nov. 4, 2015, 9:12 p.m., Vinod Kone wrote:
> > support/verify_reviews.py, line 122
> > 
> >
> > have you verified this change locally?
> > 
> > the docker_build.sh script expects certain environment variables to be 
> > set.

I ran the script locally by setting the OS, CONFIGURATION etc manually. I didnt 
know the workflow for setting these. How are these env variables set?


- Jojy


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


On Nov. 4, 2015, 8:47 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39914/
> ---
> 
> (Updated Nov. 4, 2015, 8:47 p.m.)
> 
> 
> Review request for mesos, Cody Maloney and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SSL based tests are not currently verified on reviewbot. This change would 
> make reviewbot environment to be same as mesos builds on jenkins.
> 
> 
> Diffs
> -
> 
>   support/verify_reviews.py 0d322a19af719aba8846fbc705bf8ec7647304a3 
> 
> Diff: https://reviews.apache.org/r/39914/diff/
> 
> 
> Testing
> ---
> 
> ran script locally
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38454: Fix failed test LimitedCpuIsolatorTest.ROOT_CGROUPS_Pids_and_Tids

[Kapil Arya]

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

Ship it!


Ship It!

- Kapil Arya


On Sept. 23, 2015, 9:55 p.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38454/
> ---
> 
> (Updated Sept. 23, 2015, 9:55 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Marco Massenzio, and Vinod Kone.
> 
> 
> Bugs: MESOS-3293
> https://issues.apache.org/jira/browse/MESOS-3293
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test is to isolate one single process with one thread in cgroup, and 
> checks the number of processes and threads in the cgroup before/after 
> isolation. The previous test case uses "sh -c "while true; do sleep 1; 
> done;"" as the process, however, it periodically forks a child process "sleep 
> 1" in cgroup causing the test failure (expected number of process/thread 
> should be 1, but here is 2). This patch is to issue a "cat" command, so there 
> will be only one process in cgroup.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/isolator_tests.cpp 
> a25ae97a519feb8ead6177da160df8a276ca15bf 
> 
> Diff: https://reviews.apache.org/r/38454/diff/
> 
> 
> Testing
> ---
> 
> ./mesos-tests.sh 
> --gtest_filter="LimitedCpuIsolatorTest.ROOT_CGROUPS_Pids_and_Tids" 
> --gtest_repeat=1000 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 39954: Fixed spelling in comments.

[Jojy Varghese]

> On Nov. 4, 2015, 9:10 p.m., Vinod Kone wrote:
> > the diff is empty?

hmm thats strange. If you "download" the diff, you will see the patch.


- Jojy


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


On Nov. 4, 2015, 8:44 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39954/
> ---
> 
> (Updated Nov. 4, 2015, 8:44 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed spelling in comments.
> 
> 
> Diffs
> -
> 
>   support/docker_build.sh  
> 
> Diff: https://reviews.apache.org/r/39954/diff/
> 
> 
> Testing
> ---
> 
> locally ran the script
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

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/#review105142
---

Ship it!



src/slave/slave.cpp (line 3343)


Can you add a NOTE here saying that if switch_user flag is false and the 
slave runs under a non-root user, the task will be rejected by the Posix 
filesystem isolator. Linux filesystem isolator requires slave to have root 
permission.


- Jie Yu


On Nov. 4, 2015, 8:59 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38900/
> ---
> 
> (Updated Nov. 4, 2015, 8:59 p.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 de6b58a93346c618a9214032d891c1004203ca56 
>   src/slave/constants.cpp b69471b2d57aad0c254ef3bb7dce9405abeab93a 
>   src/slave/slave.hpp e6fa66b40c7f17c500056b7d6f95d7e795a16ca0 
>   src/slave/slave.cpp 5f9b52b41eaab0c24965f28e192074340e00bde5 
>   src/tests/slave_tests.cpp 91dbdba56c7d3a374e56be92d88c0b367c7a2e1c 
> 
> Diff: https://reviews.apache.org/r/38900/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 39258: Add filesystem isolator with command executor test.

[Jie Yu]

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

Ship it!



src/tests/containerizer/filesystem_isolator_tests.cpp (line 316)


remove extra space after `-d` and add a space after `+`


- Jie Yu


On Nov. 4, 2015, 9:04 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39258/
> ---
> 
> (Updated Nov. 4, 2015, 9:04 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add filesystem isolator with command executor test.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 39008f620183d242407fea5377bfceffc57b 
>   src/tests/containerizer/provisioner.hpp 
> 507e1413470ef4f36ead657203073115d5324bef 
>   src/tests/containerizer/rootfs.hpp 56a205f7c18318ab9e99f4bb8cb969ba3302baea 
> 
> Diff: https://reviews.apache.org/r/39258/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 39803: Windows: Implemented stout/os/stat.hpp`.

[Joseph Wu]

> On Nov. 3, 2015, 5:02 p.m., Michael Hopcroft wrote:
> > This is my initial set of comments. I still need to read from line 112 on 
> > in stat.hpp and I haven't started reading reparsepoint.hpp.

Replied to a few project-specific (we might call it "tribal knowledge") points 
you raised.

If you can, please drop some of them as appropriate (i.e. if no change is 
needed).


> On Nov. 3, 2015, 5:02 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp, line 
> > 70
> > 
> >
> > Is FollowSymLink your enum or an enum that already exists in the Linux 
> > version?

This one already exists.  See: 
https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/stat.hpp#L66-L70


> On Nov. 3, 2015, 5:02 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp, line 
> > 91
> > 
> >
> > I'll just ask this once here - is string concatendation the accepted 
> > way of building up error messages in Stout?

Yeah it is.  Almost everything else (besides messages for `Error`) uses streams 
though.


> On Nov. 3, 2015, 5:02 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp, line 
> > 107
> > 
> >
> > Is the enum+switch+unreachable a common pattern in stout? It seems that 
> > choosing to use an enume for follw, instead of a bool made this code much 
> > more complex.
> > 
> > Also, this is the only use of UNREACHABLE(), so you could remove the 
> > #include if you reworked the function.
> > 
> > Perhaps size() is an existing API in stout and you have no choice about 
> > its prototype? In that case, are you sure that your version of enum 
> > FollowSymlink is exactly the same as the one used in the Linux version. 
> > 
> > You would hate for someone on the Linux side to take a dependency on 
> > the order of the items inside the enum and find that you had changed that 
> > order.

Yes, `UNREACHABLE` is commonly used in enum-switch statements.  

There is another pattern of excluding a `default` and adding a blurb like:
```
// NOTE: By not setting a default we leverage the compiler
// errors when the enumeration is augmented to find all
// the cases we need to provide.
```
(We do end up using `UNREACHABLE` anyway in some of those cases too.)


> On Nov. 3, 2015, 5:02 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp,
> >  line 55
> > 
> >
> > Google Style Guide allows auto if it help readability? Is it a good or 
> > bad idea here?

In general, we don't use `auto` unless it's completely obvious from the 
right-side what the type is.  In this case, you might expect `os::realpath` to 
return a `std::string`, `Try`, `Option`, `Path`, etc.


> On Nov. 3, 2015, 5:02 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp,
> >  line 64
> > 
> >
> > Google Style Guide isn't really clear on this one. I would recommend 
> > indenting line 66 to align with the open paren on line 65.

Generally, we would go for this:
```
return Error(
"Reparse point attribute is not set for path '" +
absolutePath.get() + "', and therefore it is not a sybolic link");
```
Note: no period at the end of the message.


> On Nov. 3, 2015, 5:02 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp, line 
> > 83
> > 
> >
> > Google Style Guide discourages default arguments.

I think we're fine with default args.


- Joseph


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


On Oct. 31, 2015, 6:31 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39803/
> ---
> 
> (Updated Oct. 31, 2015, 6:31 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Implemented stout/os/stat.hpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 67b142bbc2d80f40a1e893cc5813d58dd2aa8381 
>   
> 3rdparty

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/#review105134
---



src/Makefile.am (line 564)


We use tab indentation in the Makefilm.am



src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp (line 57)


I think this is redudant.



src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp (line 69)


We talked about removing these right? And it's not matching the actual 
params



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


We don't use alias namespaces tpyically.



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


Didn''t you pull these out?



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


Move the timeout after into the RemotePullerProcess.



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


Let the caller LOG instead of us since we already forward all necessary 
information to the caller.



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


Remove extra space



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


Fix indentation



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


Let's use consistent failure messaging here.
If you like to include the layer id please include in both cases, and also 
specify the layer as well for all cases.



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


Are you trying to check for custom registry?

The Name struct has an optional registry field that you should check 
instead:

if (name.has_registry()) {
  return Error(.);
}



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


Can you rebase on all your refactoring patches?



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


Instead of doing this, how about just create a new Image::Name?



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


Remove extra space between ' and :



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


ditto



src/slave/flags.cpp (line 118)


This is only used in the remote puller for now, so perhaps say "for pulling 
images from the Docker registry"



src/slave/flags.cpp (line 123)


How about "Default Docker registry server host"



src/slave/flags.cpp (line 128)


Default Docker registry server port


- Timothy Chen


On Nov. 4, 2015, 6:09 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> ---
> 
> (Updated Nov. 4, 2015, 6:09 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/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289 
>   src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
> 87d80026939a326bd1169f46906e36d6ef19f546 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> f314f20d989ed0125e15d2a14d0f8e56c56976d5 
>   src/slave/containerizer/mesos/provisioner/docker/puller.hpp 
> 8010b8aa75a2fc2e4b537f915f9dca028e407b63 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> ad57d8592c5f9df5115b575855ed2e99a0597359 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
>

Re: Review Request 39804: Windows: Moved `os::find` to its own file, `stout/os/find.hpp`.

[Joseph Wu]

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

Ship it!


Checked that code did not change in midst of the move.


3rdparty/libprocess/3rdparty/stout/include/stout/os/find.hpp (line 16)


```
#include 
#include 

#include 
```



3rdparty/libprocess/3rdparty/stout/include/stout/os/find.hpp (line 50)


This was originally present, but you can remove the space between the `> >`.


- Joseph Wu


On Oct. 31, 2015, 5:28 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39804/
> ---
> 
> (Updated Oct. 31, 2015, 5:28 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Moved `os::find` to its own file, `stout/os/find.hpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 67b142bbc2d80f40a1e893cc5813d58dd2aa8381 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> fc2df6831ae2cb1a91c7a8cc92965939576e575d 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/find.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39804/diff/
> 
> 
> Testing
> ---
> 
> `make check` from autotools on Ubuntu 15.
> `make check` from CMake on OS X 10.10.
> Ran `check` project in VS on Windows 10.
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 39250: Puller refactor: moved untar to a common place

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [39838]

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

Error:
 2015-11-04 21:55:30 URL:https://reviews.apache.org/r/39838/diff/raw/ 
[1452/1452] -> "39838.patch" [1]
error: patch failed: src/tests/containerizer/provisioner_docker_tests.cpp:686
error: src/tests/containerizer/provisioner_docker_tests.cpp: patch does not 
apply
Failed to apply patch

- Mesos ReviewBot


On Nov. 4, 2015, 6:09 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39250/
> ---
> 
> (Updated Nov. 4, 2015, 6:09 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Puller refactor: moved untar to a common place
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> f314f20d989ed0125e15d2a14d0f8e56c56976d5 
>   src/slave/containerizer/mesos/provisioner/docker/puller.hpp 
> 8010b8aa75a2fc2e4b537f915f9dca028e407b63 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2 
>   src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39250/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39805: Moved filesystems tests to their own file.

[Joseph Wu]

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

Ship it!


Kudos for alphabetizing the tests by name.


3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp (line 19)


```
#include 
#include 
```



3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp (lines 20 - 21)


Add `#include `


- Joseph Wu


On Oct. 31, 2015, 6:22 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39805/
> ---
> 
> (Updated Oct. 31, 2015, 6:22 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved filesystems tests to their own file.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 
> 76e1674e08bbe65a4fdf86731823a61f231d6d12 
>   3rdparty/libprocess/3rdparty/stout/include/stout/tests/utils.hpp 
> 8934ba79685685d5f5eae5991057c6ad3c8eea8e 
>   3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
> 26e1377ee625748b7fdbec327fef9ac602535f83 
>   3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
> 5a1da57f7e27cf8154f0d5f6efd47dcee8a430ff 
> 
> Diff: https://reviews.apache.org/r/39805/diff/
> 
> 
> Testing
> ---
> 
> `make check` from autotools on Ubuntu 15.
> `make check` from CMake on OS X 10.10.
> Ran `check` project in VS on Windows 10.
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 39834: Made `path_tests.cpp` standalone.

[Joseph Wu]

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

Ship it!



3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp (lines 21 - 22)


You might also want ``, `, and 
``.


- Joseph Wu


On Nov. 2, 2015, 10:36 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39834/
> ---
> 
> (Updated Nov. 2, 2015, 10:36 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `path_tests.cpp` standalone.
> 
> NB, more of these tests will move when `Hopcroft` is done with his 
> `os::symlink` changeset.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
> ad9ce324eaf940f68d04c6db7ba37d05efb1216a 
> 
> Diff: https://reviews.apache.org/r/39834/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

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/#review105151
---



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


This was done in another patch by Ben M (currently in registry_client.cpp). 
So i thought the style was safe.


- Jojy Varghese


On Nov. 4, 2015, 6:09 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> ---
> 
> (Updated Nov. 4, 2015, 6:09 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/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289 
>   src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
> 87d80026939a326bd1169f46906e36d6ef19f546 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> f314f20d989ed0125e15d2a14d0f8e56c56976d5 
>   src/slave/containerizer/mesos/provisioner/docker/puller.hpp 
> 8010b8aa75a2fc2e4b537f915f9dca028e407b63 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> ad57d8592c5f9df5115b575855ed2e99a0597359 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
>   src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> bb02d650e16d45fcf337a7954f7a26143fb2c69f 
>   src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73 
>   src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 8d90894410cd834edf49a2814d1b616718798fe8 
> 
> Diff: https://reviews.apache.org/r/38580/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38580: Added docker registry RemotePuller

[Jojy Varghese]

> On Nov. 4, 2015, 9:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 51
> > 
> >
> > Didn''t you pull these out?

Not sure what you mean.


> On Nov. 4, 2015, 9:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 129
> > 
> >
> > Move the timeout after into the RemotePullerProcess.

Since RemotePullerProcess does many sub-steps, how do I set  a timer for all of 
the steps aggregated?


> On Nov. 4, 2015, 9:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 133
> > 
> >
> > Let the caller LOG instead of us since we already forward all necessary 
> > information to the caller.

I wanted context of the failure logged out at the source of it to help debug 
issues.


> On Nov. 4, 2015, 9:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 236
> > 
> >
> > Let's use consistent failure messaging here.
> > If you like to include the layer id please include in both cases, and 
> > also specify the layer as well for all cases.

Not sure i understand.


> On Nov. 4, 2015, 9:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 295
> > 
> >
> > Are you trying to check for custom registry?
> > 
> > The Name struct has an optional registry field that you should check 
> > instead:
> > 
> > if (name.has_registry()) {
> >   return Error(.);
> > }

The issue is that we dont know what is being passed as Image::Name. Is it the 
local name or canonical name. If its local name, checking for "/" is 
sufficient. The idea is that we dont accept any paths in the name.


> On Nov. 4, 2015, 9:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 373
> > 
> >
> > Instead of doing this, how about just create a new Image::Name?

The intent here is that we create  the new image name exactly as the one passed 
one expect the repository name.


- Jojy


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


On Nov. 4, 2015, 6:09 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> ---
> 
> (Updated Nov. 4, 2015, 6:09 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/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289 
>   src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
> 87d80026939a326bd1169f46906e36d6ef19f546 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> f314f20d989ed0125e15d2a14d0f8e56c56976d5 
>   src/slave/containerizer/mesos/provisioner/docker/puller.hpp 
> 8010b8aa75a2fc2e4b537f915f9dca028e407b63 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> ad57d8592c5f9df5115b575855ed2e99a0597359 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
>   src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> bb02d650e16d45fcf337a7954f7a26143fb2c69f 
>   src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73 
>   src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 8d90894410cd834edf49a2814d1b616718798fe8 
> 
> Diff: https://reviews.apache.org/r/38580/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38883: Removed calls to apply-review.sh script. Added support for amending commit messages.

[Artem Harutyunyan]

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

(Updated Nov. 4, 2015, 2:30 p.m.)


Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, 
and Vinod Kone.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  support/apply-reviews.py PRE-CREATION 

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


Testing
---

Tested the script with python 2.7.


Thanks,

Artem Harutyunyan

Re: Review Request 38580: Added docker registry RemotePuller

[Timothy Chen]

> On Nov. 4, 2015, 9:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 373
> > 
> >
> > Instead of doing this, how about just create a new Image::Name?
> 
> Jojy Varghese wrote:
> The intent here is that we create  the new image name exactly as the one 
> passed one expect the repository name.

Which is I thought it's weird that we have to also set another name, I would 
thought having a helper function that just gives us the right remote 
Image::Name and we put all the details there is better.


- Timothy


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


On Nov. 4, 2015, 6:09 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> ---
> 
> (Updated Nov. 4, 2015, 6:09 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/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289 
>   src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
> 87d80026939a326bd1169f46906e36d6ef19f546 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> f314f20d989ed0125e15d2a14d0f8e56c56976d5 
>   src/slave/containerizer/mesos/provisioner/docker/puller.hpp 
> 8010b8aa75a2fc2e4b537f915f9dca028e407b63 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> ad57d8592c5f9df5115b575855ed2e99a0597359 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
>   src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> bb02d650e16d45fcf337a7954f7a26143fb2c69f 
>   src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73 
>   src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 8d90894410cd834edf49a2814d1b616718798fe8 
> 
> Diff: https://reviews.apache.org/r/38580/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38580: Added docker registry RemotePuller

[Timothy Chen]

> On Nov. 4, 2015, 10:05 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 43
> > 
> >
> > This was done in another patch by Ben M (currently in 
> > registry_client.cpp). So i thought the style was safe.

I think you're right, feel free to drop it.


- Timothy


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


On Nov. 4, 2015, 6:09 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> ---
> 
> (Updated Nov. 4, 2015, 6:09 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/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289 
>   src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
> 87d80026939a326bd1169f46906e36d6ef19f546 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> f314f20d989ed0125e15d2a14d0f8e56c56976d5 
>   src/slave/containerizer/mesos/provisioner/docker/puller.hpp 
> 8010b8aa75a2fc2e4b537f915f9dca028e407b63 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> ad57d8592c5f9df5115b575855ed2e99a0597359 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
>   src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> bb02d650e16d45fcf337a7954f7a26143fb2c69f 
>   src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73 
>   src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 8d90894410cd834edf49a2814d1b616718798fe8 
> 
> Diff: https://reviews.apache.org/r/38580/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>