Re: Review Request 39088: Made shell test locale-independent.

[Alexander Rojas]

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

Ship it!


Ship It!

- Alexander Rojas


On Oct. 8, 2015, 3:27 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39088/
> ---
> 
> (Updated Oct. 8, 2015, 3:27 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Jan Schlicht, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3517
> https://issues.apache.org/jira/browse/MESOS-3517
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test environment does not enforce that we run with a defined locale,
> so instead enforce it for our particular use case (here: potentially
> localized error messages from shell).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
> e6d36ec1bf414b52d0899f0edf83e0ad8910dd0e 
> 
> Diff: https://reviews.apache.org/r/39088/diff/
> 
> 
> Testing
> ---
> 
> With German locale installed:
> 
> % LANG=de_DE.UTF-8 make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 39219: Windows: Added support for `slave/state.cpp`.

[Alex Clemmer]

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

Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
Wu.


Bugs: MESOS-3615, MESOS-3627, MESOS-3628, MESOS-3629, MESOS-3630, MESOS-3631, 
MESOS-3633, MESOS-3658, MESOS-3659, MESOS-3660, and MESOS-3693
https://issues.apache.org/jira/browse/MESOS-3615
https://issues.apache.org/jira/browse/MESOS-3627
https://issues.apache.org/jira/browse/MESOS-3628
https://issues.apache.org/jira/browse/MESOS-3629
https://issues.apache.org/jira/browse/MESOS-3630
https://issues.apache.org/jira/browse/MESOS-3631
https://issues.apache.org/jira/browse/MESOS-3633
https://issues.apache.org/jira/browse/MESOS-3658
https://issues.apache.org/jira/browse/MESOS-3659
https://issues.apache.org/jira/browse/MESOS-3660
https://issues.apache.org/jira/browse/MESOS-3693


Repository: mesos


Description
---

Windows: Added support for `slave/state.cpp`.


Diffs
-

  src/slave/state.hpp 5a1a9bb2c86639612a8f065b7a66c8179696297a 
  src/slave/state.cpp 81c4b96d879f974f0dfba3fb977184122eab 

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


Testing
---

Built CMake solution and ran `make check` (or equivalent) on Windows 10, Ubuntu 
15, OS X 10.10. Built autotools solution and ran `make check` on Ubuntu 15.


Thanks,

Alex Clemmer

Review Request 39217: Windows: Added `stout/os/chsize.hpp`.

[Alex Clemmer]

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

Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
Wu.


Repository: mesos


Description
---

Windows: Added `stout/os/chsize.hpp`.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
9e9c3119ad18f4cbc70c70095c71dc4fd19553df 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/chsize.hpp PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/chsize.hpp 
PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/chsize.hpp 
PRE-CREATION 

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


Testing
---

Built CMake solution and ran `make check` (or equivalent) on Windows 10, Ubuntu 
15, OS X 10.10. Built autotools solution and ran `make check` on Ubuntu 15.


Thanks,

Alex Clemmer

Review Request 39220: CMake: Added `slave/state.cpp` to Windows builds.

[Alex Clemmer]

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

Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
Wu.


Bugs: MESOS-3615, MESOS-3627, MESOS-3628, MESOS-3629, MESOS-3630, MESOS-3631, 
MESOS-3633, MESOS-3658, MESOS-3659, MESOS-3660, and MESOS-3693
https://issues.apache.org/jira/browse/MESOS-3615
https://issues.apache.org/jira/browse/MESOS-3627
https://issues.apache.org/jira/browse/MESOS-3628
https://issues.apache.org/jira/browse/MESOS-3629
https://issues.apache.org/jira/browse/MESOS-3630
https://issues.apache.org/jira/browse/MESOS-3631
https://issues.apache.org/jira/browse/MESOS-3633
https://issues.apache.org/jira/browse/MESOS-3658
https://issues.apache.org/jira/browse/MESOS-3659
https://issues.apache.org/jira/browse/MESOS-3660
https://issues.apache.org/jira/browse/MESOS-3693


Repository: mesos


Description
---

CMake: Added `slave/state.cpp` to Windows builds.


Diffs
-

  CMakeLists.txt a5a66c13496a95bb7095504a4645909d887fb49f 
  src/CMakeLists.txt 828dbb690841c561816811dfbb044aa3afead89d 

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


Testing
---

Built CMake solution and ran `make check` (or equivalent) on Windows 10, Ubuntu 
15, OS X 10.10. Built autotools solution and ran `make check` on Ubuntu 15.


Thanks,

Alex Clemmer

Re: Review Request 39213: Windows: Moved `bootId` to is own file, `stout/os/windows/bootid.hpp`.

[Alex Clemmer]

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

(Updated Oct. 12, 2015, 6:16 a.m.)


Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
Wu.


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


Repository: mesos


Description
---

Windows: Moved `bootId` to is own file, `stout/os/windows/bootid.hpp`.

(NOTE: this does not close MESOS-3632, but does contribute to its resolution.)


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
9e9c3119ad18f4cbc70c70095c71dc4fd19553df 
  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
5141c1369af60afd6cd5c70c6295d575ea960a83 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/bootid.hpp PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/bootid.hpp 
PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/bootid.hpp 
PRE-CREATION 

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


Testing
---

Built CMake solution and ran `make check` (or equivalent) on Windows 10, Ubuntu 
15, OS X 10.10. Built autotools solution and ran `make check` on Ubuntu 15.


Thanks,

Alex Clemmer

Re: Review Request 39008: Used thread-safe replacement for strerror.

[Benjamin Bannier]

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

(Updated Oct. 12, 2015, 7:09 a.m.)


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


Changes
---

Fixed includes for Linux.


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


Repository: mesos


Description
---

Switch call sites to using safe strerror_r wrapper.


Diffs (updated)
-

  src/cli/mesos.cpp 80c3c1a7e30e7e148e17c379ec6824ab7e4c0f12 
  src/files/files.cpp 08e76b95b632b6fb9c82666550d0ae3c4e1a1a89 
  src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
  src/linux/routing/link/internal.hpp 015c0ef5be516d7786c96a96437cced1ae8487fa 
  src/linux/routing/link/link.cpp 8ea3e31e0f64c7b653f208ec74bb389a702b357a 
  src/slave/containerizer/docker.cpp 174448cc039b316329a8617f488f037a1e640e23 
  src/slave/containerizer/isolators/filesystem/linux.cpp 
8823b7850a1ac17fc4f4868aadf1b04428d2381b 
  src/slave/containerizer/isolators/filesystem/posix.cpp 
eec510c4f7655d67b33ad90210eeb57fcc910684 
  src/slave/containerizer/isolators/filesystem/shared.cpp 
73804ca5a8a3bf03e13c74a247b5c21e9af5f040 
  src/slave/containerizer/mesos/containerizer.cpp 
b904b2d88e9b62fa4ba312c4569a4d89b0dc6052 
  src/slave/containerizer/mesos/launch.cpp 
09d4d8f4d6837e93a82deef76ca07e2167d6a405 
  src/slave/containerizer/provisioner/backends/bind.cpp 
1fe1746c0bc1c9c12e1378e6438122a91b58316b 
  src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c 
  src/tests/containerizer/memory_test_helper.cpp 
8109a4314c0dcf17c5fe124d9b87ac856b3a922a 
  src/tests/script.cpp bcc1fab912410237dfe903d7a36cad9323d625a0 

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


Testing
---

make check


Thanks,

Benjamin Bannier

Re: Review Request 38977: Add reason for declineOffer

[Guangya Liu]

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

(Updated 十月 12, 2015, 7:46 a.m.)


Review request for mesos, BenjaminVW BenjaminVW, Joseph Wu, and Vinod Kone.


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


Repository: mesos


Description
---

Since this depends on MESOS-3522, so this patch only update the
interface and proto file.


Diffs (updated)
-

  include/mesos/scheduler.hpp f571d42d1508632152473c4f4ade60ae3900fce1 
  include/mesos/v1/scheduler/scheduler.proto 
871c2ef00b13eb93f65119096577ed45a95aead3 
  src/sched/sched.cpp 724d7c0cef073342436de7cf5fe834fffbf0 

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


Testing
---

Ubuntu 14.04
make
make check


Thanks,

Guangya Liu

Review Request 39218: Windows: Added support for `process/address.hpp`.

[Alex Clemmer]

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

Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
Wu.


Bugs: MESOS-3628, MESOS-3629, MESOS-3630, MESOS-3631, and MESOS-3693
https://issues.apache.org/jira/browse/MESOS-3628
https://issues.apache.org/jira/browse/MESOS-3629
https://issues.apache.org/jira/browse/MESOS-3630
https://issues.apache.org/jira/browse/MESOS-3631
https://issues.apache.org/jira/browse/MESOS-3693


Repository: mesos


Description
---

Windows: Added support for `process/address.hpp`.


Diffs
-

  3rdparty/libprocess/include/process/address.hpp 
bf230ac1a401284f4d4abdbaa53f5b8b9d83c000 

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


Testing
---

Built CMake solution and ran `make check` (or equivalent) on Windows 10, Ubuntu 
15, OS X 10.10. Built autotools solution and ran `make check` on Ubuntu 15.


Thanks,

Alex Clemmer

Re: Review Request 39220: CMake: Added `slave/state.cpp` to Windows builds.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39019, 39076, 39091, 39092, 39093, 39096, 39097, 39180, 
39181, 39201, 39182, 39202, 39203, 39204, 39207, 39208, 39209, 39210, 39213, 
39217, 39218, 39219, 39220]

All tests passed.

- Mesos ReviewBot


On Oct. 12, 2015, 6:53 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39220/
> ---
> 
> (Updated Oct. 12, 2015, 6:53 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Bugs: MESOS-3615, MESOS-3627, MESOS-3628, MESOS-3629, MESOS-3630, MESOS-3631, 
> MESOS-3633, MESOS-3658, MESOS-3659, MESOS-3660, and MESOS-3693
> https://issues.apache.org/jira/browse/MESOS-3615
> https://issues.apache.org/jira/browse/MESOS-3627
> https://issues.apache.org/jira/browse/MESOS-3628
> https://issues.apache.org/jira/browse/MESOS-3629
> https://issues.apache.org/jira/browse/MESOS-3630
> https://issues.apache.org/jira/browse/MESOS-3631
> https://issues.apache.org/jira/browse/MESOS-3633
> https://issues.apache.org/jira/browse/MESOS-3658
> https://issues.apache.org/jira/browse/MESOS-3659
> https://issues.apache.org/jira/browse/MESOS-3660
> https://issues.apache.org/jira/browse/MESOS-3693
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake: Added `slave/state.cpp` to Windows builds.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt a5a66c13496a95bb7095504a4645909d887fb49f 
>   src/CMakeLists.txt 828dbb690841c561816811dfbb044aa3afead89d 
> 
> Diff: https://reviews.apache.org/r/39220/diff/
> 
> 
> Testing
> ---
> 
> Built CMake solution and ran `make check` (or equivalent) on Windows 10, 
> Ubuntu 15, OS X 10.10. Built autotools solution and ran `make check` on 
> Ubuntu 15.
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 39216: Rename http_api_tests.cpp to scheduler_http_api_tests.cpp

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39216]

All tests passed.

- Mesos ReviewBot


On Oct. 12, 2015, 5:10 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39216/
> ---
> 
> (Updated Oct. 12, 2015, 5:10 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3594
> https://issues.apache.org/jira/browse/MESOS-3594
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rename http_api_tests.cpp to scheduler_http_api_tests.cpp
> 
> 
> Diffs
> -
> 
>   src/Makefile.am d855cb83277c3e0e2ee3feacaf6ad0962223ef6e 
>   src/tests/http_api_tests.cpp df87afe01edc27504c973efbc4061cfb7cc808f5 
> 
> Diff: https://reviews.apache.org/r/39216/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

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

[Guangya Liu]

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



include/mesos/resources.hpp (line 312)


Can you please make those comments using 
https://github.com/apache/mesos/blob/master/docs/doxygen-style-guide.md ?


- Guangya Liu


On 十月 11, 2015, 3:08 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> ---
> 
> (Updated 十月 11, 2015, 3:08 a.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, with one notable 
> failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue 
> tracked here: https://issues.apache.org/jira/browse/MESOS-3698
> 
> 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 37996: Added InheritanceTree a tree based container where children nodes inherit the values associated with their parent.

[Alexander Rojas]

> On Oct. 6, 2015, 2:39 p.m., Bernd Mathiske wrote:
> > Ship It!
> 
> Ben Mahler wrote:
> I don't think we should introduce this into stout in its current form. I 
> realize you're planning to use this for authentication stuff, but looking at 
> this on its own, it seems like a confusing abstraction. Why would we couple 
> the notion of a Tree with the semantics around properties and property 
> inheritance?
> 
> Till Toenshoff wrote:
> This code is being used in libprocess. So the options are libprocess or 
> stout for introducing it. I believe it would be a better fit for stout than 
> for libprocess as it is a data structure implementation that has no threading 
> or process specifics. Given that it already is in a reusable state, I think 
> that we should go as proposed by this RR.
> 
> Bernd Mathiske wrote:
> @bmahler, what would you rather have us do? This structure is quite close 
> to Boost's PropertyTree 
> (http://www.boost.org/doc/libs/1_59_0/doc/html/property_tree.html), and we 
> would have loved to use that one. But it does not feature inheritance, which 
> seems a natural, essential thing to have in at least some trees.
> 
> Ben Mahler wrote:
> Hm... the "inheritance" stuff that you guys need seems to just be a walk 
> of the tree from the root to the leaf you're interested in, collecting 
> properties of nodes along the way. Any reason you can't layer that 
> functionality on top or, even better, on the side via a free standing 
> function so that you don't need to introduce a new data structure?
> 
> Jie Yu wrote:
> +1 if we can compose/inherit from boost property tree rather than 
> implementing our own.

Property tree as of the moment is not distributed with Mesos. However, there is 
a reason why it wasn't added, and it is that it will require a double parsing 
of the path (one when asking the ptree, the other manually to verify every 
subpath).


- Alexander


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


On Oct. 7, 2015, 11:54 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37996/
> ---
> 
> (Updated Oct. 7, 2015, 11:54 a.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 `InheritanceTree` class which allows to create a tree where 
> nodes can be tag with some property. This property is then inherited by 
> children nodes.
> 
> Two behaviors are implemented, overriding, i.e. Adding a property to the 
> child node of another node with a property already will result in the 
> ancestor property being lost. The second behavior, accumulating, takes a 
> function and accumulates
> properties of all ancestors.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 
> 76e1674e08bbe65a4fdf86731823a61f231d6d12 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 9e9c3119ad18f4cbc70c70095c71dc4fd19553df 
>   3rdparty/libprocess/3rdparty/stout/include/stout/inheritancetree.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
> 94292f8a46ec31bbaf6e52f48109322bbe123f70 
>   3rdparty/libprocess/3rdparty/stout/tests/inheritancetree_tests.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37996/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 39088: Made shell test locale-independent.

[haosdent huang]

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

Ship it!


Ship It!

- haosdent huang


On Oct. 8, 2015, 1:27 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39088/
> ---
> 
> (Updated Oct. 8, 2015, 1:27 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Jan Schlicht, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3517
> https://issues.apache.org/jira/browse/MESOS-3517
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test environment does not enforce that we run with a defined locale,
> so instead enforce it for our particular use case (here: potentially
> localized error messages from shell).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
> e6d36ec1bf414b52d0899f0edf83e0ad8910dd0e 
> 
> Diff: https://reviews.apache.org/r/39088/diff/
> 
> 
> Testing
> ---
> 
> With German locale installed:
> 
> % LANG=de_DE.UTF-8 make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 39135: Fixed teardown test

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 十月 8, 2015, 6:25 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39135/
> ---
> 
> (Updated 十月 8, 2015, 6:25 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This teardown endpoint test needs valid credentials to really test a missing 
> 'frameworkID' request.
> Note: this change is also needed as dependant patch fixes how we validate 
> credentials in master endpoints.
> 
> 
> Diffs
> -
> 
>   src/tests/teardown_tests.cpp 2eeead7 
> 
> Diff: https://reviews.apache.org/r/39135/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38977: Add reason for declineOffer

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38977]

All tests passed.

- Mesos ReviewBot


On Oct. 12, 2015, 7:46 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38977/
> ---
> 
> (Updated Oct. 12, 2015, 7:46 a.m.)
> 
> 
> Review request for mesos, BenjaminVW BenjaminVW, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-3041
> https://issues.apache.org/jira/browse/MESOS-3041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since this depends on MESOS-3522, so this patch only update the
> interface and proto file.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler.hpp f571d42d1508632152473c4f4ade60ae3900fce1 
>   include/mesos/v1/scheduler/scheduler.proto 
> 871c2ef00b13eb93f65119096577ed45a95aead3 
>   src/sched/sched.cpp 724d7c0cef073342436de7cf5fe834fffbf0 
> 
> Diff: https://reviews.apache.org/r/38977/diff/
> 
> 
> Testing
> ---
> 
> Ubuntu 14.04
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 39205: Deprecate resource_monitoring_interval flag

[haosdent huang]

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

Ship it!


Ship It!

- haosdent huang


On Oct. 12, 2015, 3:32 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39205/
> ---
> 
> (Updated Oct. 12, 2015, 3:32 a.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3700
> https://issues.apache.org/jira/browse/MESOS-3700
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This flag should be deprecated after 0.23.0 as it is no use now.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2ab476a2d2c43e309b570d73ecac80e27b296e7e 
>   docs/oversubscription.md 5a31b1ff7f003307817732a71f3b0a7c7d60cd24 
>   src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341 
>   src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55 
>   src/slave/flags.hpp f76f0f6a0c7a01c7f061a225d7f6ef52be0ee7b5 
>   src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 
> 
> Diff: https://reviews.apache.org/r/39205/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

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

[Alexander Rojas]

> On Oct. 10, 2015, 12:10 p.m., Alex Clemmer wrote:
> > src/Makefile.am, line 463
> > 
> >
> > (I hesistate to comment this time, but...) could we add this to the 
> > `CMakeLists.txt` file in `src/`? (It looks to be a `.cpp` file, and we 
> > appear to have the other authentication `.cpp` files, so I think this one 
> > is a legitimate line we should add :) )

Not sure if I added all the files I needed to, any reason why we cannot use 
`FILE(GLOB_RECURSE …)`


- Alexander


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


On Oct. 12, 2015, 11:30 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38950/
> ---
> 
> (Updated Oct. 12, 2015, 11:30 a.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3232
> https://issues.apache.org/jira/browse/MESOS-3232
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds support for modularization of HTTP Authenticators. 
> 
> It includes an example of how to do it with the Basic HTTP Authenticator.
> 
> 
> Diffs
> -
> 
>   include/mesos/authentication/http/basic_authenticator_factory.hpp 
> PRE-CREATION 
>   include/mesos/module/http_authenticator.hpp PRE-CREATION 
>   src/CMakeLists.txt 828dbb690841c561816811dfbb044aa3afead89d 
>   src/Makefile.am d855cb83277c3e0e2ee3feacaf6ad0962223ef6e 
>   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 39099: Changed secret field in Credential from 'bytes' to 'string' for V1

[Guangya Liu]

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


This one need a rebase

- Guangya Liu


On 十月 7, 2015, 7:10 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39099/
> ---
> 
> (Updated 十月 7, 2015, 7:10 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change is introduced in dependant patch and we should update V1 proto as 
> well.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/mesos.proto eadbc9d 
> 
> Diff: https://reviews.apache.org/r/39099/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

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 Oct. 12, 2015, 11:30 a.m.)


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


Changes
---

Added new files to CMakeLists.txt


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


Repository: mesos


Description
---

Adds support for modularization of HTTP Authenticators. 

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


Diffs (updated)
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
PRE-CREATION 
  include/mesos/module/http_authenticator.hpp PRE-CREATION 
  src/CMakeLists.txt 828dbb690841c561816811dfbb044aa3afead89d 
  src/Makefile.am d855cb83277c3e0e2ee3feacaf6ad0962223ef6e 
  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 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/#review102237
---

Ship it!


Ship It!

- Guangya Liu


On 十月 2, 2015, 8:11 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
> 
> (Updated 十月 2, 2015, 8:11 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3232
> https://issues.apache.org/jira/browse/MESOS-3232
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows developers to provide their own parameters when loading modules 
> instead of using the ones provided by the user when loading Mesos. This helps 
> to deal with default modules (those used when the user doesn't provide any), 
> and for testing of the modules.
> 
> 
> Diffs
> -
> 
>   src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
> 
> Diff: https://reviews.apache.org/r/38627/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 39008: Used thread-safe replacement for strerror.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39005, 39008]

All tests passed.

- Mesos ReviewBot


On Oct. 12, 2015, 7:09 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39008/
> ---
> 
> (Updated Oct. 12, 2015, 7:09 a.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
> ---
> 
> Switch call sites to using safe strerror_r wrapper.
> 
> 
> Diffs
> -
> 
>   src/cli/mesos.cpp 80c3c1a7e30e7e148e17c379ec6824ab7e4c0f12 
>   src/files/files.cpp 08e76b95b632b6fb9c82666550d0ae3c4e1a1a89 
>   src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
>   src/linux/routing/link/internal.hpp 
> 015c0ef5be516d7786c96a96437cced1ae8487fa 
>   src/linux/routing/link/link.cpp 8ea3e31e0f64c7b653f208ec74bb389a702b357a 
>   src/slave/containerizer/docker.cpp 174448cc039b316329a8617f488f037a1e640e23 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 8823b7850a1ac17fc4f4868aadf1b04428d2381b 
>   src/slave/containerizer/isolators/filesystem/posix.cpp 
> eec510c4f7655d67b33ad90210eeb57fcc910684 
>   src/slave/containerizer/isolators/filesystem/shared.cpp 
> 73804ca5a8a3bf03e13c74a247b5c21e9af5f040 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b904b2d88e9b62fa4ba312c4569a4d89b0dc6052 
>   src/slave/containerizer/mesos/launch.cpp 
> 09d4d8f4d6837e93a82deef76ca07e2167d6a405 
>   src/slave/containerizer/provisioner/backends/bind.cpp 
> 1fe1746c0bc1c9c12e1378e6438122a91b58316b 
>   src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c 
>   src/tests/containerizer/memory_test_helper.cpp 
> 8109a4314c0dcf17c5fe124d9b87ac856b3a922a 
>   src/tests/script.cpp bcc1fab912410237dfe903d7a36cad9323d625a0 
> 
> Diff: https://reviews.apache.org/r/39008/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 39100: Changed Credential validation

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 十月 8, 2015, 6:26 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39100/
> ---
> 
> (Updated 十月 8, 2015, 6:26 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3560
> https://issues.apache.org/jira/browse/MESOS-3560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We should validate credentials before anything else in HTTP endpoints.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4b9f9ed 
> 
> Diff: https://reviews.apache.org/r/39100/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 37813: Added additional diagnostic output when a fetcher cache test fails.

[Bernd Mathiske]

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

(Updated Oct. 12, 2015, 9:25 a.m.)


Review request for mesos, Benjamin Hindman, Ben Mahler, Joseph Wu, and Till 
Toenshoff.


Changes
---

Followed Joseph's suggestions.


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


Repository: mesos


Description
---

Dumps all involved task/executor sandbox contents in test tear down
only if a failure occurred.


Diffs (updated)
-

  src/tests/fetcher_cache_tests.cpp 7e194dc6e2b2d8b857e61d1a18d696545a86ce9f 

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


Testing
---

make check on OSX, where the bug showed up.


Thanks,

Bernd Mathiske

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

[Alexander Rukletsov]

> On Sept. 29, 2015, 10:43 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 773-774
> > 
> >
> > Could you please help me understand why this check is necessary? IIUC, 
> > compiler ensures `T` is a protobuf becasue you pass `elem` of type `T` to 
> > `JSON::protobuf()`.
> > 
> > AFAIK, the only reason to do this check is to prohibit arguments like 
> > `google::protobuf::RepeatedPtrField`.
> >  Do you want to express this? I don't think it is necessary, because 
> > `JSON::Array` filled with `JSON::Array` is fine.
> > 
> > @MPark, what'd you say?
> 
> Klaus Ma wrote:
> To me, this's a safe guard to make sure this function is not abused.
> For 
> `google::protobuf::RepeatedPtrField`, 
> I think we need to define a message to include 
> `google::protobuf::RepeatedPtrField`, the proto file looks like:
> 
> message MyMessage
> {
> message Item {
> string value = 1;
> }
> repeated Item values = 1;
> }
> 
> Alexander Rukletsov wrote:
> Hmmm, I see your point. I would leave this decision to your shepherd!
> 
> Michael Park wrote:
> While `RepeatedPtrField` => `Array` is a 
> fine mapping in and of itself, `RepeatedPtrField` would 
> not arise from a legitimate protobuf definition. In order to prevent from 
> manually constructed instances of `RepeatedPtrField` 
> being passed around, I'm for keeping the `static_assert` here.

Let's document this in the code. After that feel free to fix/drop the issue.


- Alexander


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


On Oct. 4, 2015, 11:29 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 4, 2015, 11:29 a.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
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   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 37336: Simplified the caller interface to process::Subprocess

[Joris Van Remoortere]

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



3rdparty/libprocess/include/process/subprocess.hpp (line 68)


`{` on new line



3rdparty/libprocess/include/process/subprocess.hpp (line 94)


full words for variable names.



3rdparty/libprocess/include/process/subprocess.hpp (line 281)


Is this API something you agreed on with your shepherd? I'm curious why we 
provide a blocking function rather than returning a future with the provided 
timeout.



3rdparty/libprocess/include/process/subprocess.hpp (line 292)


If this value is not meaningful until wait is finished, why not make this a 
future bound by that condition rather than providing undefined behavior?


- Joris Van Remoortere


On Oct. 12, 2015, 12:29 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37336/
> ---
> 
> (Updated Oct. 12, 2015, 12:29 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-3035
> https://issues.apache.org/jira/browse/MESOS-3035
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-3035
> 
> The original API for `process::Subprocess` still left a lot of legwork
> to do for the caller; we have now added a `wait(Duration timeout)` method
> that returns a `CommandResult` (also introduced with this patch) which
> contains useful information about the command invocation; the exit code;
> stdout and, optionally, stderr too.
> 
> The `wait()` method will wait for both the process to terminate, and
> stdout/stderr to be available to read from; it also "unpacks" them into
> ready-to-use `string`s.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess.hpp 
> d2341a53aac7c779668ee80983f73158fd44bff5 
>   3rdparty/libprocess/src/subprocess.cpp 
> d6ea62ed1c914d34e0e189395831c86fff8aac22 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> ab7515325e5db0a4fd222bb982f51243d7b7e39d 
> 
> Diff: https://reviews.apache.org/r/37336/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 39135: Fixed teardown test

[Michael Park]

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

Ship it!


Ship It!

- Michael Park


On Oct. 8, 2015, 6:25 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39135/
> ---
> 
> (Updated Oct. 8, 2015, 6:25 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This teardown endpoint test needs valid credentials to really test a missing 
> 'frameworkID' request.
> Note: this change is also needed as dependant patch fixes how we validate 
> credentials in master endpoints.
> 
> 
> Diffs
> -
> 
>   src/tests/teardown_tests.cpp 2eeead7 
> 
> Diff: https://reviews.apache.org/r/39135/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

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

[Guangya Liu]

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



include/mesos/authentication/http/basic_authenticator_factory.hpp (line 33)


virtual



src/examples/test_http_authenticator_module.cpp (line 21)


put this under #include 



src/tests/http_authentication_tests.cpp (lines 19 - 34)


#include 

#include 

#include 
#include 

#include 
#include 

#include "tests/mesos.hpp"
#include "tests/module.hpp"

#include 

#include 



src/tests/http_authentication_tests.cpp (lines 42 - 46)


using std::string;

using mesos::http::BasicAuthenticatorFactory;

using process::http::Authenticator;


- Guangya Liu


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

Review Request 39230: Added check for SASL deprecation into configuration phase.

[Till Toenshoff]

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

Review request for mesos and switched to 'mcypark'.


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


Repository: mesos


Description
---

This patch also removes some useless CFLAGS handling on the SASL CRAM-MD5 
checks of the configuration.


Diffs
-

  configure.ac 66f9b32773861d2921d99189e0fbcaea48c456a9 

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


Testing
---

./configure && make check 
OSX, clang3.7 + gcc4.9


Thanks,

Till Toenshoff

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

[Marco Massenzio]

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


Would it be possible to add a few unit tests, also to show usage patterns? 
especially given the absence of any documentation, it's kinda difficult to 
figure out how is this "intended to work" and, without tests, whether it works 
at all.


src/module/manager.hpp (line 102)


would it be possible to have proper, full javadoc for this method (as well 
as the other one)?
these will be used by external module developers, so having them 
well-documented would be really great.



src/tests/module.hpp (lines 76 - 77)


same comment here - it would be great to have fully-documented, 
properly-formatted javadoc here, also explaining what could go into 
`parameters` how will this affect creating the module, etc.

while this may all appear obvious to you, it may not be so to external 
developers using this code to launch/test modules.

thanks!



src/tests/module.hpp (line 80)


(I understand this may not be your choice, but...)

I find `N` as an identifier is a poor choice because (a) it's entirely 
non-obvious *what* is it and (b) I'd expect it anyway to be an `int` of some 
sort.

Could you please consider renaming it to be something more expressive?

thanks!


- Marco Massenzio


On Oct. 2, 2015, 8:11 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
> 
> (Updated Oct. 2, 2015, 8:11 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3232
> https://issues.apache.org/jira/browse/MESOS-3232
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows developers to provide their own parameters when loading modules 
> instead of using the ones provided by the user when loading Mesos. This helps 
> to deal with default modules (those used when the user doesn't provide any), 
> and for testing of the modules.
> 
> 
> Diffs
> -
> 
>   src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
> 
> Diff: https://reviews.apache.org/r/38627/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 39230: Added check for SASL deprecation into configuration phase.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39230]

All tests passed.

- Mesos ReviewBot


On Oct. 12, 2015, 1:25 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39230/
> ---
> 
> (Updated Oct. 12, 2015, 1:25 p.m.)
> 
> 
> Review request for mesos and switched to 'mcypark'.
> 
> 
> Bugs: MESOS-3030
> https://issues.apache.org/jira/browse/MESOS-3030
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch also removes some useless CFLAGS handling on the SASL CRAM-MD5 
> checks of the configuration.
> 
> 
> Diffs
> -
> 
>   configure.ac 66f9b32773861d2921d99189e0fbcaea48c456a9 
> 
> Diff: https://reviews.apache.org/r/39230/diff/
> 
> 
> Testing
> ---
> 
> ./configure && make check 
> OSX, clang3.7 + gcc4.9
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 39223: WIP Added Quota related Tests.

[Joerg Schad]

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

(Updated Oct. 12, 2015, 4:57 p.m.)


Review request for mesos, Alexander Rukletsov and Bernd Mathiske.


Changes
---

Added Test for Non-Scalar resources.


Repository: mesos


Description
---

Right now intented for TDD.


Diffs (updated)
-

  src/tests/master_tests.cpp ee2473997ccbd1c50d0cbf65d1259ea2dfe82971 

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


Testing
---


Thanks,

Joerg Schad

Re: Review Request 37813: Added additional diagnostic output when a fetcher cache test fails.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37813]

All tests passed.

- Mesos ReviewBot


On Oct. 12, 2015, 4:25 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37813/
> ---
> 
> (Updated Oct. 12, 2015, 4:25 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Joseph Wu, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3235
> https://issues.apache.org/jira/browse/MESOS-3235
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Dumps all involved task/executor sandbox contents in test tear down
> only if a failure occurred.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 7e194dc6e2b2d8b857e61d1a18d696545a86ce9f 
> 
> Diff: https://reviews.apache.org/r/37813/diff/
> 
> 
> Testing
> ---
> 
> make check on OSX, where the bug showed up.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 39019: [WORK IN PROGRESS, NOT IMMEDIATELY NECESSARY] Windows: Added dirent compat code for non-Unix systems.

[Alex Clemmer]

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

(Updated Oct. 12, 2015, 5:44 p.m.)


Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
Wu.


Summary (updated)
-

[WORK IN PROGRESS, NOT IMMEDIATELY NECESSARY] Windows: Added dirent compat code 
for non-Unix systems.


Repository: mesos


Description
---

Windows: Added dirent compat code for non-Unix systems.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/dirent.hpp 
PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
50e35f43d87c69a83a9e7d039d1881404ea8be38 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 39211: Check for trailing characters in JSON::parse().

[Joseph Wu]

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

Ship it!



3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (line 741)


Nit: s/picojson/PicoJson/

(For consistency with other comments about PicoJson)



3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (line 754)


Nit: We don't put periods in error messages.


- Joseph Wu


On Oct. 10, 2015, 8:05 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39211/
> ---
> 
> (Updated Oct. 10, 2015, 8:05 p.m.)
> 
> 
> Review request for mesos, Adam B and Joseph Wu.
> 
> 
> Bugs: MESOS-3698
> https://issues.apache.org/jira/browse/MESOS-3698
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Check for trailing characters in JSON::parse().
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 
> 0870300c2d372722b4850155cb2a8e848b986ed5 
>   3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp 
> b95667f4c1a1c170016da024a5e4af02ccaa064e 
>   3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 
> 131d39b87e571f16c774a133eb3148e98d8b9dd4 
> 
> Diff: https://reviews.apache.org/r/39211/diff/
> 
> 
> Testing
> ---
> 
> Added tests to make sure that JSON::parse() is successfully returning errors 
> when passed invalid JSON objects. Ran `make check`.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39076: CMake: Added ability of Windows builds to include protobuf headers.

[Alex Clemmer]

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

(Updated Oct. 12, 2015, 5:51 p.m.)


Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
Wu.


Repository: mesos


Description
---

CMake: Added ability of Windows builds to include protobuf headers.


Diffs
-

  3rdparty/libprocess/cmake/Process3rdpartyConfigure.cmake 
ee1c74d31e28136bf289f4100d79a8ce568cd3af 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Michael Park]

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

Ship it!


I've shipped this patch with the minor typos and style fixes outlined below.


include/mesos/master/allocator.hpp (lines 111 - 112)


Indent 4 spaces.



include/mesos/master/allocator.hpp (line 153)


`s/new/newly/`



include/mesos/master/allocator.hpp (line 229)


`s/a/the/`



include/mesos/master/allocator.hpp (line 231)


`s/invokved/invoked/`
`s/a whitelist flag/the --whitelist flag`



include/mesos/master/allocator.hpp (line 254)


`s/reservationa/reservation/`



include/mesos/master/allocator.hpp (line 333)


`s/when//`



include/mesos/master/allocator.hpp (line 342)


`s/for/to/`


- Michael Park


On Oct. 6, 2015, 12:26 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Oct. 6, 2015, 12:26 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 8100f14220599247a340c919a03f29755b5349d8 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

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 Oct. 12, 2015, 6:24 p.m.)


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


Changes
---

Changed comments in resources.hpp to Doxygen format.


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, with one notable failure 
(ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: 
https://issues.apache.org/jira/browse/MESOS-3698

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 39211: Check for trailing characters in JSON::parse().

[Greg Mann]

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

(Updated Oct. 12, 2015, 6:29 p.m.)


Review request for mesos, Adam B and Joseph Wu.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Check for trailing characters in JSON::parse().


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 
0870300c2d372722b4850155cb2a8e848b986ed5 
  3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp 
b95667f4c1a1c170016da024a5e4af02ccaa064e 
  3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 
131d39b87e571f16c774a133eb3148e98d8b9dd4 

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


Testing
---

Added tests to make sure that JSON::parse() is successfully returning errors 
when passed invalid JSON objects. Ran `make check`.


Thanks,

Greg Mann

Re: Review Request 38746: Added TaskStatus::Reason to containerizer Termination message.

[Jie Yu]

> On Oct. 9, 2015, 4:52 a.m., Timothy Chen wrote:
> > include/mesos/mesos.proto, line 1121
> > 
> >
> > What's the reasoning behind this order?

Reordered according to alphabet.


- Jie


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


On Oct. 9, 2015, 1:28 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38746/
> ---
> 
> (Updated Oct. 9, 2015, 1:28 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2035
> https://issues.apache.org/jira/browse/MESOS-2035
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added TaskStatus::Reason to containerizer Termination message.
> 
> The following doc summarized the problem and proposed a solution:
> https://docs.google.com/document/d/1klGDAu5yBVf-CGWLqvELLIfxLfRaisGkhi6Gn7952-4/edit?usp=sharing
> 
> 
> Diffs
> -
> 
>   include/mesos/containerizer/containerizer.proto 
> f16ccc89f83da28c413ccfa0687a06b7515a605c 
>   include/mesos/mesos.proto 4a16be1f570769f3ce42a50a9da9f4fb1c227999 
>   include/mesos/slave/isolator.proto 9d38a25470a79af1eda9122c037c82b8cbbad6ed 
>   src/common/protobuf_utils.hpp 8793851fb927ab1326da6b6a424b3c6a75eb5001 
>   src/common/protobuf_utils.cpp c1e8e011e6d0b8e1cd8a836e3168685ec401b21b 
>   src/slave/containerizer/docker.cpp 6c975f904178e01797b67628a2d471ec7b3b1fbf 
>   src/slave/containerizer/external_containerizer.cpp 
> 211649201777f0d2ce802a865090129eacdd53be 
>   src/slave/containerizer/isolators/cgroups/mem.cpp 
> 89c86beb9227eb8a6e70a413e7b3934add652981 
>   src/slave/containerizer/isolators/posix/disk.cpp 
> c324c79f8d598095d07fbcb26e806a0978c2a520 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 4c1419290645ad4c44360a81618a6cea7ad190df 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b904b2d88e9b62fa4ba312c4569a4d89b0dc6052 
>   src/slave/slave.hpp 18be4f8188ad34ef4d0aa4b5eba241053d071476 
>   src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c 
>   src/tests/containerizer.cpp 1f743155526192569dd61a47ab67d8e58aab205a 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 8771ef661039310d79845513ea4602e15b2ad13d 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 5bc7d408bda0c249e1b66747d8bd87e688362e6c 
>   src/tests/slave_recovery_tests.cpp fd285fbba0ec08fe2afbdd8da0b0b451126da0eb 
>   src/tests/slave_tests.cpp dccdbb0ec5352df5da63d5ef7261bfc7fd599acb 
> 
> Diff: https://reviews.apache.org/r/38746/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> tested with Docker as well.
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 39093: CMake:[3/3] Add `make check` target.

[Joseph Wu]

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



CMakeLists.txt (lines 96 - 99)


Still only runs the libprocess tests.

This runs both tests:
```
add_custom_target(
  check ${STOUT_TESTS_TARGET}
  COMMAND ${PROCESS_TESTS_TARGET}
  DEPENDS ${PROCESS_TESTS_TARGET} ${STOUT_TESTS_TARGET}
  )
```

So for future tests, you'll need to insert another `COMMAND 
${..._TESTS_TARGET}`.


- Joseph Wu


On Oct. 9, 2015, 9:27 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39093/
> ---
> 
> (Updated Oct. 9, 2015, 9:27 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Bugs: MESOS-3597
> https://issues.apache.org/jira/browse/MESOS-3597
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake:[3/3] Add `make check` target.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt a5a66c13496a95bb7095504a4645909d887fb49f 
> 
> Diff: https://reviews.apache.org/r/39093/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

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

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39211, 39018]

All tests passed.

- Mesos ReviewBot


On Oct. 12, 2015, 6:38 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> ---
> 
> (Updated Oct. 12, 2015, 6:38 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, with one notable 
> failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue 
> tracked here: https://issues.apache.org/jira/browse/MESOS-3698
> 
> 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 38747: Adding digest utilities

[Jiang Yan Xu]

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


Incomplete review on v13. Continuing on the latest version.


3rdparty/libprocess/include/process/digest.hpp (line 40)


`/**` for doxygen comments.



3rdparty/libprocess/include/process/digest.hpp (line 59)


Relying on templates and traits or not, these should not be exposed in the 
same place where the "public" methods are seen. Put then in a "internal" 
namespace is a conventional practice.



3rdparty/libprocess/include/process/digest.hpp (line 61)


Consider using `boost::shared_array data` (see io.cpp).



3rdparty/libprocess/include/process/digest.hpp (line 66)


No need to start a new line for a single argument.



3rdparty/libprocess/include/process/digest.hpp (line 67)


With `then()` you can get `size_t` result directly so no need for taking a 
future here.



3rdparty/libprocess/include/process/digest.hpp (lines 73 - 75)


What does this mean? EOF? Add a comment?

Also, we don't need to special case this right? It will terminate at the 
next iteration right?



3rdparty/libprocess/include/process/digest.hpp (line 299)


Would it be safer to just have the caller select from the `Digest` enum?


- Jiang Yan Xu


On Oct. 7, 2015, 7:37 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Oct. 7, 2015, 7:37 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256, SHA512 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

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

[Greg Mann]

> On Oct. 12, 2015, 7:48 a.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, line 312
> > 
> >
> > Can you please make those comments using 
> > https://github.com/apache/mesos/blob/master/docs/doxygen-style-guide.md ?

Good call! Done.


- Greg


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


On Oct. 12, 2015, 6:24 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> ---
> 
> (Updated Oct. 12, 2015, 6:24 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, with one notable 
> failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue 
> tracked here: https://issues.apache.org/jira/browse/MESOS-3698
> 
> 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 38746: Added TaskStatus::Reason to containerizer Termination message.

[Jie Yu]

> On Oct. 9, 2015, 10:54 p.m., Ben Mahler wrote:
> > include/mesos/containerizer/containerizer.proto, lines 95-99
> > 
> >
> > Why do you say "executor" here but termination is about "container" 
> > above?
> > 
> > Also, can you:
> > 
> > s/task_state/state/
> > s/task_status_reasons/reasons/
> > 
> > s/'reason'/'reasons'/
> > s/of a status update for those pending/unterminated tasks/of status 
> > updates for non-terminal tasks/

Done.


> On Oct. 9, 2015, 10:54 p.m., Ben Mahler wrote:
> > include/mesos/mesos.proto, line 1100
> > 
> >
> > 0 should have been UNKNOWN, ugh :(

Added a TODO to remove it. This enum is not used anymore.


> On Oct. 9, 2015, 10:54 p.m., Ben Mahler wrote:
> > include/mesos/mesos.proto, lines 1121-1124
> > 
> >
> > Can you keep these alphabetical?
> > 
> > It's a bit unfortunate that we don't have a common 
> > "REASON_CONTAINER_LIMITATION" prefix for the limitations. Lesson learned 
> > for the next time we decide to name enums to use use prefixing to group 
> > related enums...
> > 
> > Do you want to add a generic limitation? I believe it's ok to rename 
> > these from a conversation with vinod, given they were intended for metrics 
> > rather than control flow in schedulers:
> > 
> > REASON_CONTAINER_LIMITATION
> > REASON_CONTAINER_LIMITATION_DISK
> > REASON_CONTAINER_LIMITATION_MEMORY
> > 
> > Note the presence of a general limitation that custom isolators can use.

Done.


> On Oct. 9, 2015, 10:54 p.m., Ben Mahler wrote:
> > include/mesos/slave/isolator.proto, line 42
> > 
> >
> > How about: s/for those tasks that are in non-terminal status when the 
> > container is terminated/for any remaining non-terminal tasks/ ?

Done.


> On Oct. 9, 2015, 10:54 p.m., Ben Mahler wrote:
> > include/mesos/slave/isolator.proto, line 44
> > 
> >
> > Why not s/task_status_reason/reason/ ?

Done.


> On Oct. 9, 2015, 10:54 p.m., Ben Mahler wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 1244-1246
> > 
> >
> > Can you use the arrow operator here?
> > 
> > status->isSome
> > status->get

Yes. Thanks!


> On Oct. 9, 2015, 10:54 p.m., Ben Mahler wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1252
> > 
> >
> > ! empty() ?

Done.


> On Oct. 9, 2015, 10:54 p.m., Ben Mahler wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1269
> > 
> >
> > Why did you need the trim here?

Removed.


> On Oct. 9, 2015, 10:54 p.m., Ben Mahler wrote:
> > src/slave/slave.hpp, lines 306-307
> > 
> >
> > Could you wrap on the next line, as is done with getExecutorInfo?

Done.


> On Oct. 9, 2015, 10:54 p.m., Ben Mahler wrote:
> > src/slave/slave.hpp, lines 629-636
> > 
> >
> > Hm.. I think people mind get a bit confused when they encounter this, 
> > how about we add a comment to guide them and rename it like the following:
> > 
> > ```
> > // When the agent initiates a destroyal of the container,
> > // we expect a termination to occur. The 'pendingTermation'
> > // indicates why the agent initiated the destruction and
> > // will influence the information sent in the status updates
> > // for any remaining non-terminal tasks.
> > Option pendingTermination;
> > ```

Done.


> On Oct. 9, 2015, 10:54 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 4725
> > 
> >
> > How about:
> > 
> > ```
> > (!termination->reasons().empty)
> > ```
> > 
> > instead of the size check

Looks like the protobuf we bundle does not have this method defined. It's 
introduced in later versions.


- Jie


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


On Oct. 9, 2015, 1:28 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38746/
> 

Re: Review Request 37024: Exposes mesos version information in components.

[Benjamin Mahler]

Apologies, I've committed the fix for cmake. Looking forward to getting it
set up in jenkins to catch these!

On Sun, Oct 11, 2015 at 9:53 AM, haosdent huang  wrote:

> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37024/
>
> On October 10th, 2015, 10:17 a.m. UTC, *Alex Clemmer* wrote:
>
> src/Makefile.am
>  
> (Diff
> revision 11)
>
> 554
>
>   version/version.cpp \
>
> Hmm, I know this is submitted, but can we go back and add this also to the 
> CMakeLists.txt in src/?
>
> Sorry, my fault to forgot update CMake. I open 
> https://reviews.apache.org/r/39212/, could you help review it?
>
>
> - haosdent
>
> On October 8th, 2015, 7:01 a.m. UTC, haosdent huang wrote:
> Review request for mesos and Ben Mahler.
> By haosdent huang.
>
> *Updated Oct. 8, 2015, 7:01 a.m.*
> *Bugs: * MESOS-1841 
> *Repository: * mesos
> Description
>
> Add an endpoint exposes Apache Mesos build informations and version 
> information.
>
> Testing
>
> Manual test result:
>
> $ curl http://localhost:5050/version 2>/dev/null|jq .
> {
>   "version": "0.26.0",
>   "git_sha": "4566f05e1c4c82d4d5e1c6ac563ea0fb362324e3",
>   "git_branch": "refs/heads/MESOS-1841",
>   "build_user": "haosdent",
>   "build_time": 1444185166,
>   "build_date": "2015-10-07 10:32:46"
> }
>
> Diffs
>
>- src/Makefile.am (e69892736b0edc8c264eaccd52a04d44d01f53ba)
>- src/exec/exec.cpp (7b51baaa8c08d248918974a3a22b6217e388bcb1)
>- src/local/main.cpp (18b2f0187637cd425d55c220f73faac5a1218f0f)
>- src/master/main.cpp (bafc605d6c20bd264b932e44ee80373a3f692734)
>- src/sched/sched.cpp (571e00d303009a940f17c8ed4582749a718e846d)
>- src/slave/main.cpp (854ade491c2bad0bb041cd94968215f9fd8fa4ae)
>- src/version/version.hpp (PRE-CREATION)
>- src/version/version.cpp (PRE-CREATION)
>
> View Diff 
>

Re: Review Request 38747: Adding digest utilities

[Jiang Yan Xu]

> On Oct. 1, 2015, 11:23 a.m., Jiang Yan Xu wrote:
> > Sorry I haven't chimed in earlier. I made one comment earlier with a 
> > reference to a pending review  but 
> > didn't look at the review closely. I also have a ticket 
> > https://issues.apache.org/jira/browse/MESOS-3191 create before.
> > 
> > I have a few questions, just to help me understand the context.
> > 
> > 1) Since the motivation for this util is for computing and verifying docker 
> > image hash, how will we use it? Given that this implementation requires 
> > USE_SSL_SOCKET, do we want to tie the docker provisioner to the 
> > --enable-ssl flag? Or should we create another flag or make `libssl-dev` or 
> > `openssl-devel` a dependency by default? What is the plan?
> > 
> > 2) What's the advantage of using openssl APIs directly? This implementation 
> > is obviously more complex than the alternative 
> > , which simply calls out to a few 
> > widely distributed system binaries on our supported platforms. (sha256sum 
> > sha512sum are part of GNU coreutils while shasum is on every mac). The 
> > linked review needs to address some comments but it's not far from ready 
> > for shipit (it's not a priority for us right now but you can take it if you 
> > like).
> > 
> > Thanks!
> 
> Jojy Varghese wrote:
> Thanks for feedback. To answer your questions:
> 
> - We currently depend on SSL for docker remote registry client as thats 
> the authentication type we support. The code exposes simple APIs that can be 
> called to get digest of a string, file etc. The test file serves as examples 
> of usage.
> - The advantage is that we can use the API without spawning a process. 
> Else you can image the number of spawns for a docker image with many layers. 
> In short efficiency. Another reason is that the review you pointed me to also 
> depends on those utils to be available. So we still need a fallback when 
> those utils are not available.
> - The code is open to adding more fallback implementations that can 
> incorporate the method in https://reviews.apache.org/r/34138/. So we can 
> still add those fallback. I would see this implementation as a superset and 
> not a replacement for the code presented in 
> https://reviews.apache.org/r/34138/.
> 
> -jojy

Sorry for the delay due to my travel. I realize much work has already been done 
in this and we should probably still push this forward so I'll comment on the 
code separately but the following is still high-level:

I guess I am still not fully sold on the necessity of this when we already do 
the rest of the image provisioning via subprocess commands. (i.e., `cp`, 
`tar`). Hashing using these commands look natual to me, especially because it 
doesn't interact with the rest of the codebase in anyway. It maybe something 
easier to do for the task at hand. Some of the thoughts below are applicable if 
we adopt the native implementation as well.

## SSL
Ok I see that SSL is required for for docker (or more precicsely docker when 
pulling from the registry). But how are we enabling this dependency? IIUC 
currently `USE_SSL_SOCKET` and `--enable-ssl` are tied to libevent which has 
nothing to do with SHA. I can understand that in order to use docker registry I 
have to turn on libevent + SSL but if I just want to use the hashing utility I 
have think I should be forced to switch from libev to libevent. Maybe we should 
come up with another flag and preprocessor macro for this (use openssl 
headers). FWIW I think openssl may have expected common usage in the future to 
be a hard/unconditional dependency. Which is easier?

I am all for exposing simple and generic APIs for this utility but I think we 
are discussing the implementation details here.

## Overhead of spawning a process
We already call `tar` and perhaps `cp` in subprocesses for each layer so this 
is definitely not a bottleneck. :)

## Dependency on the availability of the binary
See my comments above. I think sha256sum and sha512sum on Linux and shasum on 
OSX are widely distributed (more common than the openssl headers). They appear 
to be as common as `tar` and `mount`.

## Subprocess as a fallback.
Sure they can coexist. I am merely talking about complexity and priority. If 
openssl if a hard dependency then the subprocess implementation probably 
doesn't have to exist. On the other hand, this native implementation does data 
reading and processing in a serial manner, I don't know if these binaries have 
any optimization. Note that for large binaries we are using SHAing takes close 
to a minute so any optimization would be great to have.


- Jiang Yan


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


On Oct. 7, 2015, 7:37 p.m., Jojy Varghese wrote:
> 
> 

Re: Review Request 39201: Included `stout/check.hpp` in `future.hpp`.

[Joseph Wu]

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

Ship it!


Ship It!

- Joseph Wu


On Oct. 9, 2015, 9:31 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39201/
> ---
> 
> (Updated Oct. 9, 2015, 9:31 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Windows integration work will require us to support `stout/net.hpp`,
> but this file includes `stout/os.hpp`, which would be onerous to port.
> 
> To avoid having to port that entire file, we simply roped in what we
> needed from `stout/net.hpp`. A consequence of unincluding os.hpp is that
> `process/future.hpp` now requires a direct inclusion of
> `stout/check.hpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/future.hpp 
> 9006b8a83d03eab6e67de12a954110029b7d150e 
> 
> Diff: https://reviews.apache.org/r/39201/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 37024: Exposes mesos version information in components.

[Alex Clemmer]

Oh man, me too. :)

On Mon, Oct 12, 2015 at 11:50 AM, Benjamin Mahler  wrote:

> Apologies, I've committed the fix for cmake. Looking forward to getting it
> set up in jenkins to catch these!
>
> On Sun, Oct 11, 2015 at 9:53 AM, haosdent huang 
> wrote:
>
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/37024/
>>
>> On October 10th, 2015, 10:17 a.m. UTC, *Alex Clemmer* wrote:
>>
>> src/Makefile.am
>> 
>>  (Diff
>> revision 11)
>>
>> 554
>>
>>  version/version.cpp \
>>
>> Hmm, I know this is submitted, but can we go back and add this also to the 
>> CMakeLists.txt in src/?
>>
>> Sorry, my fault to forgot update CMake. I open 
>> https://reviews.apache.org/r/39212/, could you help review it?
>>
>>
>> - haosdent
>>
>> On October 8th, 2015, 7:01 a.m. UTC, haosdent huang wrote:
>> Review request for mesos and Ben Mahler.
>> By haosdent huang.
>>
>> *Updated Oct. 8, 2015, 7:01 a.m.*
>> *Bugs: * MESOS-1841 
>> *Repository: * mesos
>> Description
>>
>> Add an endpoint exposes Apache Mesos build informations and version 
>> information.
>>
>> Testing
>>
>> Manual test result:
>>
>> $ curl http://localhost:5050/version 2>/dev/null|jq .
>> {
>>   "version": "0.26.0",
>>   "git_sha": "4566f05e1c4c82d4d5e1c6ac563ea0fb362324e3",
>>   "git_branch": "refs/heads/MESOS-1841",
>>   "build_user": "haosdent",
>>   "build_time": 1444185166,
>>   "build_date": "2015-10-07 10:32:46"
>> }
>>
>> Diffs
>>
>>- src/Makefile.am (e69892736b0edc8c264eaccd52a04d44d01f53ba)
>>- src/exec/exec.cpp (7b51baaa8c08d248918974a3a22b6217e388bcb1)
>>- src/local/main.cpp (18b2f0187637cd425d55c220f73faac5a1218f0f)
>>- src/master/main.cpp (bafc605d6c20bd264b932e44ee80373a3f692734)
>>- src/sched/sched.cpp (571e00d303009a940f17c8ed4582749a718e846d)
>>- src/slave/main.cpp (854ade491c2bad0bb041cd94968215f9fd8fa4ae)
>>- src/version/version.hpp (PRE-CREATION)
>>- src/version/version.cpp (PRE-CREATION)
>>
>> View Diff 
>>
>
>


-- 
Alex

Theory is the first term in the Taylor series of practice. -- Thomas M
Cover (1992)

Re: Review Request 39097: CMake:[2/2] remove `__WINDOWS__` flag definition from Stout config.

[Joseph Wu]

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

Ship it!


Ship It!

- Joseph Wu


On Oct. 9, 2015, 9:28 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39097/
> ---
> 
> (Updated Oct. 9, 2015, 9:28 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake:[2/2] remove `__WINDOWS__` flag definition from Stout config.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/cmake/StoutTestsConfigure.cmake 
> c4d1b7b3f896f8037d922fafcdbdd0e31a70e140 
> 
> Diff: https://reviews.apache.org/r/39097/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

> On Oct. 1, 2015, 6:23 p.m., Jiang Yan Xu wrote:
> > Sorry I haven't chimed in earlier. I made one comment earlier with a 
> > reference to a pending review  but 
> > didn't look at the review closely. I also have a ticket 
> > https://issues.apache.org/jira/browse/MESOS-3191 create before.
> > 
> > I have a few questions, just to help me understand the context.
> > 
> > 1) Since the motivation for this util is for computing and verifying docker 
> > image hash, how will we use it? Given that this implementation requires 
> > USE_SSL_SOCKET, do we want to tie the docker provisioner to the 
> > --enable-ssl flag? Or should we create another flag or make `libssl-dev` or 
> > `openssl-devel` a dependency by default? What is the plan?
> > 
> > 2) What's the advantage of using openssl APIs directly? This implementation 
> > is obviously more complex than the alternative 
> > , which simply calls out to a few 
> > widely distributed system binaries on our supported platforms. (sha256sum 
> > sha512sum are part of GNU coreutils while shasum is on every mac). The 
> > linked review needs to address some comments but it's not far from ready 
> > for shipit (it's not a priority for us right now but you can take it if you 
> > like).
> > 
> > Thanks!
> 
> Jojy Varghese wrote:
> Thanks for feedback. To answer your questions:
> 
> - We currently depend on SSL for docker remote registry client as thats 
> the authentication type we support. The code exposes simple APIs that can be 
> called to get digest of a string, file etc. The test file serves as examples 
> of usage.
> - The advantage is that we can use the API without spawning a process. 
> Else you can image the number of spawns for a docker image with many layers. 
> In short efficiency. Another reason is that the review you pointed me to also 
> depends on those utils to be available. So we still need a fallback when 
> those utils are not available.
> - The code is open to adding more fallback implementations that can 
> incorporate the method in https://reviews.apache.org/r/34138/. So we can 
> still add those fallback. I would see this implementation as a superset and 
> not a replacement for the code presented in 
> https://reviews.apache.org/r/34138/.
> 
> -jojy
> 
> Jiang Yan Xu wrote:
> Sorry for the delay due to my travel. I realize much work has already 
> been done in this and we should probably still push this forward so I'll 
> comment on the code separately but the following is still high-level:
> 
> I guess I am still not fully sold on the necessity of this when we 
> already do the rest of the image provisioning via subprocess commands. (i.e., 
> `cp`, `tar`). Hashing using these commands look natual to me, especially 
> because it doesn't interact with the rest of the codebase in anyway. It maybe 
> something easier to do for the task at hand. Some of the thoughts below are 
> applicable if we adopt the native implementation as well.
> 
> ## SSL
> Ok I see that SSL is required for for docker (or more precicsely docker 
> when pulling from the registry). But how are we enabling this dependency? 
> IIUC currently `USE_SSL_SOCKET` and `--enable-ssl` are tied to libevent which 
> has nothing to do with SHA. I can understand that in order to use docker 
> registry I have to turn on libevent + SSL but if I just want to use the 
> hashing utility I have think I should be forced to switch from libev to 
> libevent. Maybe we should come up with another flag and preprocessor macro 
> for this (use openssl headers). FWIW I think openssl may have expected common 
> usage in the future to be a hard/unconditional dependency. Which is easier?
> 
> I am all for exposing simple and generic APIs for this utility but I 
> think we are discussing the implementation details here.
> 
> ## Overhead of spawning a process
> We already call `tar` and perhaps `cp` in subprocesses for each layer so 
> this is definitely not a bottleneck. :)
> 
> ## Dependency on the availability of the binary
> See my comments above. I think sha256sum and sha512sum on Linux and 
> shasum on OSX are widely distributed (more common than the openssl headers). 
> They appear to be as common as `tar` and `mount`.
> 
> ## Subprocess as a fallback.
> Sure they can coexist. I am merely talking about complexity and priority. 
> If openssl if a hard dependency then the subprocess implementation probably 
> doesn't have to exist. On the other hand, this native implementation does 
> data reading and processing in a serial manner, I don't know if these 
> binaries have any optimization. Note that for large binaries we are using 
> SHAing takes close to a minute so any optimization would be great to have.

Thanks for the comments Yan, inspite of your busy schedule.

To summarize the "YEAs" and "NAYs" for this solution:

YEA:
- 

Re: Review Request 39202: CMake: Moved libevent, gmock, http-parser to CMake on Windows.

[Joseph Wu]

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



3rdparty/libprocess/3rdparty/CMakeLists.txt (line 124)


Templatize and rename upon copy.  (See comment below).



3rdparty/libprocess/3rdparty/CMakeLists.txt 


Where are you moving this to?
(Or why do we *not* need to build Gmock now?)



3rdparty/libprocess/3rdparty/http-parser/CMakeLists.txt (line 17)


Since you copy this file into its final destination, could you make this a 
template?

i.e. Rename to `CMakeLists.txt.template`.


- Joseph Wu


On Oct. 9, 2015, 9:36 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39202/
> ---
> 
> (Updated Oct. 9, 2015, 9:36 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake: Moved libevent, gmock, http-parser to CMake on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/CMakeLists.txt 
> f0de224b0d7924344fb1945b387b728a7241df87 
>   3rdparty/libprocess/3rdparty/http-parser/CMakeLists.txt PRE-CREATION 
>   3rdparty/libprocess/cmake/Process3rdpartyConfigure.cmake 
> ee1c74d31e28136bf289f4100d79a8ce568cd3af 
>   3rdparty/libprocess/cmake/ProcessConfigure.cmake 
> 60108ff598fb075584196aa3d8e8e66e726c9f2a 
>   3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 
> ea6db09e1a1aa01450aee93814e07f09feae7ac9 
> 
> Diff: https://reviews.apache.org/r/39202/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 39203: CMake: fixed typo in agent include directory configuration.

[Joseph Wu]

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

Ship it!


Ship It!

- Joseph Wu


On Oct. 10, 2015, 2:58 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39203/
> ---
> 
> (Updated Oct. 10, 2015, 2:58 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake: fixed typo in agent include directory configuration.
> 
> 
> Diffs
> -
> 
>   src/slave/cmake/SlaveConfigure.cmake 
> 5817547526ac6851e51baa05f1a72e225fc65791 
> 
> Diff: https://reviews.apache.org/r/39203/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 38746: Added TaskStatus::Reason to containerizer Termination message.

[Jie Yu]

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

(Updated Oct. 12, 2015, 7:33 p.m.)


Review request for mesos, Ben Mahler, Timothy Chen, and Vinod Kone.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Added TaskStatus::Reason to containerizer Termination message.

The following doc summarized the problem and proposed a solution:
https://docs.google.com/document/d/1klGDAu5yBVf-CGWLqvELLIfxLfRaisGkhi6Gn7952-4/edit?usp=sharing


Diffs (updated)
-

  include/mesos/containerizer/containerizer.proto 
f16ccc89f83da28c413ccfa0687a06b7515a605c 
  include/mesos/mesos.proto 4a16be1f570769f3ce42a50a9da9f4fb1c227999 
  include/mesos/slave/isolator.proto 9d38a25470a79af1eda9122c037c82b8cbbad6ed 
  src/common/protobuf_utils.hpp 8793851fb927ab1326da6b6a424b3c6a75eb5001 
  src/common/protobuf_utils.cpp c1e8e011e6d0b8e1cd8a836e3168685ec401b21b 
  src/slave/containerizer/docker.cpp 6c975f904178e01797b67628a2d471ec7b3b1fbf 
  src/slave/containerizer/external_containerizer.cpp 
211649201777f0d2ce802a865090129eacdd53be 
  src/slave/containerizer/isolators/cgroups/mem.cpp 
89c86beb9227eb8a6e70a413e7b3934add652981 
  src/slave/containerizer/isolators/posix/disk.cpp 
c324c79f8d598095d07fbcb26e806a0978c2a520 
  src/slave/containerizer/mesos/containerizer.hpp 
4c1419290645ad4c44360a81618a6cea7ad190df 
  src/slave/containerizer/mesos/containerizer.cpp 
b904b2d88e9b62fa4ba312c4569a4d89b0dc6052 
  src/slave/slave.hpp 18be4f8188ad34ef4d0aa4b5eba241053d071476 
  src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c 
  src/tests/containerizer.cpp 1f743155526192569dd61a47ab67d8e58aab205a 
  src/tests/containerizer/docker_containerizer_tests.cpp 
8771ef661039310d79845513ea4602e15b2ad13d 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
5bc7d408bda0c249e1b66747d8bd87e688362e6c 
  src/tests/oversubscription_tests.cpp 2cf047e6ae061c1b9eed6238e1df67c91ab3514a 
  src/tests/slave_recovery_tests.cpp fd285fbba0ec08fe2afbdd8da0b0b451126da0eb 
  src/tests/slave_tests.cpp dccdbb0ec5352df5da63d5ef7261bfc7fd599acb 

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


Testing
---

sudo make check

tested with Docker as well.


Thanks,

Jie Yu

Re: Review Request 39093: CMake:[3/3] Add `make check` target.

[Alex Clemmer]

> On Oct. 12, 2015, 7:06 p.m., Joseph Wu wrote:
> > CMakeLists.txt, lines 96-99
> > 
> >
> > Still only runs the libprocess tests.
> > 
> > This runs both tests:
> > ```
> > add_custom_target(
> >   check ${STOUT_TESTS_TARGET}
> >   COMMAND ${PROCESS_TESTS_TARGET}
> >   DEPENDS ${PROCESS_TESTS_TARGET} ${STOUT_TESTS_TARGET}
> >   )
> > ```
> > 
> > So for future tests, you'll need to insert another `COMMAND 
> > ${..._TESTS_TARGET}`.

Oh man, sorry, the process tests fail on corpnet, and in the flurry of commits 
I forgot to come back and verify this worked when I went home.


- Alex


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


On Oct. 10, 2015, 4:27 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39093/
> ---
> 
> (Updated Oct. 10, 2015, 4:27 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Bugs: MESOS-3597
> https://issues.apache.org/jira/browse/MESOS-3597
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake:[3/3] Add `make check` target.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt a5a66c13496a95bb7095504a4645909d887fb49f 
> 
> Diff: https://reviews.apache.org/r/39093/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 38746: Added TaskStatus::Reason to containerizer Termination message.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38746]

All tests passed.

- Mesos ReviewBot


On Oct. 12, 2015, 7:33 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38746/
> ---
> 
> (Updated Oct. 12, 2015, 7:33 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2035
> https://issues.apache.org/jira/browse/MESOS-2035
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added TaskStatus::Reason to containerizer Termination message.
> 
> The following doc summarized the problem and proposed a solution:
> https://docs.google.com/document/d/1klGDAu5yBVf-CGWLqvELLIfxLfRaisGkhi6Gn7952-4/edit?usp=sharing
> 
> 
> Diffs
> -
> 
>   include/mesos/containerizer/containerizer.proto 
> f16ccc89f83da28c413ccfa0687a06b7515a605c 
>   include/mesos/mesos.proto 4a16be1f570769f3ce42a50a9da9f4fb1c227999 
>   include/mesos/slave/isolator.proto 9d38a25470a79af1eda9122c037c82b8cbbad6ed 
>   src/common/protobuf_utils.hpp 8793851fb927ab1326da6b6a424b3c6a75eb5001 
>   src/common/protobuf_utils.cpp c1e8e011e6d0b8e1cd8a836e3168685ec401b21b 
>   src/slave/containerizer/docker.cpp 6c975f904178e01797b67628a2d471ec7b3b1fbf 
>   src/slave/containerizer/external_containerizer.cpp 
> 211649201777f0d2ce802a865090129eacdd53be 
>   src/slave/containerizer/isolators/cgroups/mem.cpp 
> 89c86beb9227eb8a6e70a413e7b3934add652981 
>   src/slave/containerizer/isolators/posix/disk.cpp 
> c324c79f8d598095d07fbcb26e806a0978c2a520 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 4c1419290645ad4c44360a81618a6cea7ad190df 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b904b2d88e9b62fa4ba312c4569a4d89b0dc6052 
>   src/slave/slave.hpp 18be4f8188ad34ef4d0aa4b5eba241053d071476 
>   src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c 
>   src/tests/containerizer.cpp 1f743155526192569dd61a47ab67d8e58aab205a 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 8771ef661039310d79845513ea4602e15b2ad13d 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 5bc7d408bda0c249e1b66747d8bd87e688362e6c 
>   src/tests/oversubscription_tests.cpp 
> 2cf047e6ae061c1b9eed6238e1df67c91ab3514a 
>   src/tests/slave_recovery_tests.cpp fd285fbba0ec08fe2afbdd8da0b0b451126da0eb 
>   src/tests/slave_tests.cpp dccdbb0ec5352df5da63d5ef7261bfc7fd599acb 
> 
> Diff: https://reviews.apache.org/r/38746/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> tested with Docker as well.
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 39193: Fixed leakage of fts_open.

[Chi Zhang]

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

(Updated Oct. 12, 2015, 8:19 p.m.)


Review request for mesos and Ben Mahler.


Bugs: mesos-3519
https://issues.apache.org/jira/browse/mesos-3519


Repository: mesos


Description
---

Fixed leakage of fts_open.


Diffs (updated)
-

  src/linux/cgroups.cpp 53c568bd9559bbe46adb90265445f466245a0722 

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


Testing (updated)
---

make check


Thanks,

Chi Zhang

Re: Review Request 39192: Fixed leakage of fts_open.

[Chi Zhang]

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

(Updated Oct. 12, 2015, 8:18 p.m.)


Review request for mesos and Ben Mahler.


Bugs: mesos-3519
https://issues.apache.org/jira/browse/mesos-3519


Repository: mesos


Description
---

Fixed leakage of fts_open.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
5d2f39d9a9d963225bf463572cb8fe99dd9aa6f5 

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


Testing (updated)
---

make check


Thanks,

Chi Zhang

Re: Review Request 39192: Fixed leakage of fts_open.

[Ben Mahler]

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

Ship it!


Ship It!

- Ben Mahler


On Oct. 12, 2015, 8:18 p.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39192/
> ---
> 
> (Updated Oct. 12, 2015, 8:18 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: mesos-3519
> https://issues.apache.org/jira/browse/mesos-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed leakage of fts_open.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> 5d2f39d9a9d963225bf463572cb8fe99dd9aa6f5 
> 
> Diff: https://reviews.apache.org/r/39192/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 39193: Fixed leakage of fts_open.

[Ben Mahler]

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

Ship it!



src/linux/cgroups.cpp (lines 925 - 926)


The = should go on the first line, I'll fix this for you


- Ben Mahler


On Oct. 12, 2015, 8:19 p.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39193/
> ---
> 
> (Updated Oct. 12, 2015, 8:19 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: mesos-3519
> https://issues.apache.org/jira/browse/mesos-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed leakage of fts_open.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 53c568bd9559bbe46adb90265445f466245a0722 
> 
> Diff: https://reviews.apache.org/r/39193/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 39230: Added check for SASL deprecation into configuration phase.

[James Peach]

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

Ship it!


It's marginal, but I think ```AC_LANG_PROGRAM``` is a little clearer that 
```AC_LANG_SOURCE```.

If you are going to rely on the diagnostic pragmas, an alternative is to simply 
use those to disable the warning in the SASL wrappers which is a little more 
tightly scoped.

- James Peach


On Oct. 12, 2015, 1:25 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39230/
> ---
> 
> (Updated Oct. 12, 2015, 1:25 p.m.)
> 
> 
> Review request for mesos and switched to 'mcypark'.
> 
> 
> Bugs: MESOS-3030
> https://issues.apache.org/jira/browse/MESOS-3030
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch also removes some useless CFLAGS handling on the SASL CRAM-MD5 
> checks of the configuration.
> 
> 
> Diffs
> -
> 
>   configure.ac 66f9b32773861d2921d99189e0fbcaea48c456a9 
> 
> Diff: https://reviews.apache.org/r/39230/diff/
> 
> 
> Testing
> ---
> 
> ./configure && make check 
> OSX, clang3.7 + gcc4.9
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 39093: CMake:[3/3] Add `make check` target.

[Alex Clemmer]

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

(Updated Oct. 12, 2015, 7:41 p.m.)


Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
Wu.


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


Repository: mesos


Description
---

CMake:[3/3] Add `make check` target.


Diffs (updated)
-

  CMakeLists.txt a5a66c13496a95bb7095504a4645909d887fb49f 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 39202: CMake: Moved libevent, gmock, http-parser to CMake on Windows.

[Alex Clemmer]

> On Oct. 12, 2015, 7:26 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/CMakeLists.txt, lines 241-246
> > 
> >
> > Where are you moving this to?
> > (Or why do we *not* need to build Gmock now?)

Sorry, I didn't add a commit message here in my haste.

Basically: GMock 1.7 includes a new CMake-based build system as an option, 
alongside the VS solution. So we just use that instead of the VS solution. By 
ommitting a build command here we cause CMake to use its "default" behavior, 
which is to treat the project as a CMake project. In VS, this means 
specifically that GMock becomes "just another C++ project" embedded in the 
Mesos solution file. So it's very convenient.

And, btw, the reason we're doing this is because GMock's build is broken on 
Ubuntu 15 when you use autotools. So transitioning to CMake has the practical 
advantage of causing the build to work on Joris's machine.

As an action item, I will update the commit message to explain this. :) Does 
that sound good?


- Alex


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


On Oct. 10, 2015, 4:36 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39202/
> ---
> 
> (Updated Oct. 10, 2015, 4:36 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake: Moved libevent, gmock, http-parser to CMake on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/CMakeLists.txt 
> f0de224b0d7924344fb1945b387b728a7241df87 
>   3rdparty/libprocess/3rdparty/http-parser/CMakeLists.txt PRE-CREATION 
>   3rdparty/libprocess/cmake/Process3rdpartyConfigure.cmake 
> ee1c74d31e28136bf289f4100d79a8ce568cd3af 
>   3rdparty/libprocess/cmake/ProcessConfigure.cmake 
> 60108ff598fb075584196aa3d8e8e66e726c9f2a 
>   3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 
> ea6db09e1a1aa01450aee93814e07f09feae7ac9 
> 
> Diff: https://reviews.apache.org/r/39202/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 39093: CMake:[3/3] Add `make check` target.

[Joseph Wu]

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

Ship it!


Ship It!

- Joseph Wu


On Oct. 12, 2015, 12:41 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39093/
> ---
> 
> (Updated Oct. 12, 2015, 12:41 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Bugs: MESOS-3597
> https://issues.apache.org/jira/browse/MESOS-3597
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake:[3/3] Add `make check` target.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt a5a66c13496a95bb7095504a4645909d887fb49f 
> 
> Diff: https://reviews.apache.org/r/39093/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

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 Oct. 12, 2015, 6:34 p.m.)


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


Changes
---

Removed periods from error messages.


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, with one notable failure 
(ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: 
https://issues.apache.org/jira/browse/MESOS-3698

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 Oct. 12, 2015, 6:38 p.m.)


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


Changes
---

Removed periods from v1 error messages.


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, with one notable failure 
(ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: 
https://issues.apache.org/jira/browse/MESOS-3698

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 37336: Simplified the caller interface to process::Subprocess

[Marco Massenzio]

> On Oct. 12, 2015, 4:08 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 281
> > 
> >
> > Is this API something you agreed on with your shepherd? I'm curious why 
> > we provide a blocking function rather than returning a future with the 
> > provided timeout.

uhm... `wait()` is by definition blocking :)
following our conversation, I'm going to change the name (and the signature) to 
return a Future instead


> On Oct. 12, 2015, 4:08 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 292
> > 
> >
> > If this value is not meaningful until wait is finished, why not make 
> > this a future bound by that condition rather than providing undefined 
> > behavior?

With the new signature, yes this will change too.
BTW this is not "undefined" - it is created in a way that it makes sense even 
in the case of failures.


- Marco


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


On Oct. 12, 2015, 12:29 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37336/
> ---
> 
> (Updated Oct. 12, 2015, 12:29 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-3035
> https://issues.apache.org/jira/browse/MESOS-3035
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-3035
> 
> The original API for `process::Subprocess` still left a lot of legwork
> to do for the caller; we have now added a `wait(Duration timeout)` method
> that returns a `CommandResult` (also introduced with this patch) which
> contains useful information about the command invocation; the exit code;
> stdout and, optionally, stderr too.
> 
> The `wait()` method will wait for both the process to terminate, and
> stdout/stderr to be available to read from; it also "unpacks" them into
> ready-to-use `string`s.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess.hpp 
> d2341a53aac7c779668ee80983f73158fd44bff5 
>   3rdparty/libprocess/src/subprocess.cpp 
> d6ea62ed1c914d34e0e189395831c86fff8aac22 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> ab7515325e5db0a4fd222bb982f51243d7b7e39d 
> 
> Diff: https://reviews.apache.org/r/37336/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 39037: Allow description empty in help information.

[Ben Mahler]

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



3rdparty/libprocess/src/help.cpp (lines 41 - 75)


To make things consistent, how about we take every argument as a const 
reference here and do the newline append directly to 'help', as you've done for 
description:

```
string HELP(
const string& tldr,
const Option& description,
const Option& references)
{
  // Construct the help string.
  string help =
"### TL;DR; ###\n" +
tldr;

  if (!strings::endsWith(help, "\n")) {
help += "\n";
  }

  if (description.isSome()) {
help +=
  "\n"
  "### DESCRIPTION ###\n" +
  description.get();

if (!strings::endsWith(help, "\n")) {
  help += "\n";
}
  }

  if (references.isSome()) {
help += "\n";
help += references.get();
  }

  return help;
}
```


- Ben Mahler


On Oct. 8, 2015, 6:59 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39037/
> ---
> 
> (Updated Oct. 8, 2015, 6:59 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3104
> https://issues.apache.org/jira/browse/MESOS-3104
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow description empty in help information.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/help.hpp 
> e7dc670648e8abd2fef7be79835f0b71e3e91258 
>   3rdparty/libprocess/src/help.cpp 822c0844c61c7cabfca2b0534a5fb40001bd7cc7 
> 
> Diff: https://reviews.apache.org/r/39037/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39204: Windows: Added support for `stout/os/read.hpp`.

[Joseph Wu]

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

Ship it!


Looks good.  (Checked that logical content of `read.hpp` is identical to 
`posix/read.hpp`.)


3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp (lines 136 - 138)


Nit: 2 newlines too many


- Joseph Wu


On Oct. 10, 2015, 1:12 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39204/
> ---
> 
> (Updated Oct. 10, 2015, 1:12 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Bugs: MESOS-3641
> https://issues.apache.org/jira/browse/MESOS-3641
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Added support for `stout/os/read.hpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 9e9c3119ad18f4cbc70c70095c71dc4fd19553df 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/read.hpp 
> ffacce164980157ea225a4e64e33b8bf8ec38ab7 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp 
> d5698a5b44fd6083ac3119d6825d31f46efb2f38 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/read.hpp 
> 09d63329f16f13d408742f9fc8f596d76c4d70c9 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
> 50e35f43d87c69a83a9e7d039d1881404ea8be38 
> 
> Diff: https://reviews.apache.org/r/39204/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 38919: Validation of Docker Image Manifests

[Gilbert Song]

> On Oct. 9, 2015, 3:22 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioner/docker/spec.hpp, line 38
> > 
> >
> > Nit: s/validateManifest/validate
> > 
> > We tend to use non-redundant names. This is clear from the 
> > signature/comment that this function validates a manifest.
> > 
> > `Option validate(const docker::DockerImageManifest& manifest);`

I changed it from 'validate' to 'validateManifest', because we will have other 
validations here, e.g., validateLayout(const std::string), validateImageID().


> On Oct. 9, 2015, 3:22 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioner/docker/spec.hpp, line 40
> > 
> >
> > I am a bit confused here. Doesn't the `validateManifest` function 
> > validates this 4 items ?
> > 
> > If so, can we remove this TODO ?

Thanks for pointing it out. 'Manifest' should be deleted.


> On Oct. 9, 2015, 3:22 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioner/docker/spec.cpp, line 38
> > 
> >
> > I am assuming .size() returns an `std::size_t` ?
> > 
> > Hence just checking for `==0` should suffice ?
> > 
> > Ditto for the other 2 cases

>From message.ph.h, the returned type is 'int'.


> On Oct. 9, 2015, 3:22 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioner/docker/spec.cpp, line 39
> > 
> >
> > We tend to avoid periods at the end of log/error statements. Can you 
> > remove them. Ditto for the other 3 occurences

Thank you for pointing it out.


> On Oct. 9, 2015, 3:22 p.m., Anand Mazumdar wrote:
> > src/tests/containerizer/provisioner_docker_tests.cpp, line 297
> > 
> >
> > Did you miss adding this block in the last review of this chain ?
> > 
> > Usually we tend to make each review in a chain self-complete by itself 
> > that can be ideally submitted on it's own. 
> > 
> > There might be some exceptions to this in some circumstances though 
> > when this is not possible.

It is originally there, but deleted by mistake during a git rebase. So I add it 
back.


> On Oct. 9, 2015, 3:22 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioner/docker/spec.cpp, line 59
> > 
> >
> > const string& blobSum

which will cause an error "error: binding of reference to type 
'basic_string<[...]>' to a value of type 'const basic_string<[...]>' drops 
qualifiers".


> On Oct. 9, 2015, 3:22 p.m., Anand Mazumdar wrote:
> > src/tests/containerizer/provisioner_docker_tests.cpp, line 386
> > 
> >
> > How about:
> > 
> > // Test Manifest Validation with empty repeated 'fsLayers' field.
> > 
> > Ditto for the following test-case.

Nice point.


- Gilbert


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


On Oct. 9, 2015, 4:39 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38919/
> ---
> 
> (Updated Oct. 9, 2015, 4:39 p.m.)
> 
> 
> Review request for mesos, Jojy Varghese and Timothy Chen.
> 
> 
> Bugs: MESOS-3099
> https://issues.apache.org/jira/browse/MESOS-3099
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Validation of Docker Image Manifests
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/provisioner/docker/spec.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/spec.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> d895eb9d0723e52cff8b21ef2deeaef1911d019c 
> 
> Diff: https://reviews.apache.org/r/38919/diff/
> 
> 
> Testing
> ---
> 
> make check (Ubuntu14.04 + clang++-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 38746: Added TaskStatus::Reason to containerizer Termination message.

[Ben Mahler]

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

Ship it!


Really nice to see this stuff get fixed, thanks Jie!


include/mesos/mesos.proto (lines 1100 - 1101)


Shall we say why it's "bad"? i.e. the default value when a caller doesn't 
check for presence is 0 and so ideally the 0 reason is not a valid one.



include/mesos/mesos.proto (line 1102)


Looks like tab characters were introduced here?



include/mesos/mesos.proto (lines 1105 - 1107)


Would be nice to have these renames in the upgrade notes, mentioning that 
the numbers are backwards compatible but there needs to be a compile time 
adjustment for those relying on REASON_MEMORY_LIMIT :)



src/slave/slave.cpp (lines 1673 - 1695)


It's still a little unsettling that these container update / launch failure 
cases don't properly set the executor state to Executor::TERMINATING, can you 
at least add a TODO in these to signal the reader of the code?

Curious what we can do to simplify all of these redundant container update 
failure cases, worth thinking about later..



src/slave/slave.cpp (lines 4518 - 4519)


Is this still needed? Looks like you've already done this?


- Ben Mahler


On Oct. 12, 2015, 7:33 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38746/
> ---
> 
> (Updated Oct. 12, 2015, 7:33 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2035
> https://issues.apache.org/jira/browse/MESOS-2035
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added TaskStatus::Reason to containerizer Termination message.
> 
> The following doc summarized the problem and proposed a solution:
> https://docs.google.com/document/d/1klGDAu5yBVf-CGWLqvELLIfxLfRaisGkhi6Gn7952-4/edit?usp=sharing
> 
> 
> Diffs
> -
> 
>   include/mesos/containerizer/containerizer.proto 
> f16ccc89f83da28c413ccfa0687a06b7515a605c 
>   include/mesos/mesos.proto 4a16be1f570769f3ce42a50a9da9f4fb1c227999 
>   include/mesos/slave/isolator.proto 9d38a25470a79af1eda9122c037c82b8cbbad6ed 
>   src/common/protobuf_utils.hpp 8793851fb927ab1326da6b6a424b3c6a75eb5001 
>   src/common/protobuf_utils.cpp c1e8e011e6d0b8e1cd8a836e3168685ec401b21b 
>   src/slave/containerizer/docker.cpp 6c975f904178e01797b67628a2d471ec7b3b1fbf 
>   src/slave/containerizer/external_containerizer.cpp 
> 211649201777f0d2ce802a865090129eacdd53be 
>   src/slave/containerizer/isolators/cgroups/mem.cpp 
> 89c86beb9227eb8a6e70a413e7b3934add652981 
>   src/slave/containerizer/isolators/posix/disk.cpp 
> c324c79f8d598095d07fbcb26e806a0978c2a520 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 4c1419290645ad4c44360a81618a6cea7ad190df 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b904b2d88e9b62fa4ba312c4569a4d89b0dc6052 
>   src/slave/slave.hpp 18be4f8188ad34ef4d0aa4b5eba241053d071476 
>   src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c 
>   src/tests/containerizer.cpp 1f743155526192569dd61a47ab67d8e58aab205a 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 8771ef661039310d79845513ea4602e15b2ad13d 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 5bc7d408bda0c249e1b66747d8bd87e688362e6c 
>   src/tests/oversubscription_tests.cpp 
> 2cf047e6ae061c1b9eed6238e1df67c91ab3514a 
>   src/tests/slave_recovery_tests.cpp fd285fbba0ec08fe2afbdd8da0b0b451126da0eb 
>   src/tests/slave_tests.cpp dccdbb0ec5352df5da63d5ef7261bfc7fd599acb 
> 
> Diff: https://reviews.apache.org/r/38746/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> tested with Docker as well.
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 39207: Windows: Move `write` to its own file, `stout/os/write.hpp`.

[Joseph Wu]

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

Ship it!


LGTM.  Verified that none of the `write` logic changed in the move.

- Joseph Wu


On Oct. 10, 2015, 12:47 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39207/
> ---
> 
> (Updated Oct. 10, 2015, 12:47 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Bugs: MESOS-3632
> https://issues.apache.org/jira/browse/MESOS-3632
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Move `write` to its own file, `stout/os/write.hpp`.
> 
> (This part isn't going in the commit message, but just FYI, we're doing this 
> (1) because we need it to support `slave/state.hpp`, (2) because we don't 
> want to port all of `os.hpp` to get this one function, and (3) because 
> `stout/os/read.hpp` already exists, so it makes sense to have one for `write` 
> as well.)
> 
> (Also, please do not close MESOS-3632, this is just a partial solution.)
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 9e9c3119ad18f4cbc70c70095c71dc4fd19553df 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 5141c1369af60afd6cd5c70c6295d575ea960a83 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/write.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> 5d2f39d9a9d963225bf463572cb8fe99dd9aa6f5 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> 2b0966889af73238a08e29f1136d0ce286a0ffda 
> 
> Diff: https://reviews.apache.org/r/39207/diff/
> 
> 
> Testing
> ---
> 
> Ran `make check` on Windows 10, OS X 10.10, Ubuntu 15. On Ubuntu 15, we also 
> checked that the autotools solution builds and the tests run as well.
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 39208: Windows: Add windows support to `stout/protobuf.hpp`.

[Joseph Wu]

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

Ship it!



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (lines 41 - 42)


Do you want to `#include ` too?


- Joseph Wu


On Oct. 10, 2015, 4:46 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39208/
> ---
> 
> (Updated Oct. 10, 2015, 4:46 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Bugs: MESOS-3634
> https://issues.apache.org/jira/browse/MESOS-3634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Add windows support to `stout/protobuf.hpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
> 2285ce9eaba668d5215c108849055fe92163da4d 
> 
> Diff: https://reviews.apache.org/r/39208/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

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

(Updated Oct. 12, 2015, 9:14 p.m.)


Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.


Changes
---

review comments adderssed.


Repository: mesos


Description
---

Added SHA256, SHA512 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am eb7393904988afc0bc5e9f7dadd545e0d6c04e5f 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

> On Oct. 12, 2015, 7:18 p.m., Jiang Yan Xu wrote:
> > 3rdparty/libprocess/include/process/digest.hpp, line 61
> > 
> >
> > Consider using `boost::shared_array data` (see io.cpp).

Since c++11 standard way is to use shared_ptr with deleter, shared_ptr is used. 
I have changed the deleter to use the std default deleter. I am not passing 
smart pointer to the called function as its not required.


> On Oct. 12, 2015, 7:18 p.m., Jiang Yan Xu wrote:
> > 3rdparty/libprocess/include/process/digest.hpp, lines 73-75
> > 
> >
> > What does this mean? EOF? Add a comment?
> > 
> > Also, we don't need to special case this right? It will terminate at 
> > the next iteration right?

You are right. The early return is just an optimization.


> On Oct. 12, 2015, 7:18 p.m., Jiang Yan Xu wrote:
> > 3rdparty/libprocess/include/process/digest.hpp, line 299
> > 
> >
> > Would it be safer to just have the caller select from the `Digest` enum?

This API was chosen to enable use cases like Docker which gets the SHA method 
as a string. This reduces the logic at client/caller site.


- Jojy


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


On Oct. 12, 2015, 9:14 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Oct. 12, 2015, 9:14 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256, SHA512 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am eb7393904988afc0bc5e9f7dadd545e0d6c04e5f 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38570: Change documentation image links to absolute paths.

[Niklas Nielsen]

> On Oct. 6, 2015, 2:29 p.m., Ben Mahler wrote:
> > docs/external-containerizer.md, line 92
> > 
> >
> > How will this work when we add multiple versions of documents to the 
> > website? Now this hardcodes "latest" as the version?
> 
> Joseph Wu wrote:
> I'm not sure.
> 
> From what I've seen of the website generation code, we currently wipe out 
> older versions of the docs during generation.  We'd first have to change the 
> site's Rakefile (and probably the release process) to keep older versions.
> 
> After that, we'd still have to deal with the problem of relative URLs and 
> trailing slashes:
> 
> http://stackoverflow.com/questions/5457885/relative-urls-and-trailing-slashes
> 
> Perhaps we could change the markdown parser to replace image links with a 
> versioned absolute path.  (I'm not sure what that would entail.)
> 
> OR
> 
> We could change all the image links to relative links (i.e. 
> `images/ec_launch_seqdiag.png`) and remove trailing slashes from all 
> documentation links.  (Maybe add redirects from trailing slash pages to 
> non-trailing slash pages.)
> 
> 
> What do you think?

BenM: ping ^^


- Niklas


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


On Oct. 6, 2015, 9:45 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38570/
> ---
> 
> (Updated Oct. 6, 2015, 9:45 a.m.)
> 
> 
> Review request for mesos, Adam B, Dave Lester, Artem Harutyunyan, Kapil Arya, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-3183
> https://issues.apache.org/jira/browse/MESOS-3183
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also removes extraneous `?raw=true` from the links.
> 
> 
> Diffs
> -
> 
>   docs/external-containerizer.md 96932189b74dce1ae28e0bd73d5543d1afaffb0b 
>   docs/fetcher-cache-internals.md e8a68d1230420d1afca61f92c1ab6be12a70dbf2 
>   docs/maintenance.md a5831ffa092a9ea6decbe2e640bae637c759a308 
>   docs/networking-for-mesos-managed-containers.md 
> 33568a8233523ff3805f962371c94346e608208b 
>   docs/oversubscription.md 5a31b1ff7f003307817732a71f3b0a7c7d60cd24 
> 
> Diff: https://reviews.apache.org/r/38570/diff/
> 
> 
> Testing
> ---
> 
> Patched `Rakefile` to include images in the website (See JIRA for the patch).
> Then rendered with: https://github.com/mesosphere/mesos-website-container
> 
> Confirmed that images show up on the modified docs.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

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

[Gilbert Song]

> On Oct. 9, 2015, 1:12 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioner/docker/spec.hpp, line 18
> > 
> >
> > We generally prefer header files that are `complete` i.e. they compile 
> > on their own. Can you ensure that we include `stout/json.hpp` here since it 
> > has `JSON::Object`. Also , do a sweep of the other includes.

Thank you for reminding. For your recommendation of sweeping, the others are 
included and used in the next patch, which is depending this one. So I leave 
them here.


> On Oct. 9, 2015, 1:12 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioner/docker/spec.hpp, line 22
> > 
> >
> > Do we need this ? If not, remove this include.

Ditto. It is used in the next patch.


> On Oct. 9, 2015, 1:12 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioner/docker/spec.cpp, line 20
> > 
> >
> > Do you need this ? If not remove it

Ditto. (referring to https://reviews.apache.org/r/38919/diff/4#index_header)


> On Oct. 9, 2015, 1:12 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioner/docker/spec.cpp, line 24
> > 
> >
> > Do you need this ? If not remove it

ditto.


> On Oct. 9, 2015, 1:12 p.m., Anand Mazumdar wrote:
> > src/tests/containerizer/provisioner_docker_tests.cpp, line 331
> > 
> >
> > Nit: Does it make sense to change the ASSERT_EQ to EXPECT_EQ ? 
> > 
> > Might help the user to see a list of all the things that failed instead 
> > of just aborting at the first failure.

This is a good point.


- Gilbert


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


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

Re: Review Request 38578: Update upgrade.md for SUPPRESS related upgrade

[Guangya Liu]

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

(Updated Oct. 13, 2015, 1:08 a.m.)


Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Update upgrade.md for SUPPRESS related upgrade


Diffs (updated)
-

  docs/upgrades.md c2e39b7ddb12a00cd7ce5ca78bb34af9ba67b3c0 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 39184: RegistryClient refactor: reordered ctor parameters

[Timothy Chen]

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

Ship it!


Ship It!

- Timothy Chen


On Oct. 9, 2015, 11:51 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39184/
> ---
> 
> (Updated Oct. 9, 2015, 11:51 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> RegistryClient refactor: reordered ctor parameters
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/provisioner/docker/registry_client.hpp 
> fdb68b675582f603ffb3e96f31c1c9405e238567 
>   src/slave/containerizer/provisioner/docker/registry_client.cpp 
> 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 
> 
> Diff: https://reviews.apache.org/r/39184/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

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

[Ben Mahler]

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

Ship it!


Thanks for your patience Tim! Looks good, just some minor comments (let's 
remove isStatus now that we don't have to do the casting stuff).


3rdparty/libprocess/include/process/http.hpp (lines 416 - 419)


This is no longer necessary, yes? Callers can just do an equality check 
against 'code'.



3rdparty/libprocess/src/decoder.hpp (lines 444 - 447)


Looks like this check should take place before setting the code and status, 
since it's validating the code.



3rdparty/libprocess/src/decoder.hpp (lines 656 - 659)


Ditto here, can you move this above the setting of the code and status?



3rdparty/libprocess/src/http.cpp (line 121)


Brace on newline :)



3rdparty/libprocess/src/tests/benchmarks.cpp (line 264)


Per my comment above, let's remove isStatus and just directly do equality 
against the 'code' field. Ditto for the other cases below.



3rdparty/libprocess/src/tests/http_tests.cpp (lines 128 - 130)


Could you use the -> operator here and in all of the code below to avoid 
the .get(). mess? Thanks Tim!


- Ben Mahler


On Oct. 8, 2015, 11:05 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38416/
> ---
> 
> (Updated Oct. 8, 2015, 11:05 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, Jie Yu, Jojy Varghese, 
> and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3429
> https://issues.apache.org/jira/browse/MESOS-3429
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow HTTP response codes to be checked with code.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> 591c1a959057155e1bf0f5bd73352e78d1c15cb3 
>   3rdparty/libprocess/src/decoder.hpp 
> 53b8079968a0145651bad9d11aa4be2b504de57a 
>   3rdparty/libprocess/src/http.cpp ebf76093d654397260fc8da84c4f2b0fd6541483 
>   3rdparty/libprocess/src/process.cpp 
> d1c81f1d244f02bf42cab97198587ce1b8c7c407 
>   3rdparty/libprocess/src/tests/benchmarks.cpp 
> bb9ced8933bf2bb97ae6b3cffdb5528676e53c11 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 38f3ad7e46f5b6ef4850cdf7fdcc115715e98472 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> ffd260a3fa2e49b3de183ba7b392b71afaaba2e5 
> 
> Diff: https://reviews.apache.org/r/38416/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38570: Change documentation image links to absolute paths.

[Joseph Wu]

> On Oct. 6, 2015, 2:29 p.m., Ben Mahler wrote:
> > docs/external-containerizer.md, line 92
> > 
> >
> > How will this work when we add multiple versions of documents to the 
> > website? Now this hardcodes "latest" as the version?
> 
> Joseph Wu wrote:
> I'm not sure.
> 
> From what I've seen of the website generation code, we currently wipe out 
> older versions of the docs during generation.  We'd first have to change the 
> site's Rakefile (and probably the release process) to keep older versions.
> 
> After that, we'd still have to deal with the problem of relative URLs and 
> trailing slashes:
> 
> http://stackoverflow.com/questions/5457885/relative-urls-and-trailing-slashes
> 
> Perhaps we could change the markdown parser to replace image links with a 
> versioned absolute path.  (I'm not sure what that would entail.)
> 
> OR
> 
> We could change all the image links to relative links (i.e. 
> `images/ec_launch_seqdiag.png`) and remove trailing slashes from all 
> documentation links.  (Maybe add redirects from trailing slash pages to 
> non-trailing slash pages.)
> 
> 
> What do you think?
> 
> Niklas Nielsen wrote:
> BenM: ping ^^
> 
> Ben Mahler wrote:
> Removing the trailing slashes on documentation links sounds good to me. 
> Also, why did you remove raw=true, was it a no-op, or?

I'll sync with Dave L. or Jake F. about the trailing slashes.

The `raw=true` isn't necessary anymore (I believe).  In the past, I think it 
was necessary for images to show up on the Github previews, but now images show 
with or without the `raw=true`.
(For example, the newer docs with images do not have `raw=true`, but they show 
up on the previews).


- Joseph


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


On Oct. 6, 2015, 9:45 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38570/
> ---
> 
> (Updated Oct. 6, 2015, 9:45 a.m.)
> 
> 
> Review request for mesos, Adam B, Dave Lester, Artem Harutyunyan, Kapil Arya, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-3183
> https://issues.apache.org/jira/browse/MESOS-3183
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also removes extraneous `?raw=true` from the links.
> 
> 
> Diffs
> -
> 
>   docs/external-containerizer.md 96932189b74dce1ae28e0bd73d5543d1afaffb0b 
>   docs/fetcher-cache-internals.md e8a68d1230420d1afca61f92c1ab6be12a70dbf2 
>   docs/maintenance.md a5831ffa092a9ea6decbe2e640bae637c759a308 
>   docs/networking-for-mesos-managed-containers.md 
> 33568a8233523ff3805f962371c94346e608208b 
>   docs/oversubscription.md 5a31b1ff7f003307817732a71f3b0a7c7d60cd24 
> 
> Diff: https://reviews.apache.org/r/38570/diff/
> 
> 
> Testing
> ---
> 
> Patched `Rakefile` to include images in the website (See JIRA for the patch).
> Then rendered with: https://github.com/mesosphere/mesos-website-container
> 
> Confirmed that images show up on the modified docs.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39249: Updated upgrades.md about TaskStatus::Reason naming changes.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38746, 39249]

All tests passed.

- Mesos ReviewBot


On Oct. 13, 2015, 12:14 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39249/
> ---
> 
> (Updated Oct. 13, 2015, 12:14 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2035
> https://issues.apache.org/jira/browse/MESOS-2035
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated upgrades.md about TaskStatus::Reason naming changes.
> 
> 
> Diffs
> -
> 
>   docs/upgrades.md a2e073ffb7e21d7788bcafb8ecc908b8d5a4c8a6 
> 
> Diff: https://reviews.apache.org/r/39249/diff/
> 
> 
> Testing
> ---
> 
> Tested the format in Mou
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 38746: Added TaskStatus::Reason to containerizer Termination message.

[Jie Yu]

> On Oct. 12, 2015, 9:03 p.m., Ben Mahler wrote:
> > include/mesos/mesos.proto, lines 1100-1101
> > 
> >
> > Shall we say why it's "bad"? i.e. the default value when a caller 
> > doesn't check for presence is 0 and so ideally the 0 reason is not a valid 
> > one.

Done. Thanks!


> On Oct. 12, 2015, 9:03 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, lines 1673-1695
> > 
> >
> > It's still a little unsettling that these container update / launch 
> > failure cases don't properly set the executor state to 
> > Executor::TERMINATING, can you at least add a TODO in these to signal the 
> > reader of the code?
> > 
> > Curious what we can do to simplify all of these redundant container 
> > update failure cases, worth thinking about later..

Yes, I'll follow up with a patch shortly. Working on it. I'll add a TODO for 
now.


> On Oct. 12, 2015, 9:03 p.m., Ben Mahler wrote:
> > include/mesos/mesos.proto, lines 1105-1107
> > 
> >
> > Would be nice to have these renames in the upgrade notes, mentioning 
> > that the numbers are backwards compatible but there needs to be a compile 
> > time adjustment for those relying on REASON_MEMORY_LIMIT :)

Expecting a small patch shortly.


- Jie


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


On Oct. 12, 2015, 7:33 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38746/
> ---
> 
> (Updated Oct. 12, 2015, 7:33 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2035
> https://issues.apache.org/jira/browse/MESOS-2035
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added TaskStatus::Reason to containerizer Termination message.
> 
> The following doc summarized the problem and proposed a solution:
> https://docs.google.com/document/d/1klGDAu5yBVf-CGWLqvELLIfxLfRaisGkhi6Gn7952-4/edit?usp=sharing
> 
> 
> Diffs
> -
> 
>   include/mesos/containerizer/containerizer.proto 
> f16ccc89f83da28c413ccfa0687a06b7515a605c 
>   include/mesos/mesos.proto 4a16be1f570769f3ce42a50a9da9f4fb1c227999 
>   include/mesos/slave/isolator.proto 9d38a25470a79af1eda9122c037c82b8cbbad6ed 
>   src/common/protobuf_utils.hpp 8793851fb927ab1326da6b6a424b3c6a75eb5001 
>   src/common/protobuf_utils.cpp c1e8e011e6d0b8e1cd8a836e3168685ec401b21b 
>   src/slave/containerizer/docker.cpp 6c975f904178e01797b67628a2d471ec7b3b1fbf 
>   src/slave/containerizer/external_containerizer.cpp 
> 211649201777f0d2ce802a865090129eacdd53be 
>   src/slave/containerizer/isolators/cgroups/mem.cpp 
> 89c86beb9227eb8a6e70a413e7b3934add652981 
>   src/slave/containerizer/isolators/posix/disk.cpp 
> c324c79f8d598095d07fbcb26e806a0978c2a520 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 4c1419290645ad4c44360a81618a6cea7ad190df 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b904b2d88e9b62fa4ba312c4569a4d89b0dc6052 
>   src/slave/slave.hpp 18be4f8188ad34ef4d0aa4b5eba241053d071476 
>   src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c 
>   src/tests/containerizer.cpp 1f743155526192569dd61a47ab67d8e58aab205a 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 8771ef661039310d79845513ea4602e15b2ad13d 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 5bc7d408bda0c249e1b66747d8bd87e688362e6c 
>   src/tests/oversubscription_tests.cpp 
> 2cf047e6ae061c1b9eed6238e1df67c91ab3514a 
>   src/tests/slave_recovery_tests.cpp fd285fbba0ec08fe2afbdd8da0b0b451126da0eb 
>   src/tests/slave_tests.cpp dccdbb0ec5352df5da63d5ef7261bfc7fd599acb 
> 
> Diff: https://reviews.apache.org/r/38746/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> tested with Docker as well.
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 38747: Adding digest utilities

[Jiang Yan Xu]

> On Oct. 1, 2015, 11:23 a.m., Jiang Yan Xu wrote:
> > Sorry I haven't chimed in earlier. I made one comment earlier with a 
> > reference to a pending review  but 
> > didn't look at the review closely. I also have a ticket 
> > https://issues.apache.org/jira/browse/MESOS-3191 create before.
> > 
> > I have a few questions, just to help me understand the context.
> > 
> > 1) Since the motivation for this util is for computing and verifying docker 
> > image hash, how will we use it? Given that this implementation requires 
> > USE_SSL_SOCKET, do we want to tie the docker provisioner to the 
> > --enable-ssl flag? Or should we create another flag or make `libssl-dev` or 
> > `openssl-devel` a dependency by default? What is the plan?
> > 
> > 2) What's the advantage of using openssl APIs directly? This implementation 
> > is obviously more complex than the alternative 
> > , which simply calls out to a few 
> > widely distributed system binaries on our supported platforms. (sha256sum 
> > sha512sum are part of GNU coreutils while shasum is on every mac). The 
> > linked review needs to address some comments but it's not far from ready 
> > for shipit (it's not a priority for us right now but you can take it if you 
> > like).
> > 
> > Thanks!
> 
> Jojy Varghese wrote:
> Thanks for feedback. To answer your questions:
> 
> - We currently depend on SSL for docker remote registry client as thats 
> the authentication type we support. The code exposes simple APIs that can be 
> called to get digest of a string, file etc. The test file serves as examples 
> of usage.
> - The advantage is that we can use the API without spawning a process. 
> Else you can image the number of spawns for a docker image with many layers. 
> In short efficiency. Another reason is that the review you pointed me to also 
> depends on those utils to be available. So we still need a fallback when 
> those utils are not available.
> - The code is open to adding more fallback implementations that can 
> incorporate the method in https://reviews.apache.org/r/34138/. So we can 
> still add those fallback. I would see this implementation as a superset and 
> not a replacement for the code presented in 
> https://reviews.apache.org/r/34138/.
> 
> -jojy
> 
> Jiang Yan Xu wrote:
> Sorry for the delay due to my travel. I realize much work has already 
> been done in this and we should probably still push this forward so I'll 
> comment on the code separately but the following is still high-level:
> 
> I guess I am still not fully sold on the necessity of this when we 
> already do the rest of the image provisioning via subprocess commands. (i.e., 
> `cp`, `tar`). Hashing using these commands look natual to me, especially 
> because it doesn't interact with the rest of the codebase in anyway. It maybe 
> something easier to do for the task at hand. Some of the thoughts below are 
> applicable if we adopt the native implementation as well.
> 
> ## SSL
> Ok I see that SSL is required for for docker (or more precicsely docker 
> when pulling from the registry). But how are we enabling this dependency? 
> IIUC currently `USE_SSL_SOCKET` and `--enable-ssl` are tied to libevent which 
> has nothing to do with SHA. I can understand that in order to use docker 
> registry I have to turn on libevent + SSL but if I just want to use the 
> hashing utility I have think I should be forced to switch from libev to 
> libevent. Maybe we should come up with another flag and preprocessor macro 
> for this (use openssl headers). FWIW I think openssl may have expected common 
> usage in the future to be a hard/unconditional dependency. Which is easier?
> 
> I am all for exposing simple and generic APIs for this utility but I 
> think we are discussing the implementation details here.
> 
> ## Overhead of spawning a process
> We already call `tar` and perhaps `cp` in subprocesses for each layer so 
> this is definitely not a bottleneck. :)
> 
> ## Dependency on the availability of the binary
> See my comments above. I think sha256sum and sha512sum on Linux and 
> shasum on OSX are widely distributed (more common than the openssl headers). 
> They appear to be as common as `tar` and `mount`.
> 
> ## Subprocess as a fallback.
> Sure they can coexist. I am merely talking about complexity and priority. 
> If openssl if a hard dependency then the subprocess implementation probably 
> doesn't have to exist. On the other hand, this native implementation does 
> data reading and processing in a serial manner, I don't know if these 
> binaries have any optimization. Note that for large binaries we are using 
> SHAing takes close to a minute so any optimization would be great to have.
> 
> Jojy Varghese wrote:
> Thanks for the comments Yan, inspite of your busy schedule.
> 
> To summarize the 

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


Bad patch!

Reviews applied: [39013, 38443, 39184, 39155, 39156, 38579, 39014, 39068, 
38941, 39015]

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

Error:
 2015-10-13 01:18:17 URL:https://reviews.apache.org/r/39015/diff/raw/ 
[7800/7800] -> "39015.patch" [1]
error: patch failed: 
src/slave/containerizer/provisioner/docker/registry_client.cpp:112
error: src/slave/containerizer/provisioner/docker/registry_client.cpp: patch 
does not apply
Failed to apply patch

- Mesos ReviewBot


On Oct. 13, 2015, 12:37 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39250/
> ---
> 
> (Updated Oct. 13, 2015, 12:37 a.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Puller refactor: moved untar to a common place
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp 
> 74d0e1ead7d630e65a7e75cb6123139b9197efef 
>   src/slave/containerizer/provisioner/docker/puller.hpp 
> 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 
>   src/slave/containerizer/provisioner/docker/puller.cpp 
> cb05324689ffa26ce830b513e2d71b55517da3cb 
>   src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39250/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38580: Added docker registry RemotePuller

[Jojy Varghese]

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

Can i add that as a separate patch?


- Jojy


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


On Oct. 12, 2015, 9:35 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> ---
> 
> (Updated Oct. 12, 2015, 9:35 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 828dbb690841c561816811dfbb044aa3afead89d 
>   src/Makefile.am d855cb83277c3e0e2ee3feacaf6ad0962223ef6e 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp 
> 4574e8a04663482625d7b54f765741f221ec13e0 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp 
> 74d0e1ead7d630e65a7e75cb6123139b9197efef 
>   src/slave/containerizer/provisioner/docker/puller.hpp 
> 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 
>   src/slave/containerizer/provisioner/docker/puller.cpp 
> cb05324689ffa26ce830b513e2d71b55517da3cb 
>   src/slave/containerizer/provisioner/docker/registry_client.hpp 
> fdb68b675582f603ffb3e96f31c1c9405e238567 
>   src/slave/containerizer/provisioner/docker/registry_client.cpp 
> 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 
>   src/slave/containerizer/provisioner/docker/remote_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp 
> 637c97c0973bda492826803a962c99635647692d 
>   src/slave/flags.hpp f76f0f6a0c7a01c7f061a225d7f6ef52be0ee7b5 
>   src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> d895eb9d0723e52cff8b21ef2deeaef1911d019c 
> 
> Diff: https://reviews.apache.org/r/38580/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

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

[Alexander Rukletsov]

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

Ship it!



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (lines 777 - 778)


s/google::protobuf::Message/protobuf message/ for consistency?



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 778)


Newline before



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (lines 785 - 786)


Newline, please!


- Alexander Rukletsov


On Oct. 4, 2015, 11:29 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 4, 2015, 11:29 a.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
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   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 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

> On 十月 12, 2015, 6:15 p.m., Michael Park wrote:
> > I've shipped this patch with the minor typos and style fixes outlined below.

Thanks Michael Park for the update ;-)


- Guangya


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


On 十月 6, 2015, 12:26 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated 十月 6, 2015, 12:26 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 8100f14220599247a340c919a03f29755b5349d8 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 38564: Add a new callback enabling custom attribute discovery logic

[Niklas Nielsen]

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

Ship it!


Mind rebasing this, Felix? :) We can land this after fixing the few comments 
below


include/mesos/hook.hpp (lines 129 - 133)


Let's wrap at 80 cols here and in comments below



include/mesos/hook.hpp (line 134)


Let's rename this to 'slaveAttributesDecorator': here and in rest of patch



src/tests/hook_tests.cpp (line 659)


Should we rename this test as you are testing both decorators here?
If not, maybe create a seperate test for it.


- Niklas Nielsen


On Sept. 22, 2015, 9:32 a.m., Felix Abecassis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38564/
> ---
> 
> (Updated Sept. 22, 2015, 9:32 a.m.)
> 
> 
> Review request for mesos, Connor Doyle and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3366
> https://issues.apache.org/jira/browse/MESOS-3366
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add a new callback enabling custom attribute discovery logic
> 
> 
> Diffs
> -
> 
>   include/mesos/hook.hpp 2353602c8ac8b89e3fa86bda7d1e262a3debef80 
>   src/examples/test_hook_module.cpp 0dc74d60576af6f88cbdc1c9a6f82348c5761d2f 
>   src/hook/manager.hpp a517c05855b07831a9728c1191243d114781b70a 
>   src/hook/manager.cpp 691976e94d5ac9fe4f8ba583214f24900d14248c 
>   src/slave/slave.cpp 29865ece00fa8bff3054a7f8c87cbf93403405db 
>   src/tests/hook_tests.cpp b23a587c683c391ca860b3b7d876902987f4d158 
> 
> Diff: https://reviews.apache.org/r/38564/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Felix Abecassis
> 
>

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 Oct. 13, 2015, 12:37 a.m.)


Review request for mesos and Timothy Chen.


Changes
---

moved more untar code to use common code.


Repository: mesos


Description
---

Puller refactor: moved untar to a common place


Diffs (updated)
-

  src/slave/containerizer/provisioner/docker/local_puller.cpp 
74d0e1ead7d630e65a7e75cb6123139b9197efef 
  src/slave/containerizer/provisioner/docker/puller.hpp 
105b4e75439c2ad4c08e2fd364f288f1d39b9b59 
  src/slave/containerizer/provisioner/docker/puller.cpp 
cb05324689ffa26ce830b513e2d71b55517da3cb 
  src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 39211: Check for trailing characters in JSON::parse().

[Adam B]

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


Looks great! Just a couple of suggestions to expand unit testing & logging.


3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (line 754)


Worth including s.substring(end..last_char) in the error message, so the 
user knows what trailing chars?



3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp (line 232)


Could also test the positive case that we correctly parse (no error) a 
jsonString with a valid element and only whitespace trailing.



3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp (line 252)


But parsing this as an array should succeed without error, correct? Can we 
validate that in the test too?


- Adam B


On Oct. 12, 2015, 11:29 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39211/
> ---
> 
> (Updated Oct. 12, 2015, 11:29 a.m.)
> 
> 
> Review request for mesos, Adam B and Joseph Wu.
> 
> 
> Bugs: MESOS-3698
> https://issues.apache.org/jira/browse/MESOS-3698
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Check for trailing characters in JSON::parse().
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 
> 0870300c2d372722b4850155cb2a8e848b986ed5 
>   3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp 
> b95667f4c1a1c170016da024a5e4af02ccaa064e 
>   3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 
> 131d39b87e571f16c774a133eb3148e98d8b9dd4 
> 
> Diff: https://reviews.apache.org/r/39211/diff/
> 
> 
> Testing
> ---
> 
> Added tests to make sure that JSON::parse() is successfully returning errors 
> when passed invalid JSON objects. Ran `make check`.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 38580: Added docker registry RemotePuller

[Jojy Varghese]

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

(Updated Oct. 12, 2015, 9:35 p.m.)


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


Changes
---

Added to cmake.


Repository: mesos


Description
---

Integrated remote puller with store. Tests will follow.


Diffs (updated)
-

  src/CMakeLists.txt 828dbb690841c561816811dfbb044aa3afead89d 
  src/Makefile.am d855cb83277c3e0e2ee3feacaf6ad0962223ef6e 
  src/slave/containerizer/provisioner/docker/local_puller.hpp 
4574e8a04663482625d7b54f765741f221ec13e0 
  src/slave/containerizer/provisioner/docker/local_puller.cpp 
74d0e1ead7d630e65a7e75cb6123139b9197efef 
  src/slave/containerizer/provisioner/docker/puller.hpp 
105b4e75439c2ad4c08e2fd364f288f1d39b9b59 
  src/slave/containerizer/provisioner/docker/puller.cpp 
cb05324689ffa26ce830b513e2d71b55517da3cb 
  src/slave/containerizer/provisioner/docker/registry_client.hpp 
fdb68b675582f603ffb3e96f31c1c9405e238567 
  src/slave/containerizer/provisioner/docker/registry_client.cpp 
4931ae8869a697b1e9d8d4cbc0a871e7cd506285 
  src/slave/containerizer/provisioner/docker/remote_puller.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/store.cpp 
637c97c0973bda492826803a962c99635647692d 
  src/slave/flags.hpp f76f0f6a0c7a01c7f061a225d7f6ef52be0ee7b5 
  src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 
  src/tests/containerizer/provisioner_docker_tests.cpp 
d895eb9d0723e52cff8b21ef2deeaef1911d019c 

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


Testing
---

make check.


Thanks,

Jojy Varghese

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

[Alexander Rukletsov]

> On Sept. 29, 2015, 12:14 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 762-763
> > 
> >
> > I've seen your discussion with @Jan above, here is what I think 
> > regarding `ABORT` vs. `Try<>`. 
> > 
> > First off, some background. We tend to use `ABORT()` and `CHECK_*` 
> > macros when so-called "internal invariant" is violated, for example an 
> > object is being used without proper initialization, or a change is being 
> > made to an instance, that does not exists any more. On the other side 
> > `Try<>` and `Error<>` family is used when our expectation about the outer 
> > world does not hold, or, in other words, when an action cannot be 
> > completed, but no internal invariant is broken. User input, I/O, network 
> > operations are good examples of the latter case.
> > 
> > Let's try to figure out what we have here. I would say, it's an 
> > internal invariant, and here is why. JSON is less strict as Protobuf, 
> > therefore conversion Protobuf->JSON always exists (note that this is not 
> > symmetrical, JSON->Protobuf is not always possible, hence we use `Try<>` in 
> > e.g. `protobuf::parse<>()`). The only reason it may fail (remember we do 
> > not handle OOM exceptions) is because we convert a protobuf message of a 
> > yet unknown format (most probably future proto versions), which is a 
> > violation of an internal invariant!
> > 
> > Therefore I would suggest we keep `ABORT()` but document in the 
> > preambular comment why we use `ABORT()` and not `Try<>`, explaining what 
> > internal invariant is in this case.

I would like us to document the motivation for `ABORT()` here.


- Alexander


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


On Oct. 4, 2015, 11:29 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 4, 2015, 11:29 a.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
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   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
> 
>

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

Review request for mesos and Timothy Chen.


Repository: mesos


Description
---

Puller refactor: moved untar to a common place


Diffs
-

  src/slave/containerizer/provisioner/docker/local_puller.cpp 
74d0e1ead7d630e65a7e75cb6123139b9197efef 
  src/slave/containerizer/provisioner/docker/puller.hpp 
105b4e75439c2ad4c08e2fd364f288f1d39b9b59 
  src/slave/containerizer/provisioner/docker/puller.cpp 
cb05324689ffa26ce830b513e2d71b55517da3cb 
  src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38564: Add a new callback enabling custom attribute discovery logic

[Felix Abecassis]

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

(Updated Oct. 13, 2015, 1:39 a.m.)


Review request for mesos, Connor Doyle and Niklas Nielsen.


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


Repository: mesos


Description
---

Add a new callback enabling custom attribute discovery logic


Diffs (updated)
-

  include/mesos/hook.hpp 0c1042a 
  src/examples/test_hook_module.cpp cd7c184 
  src/hook/manager.hpp 3af1ff8 
  src/hook/manager.cpp 108bd46 
  src/slave/slave.cpp 01c5e42 
  src/tests/hook_tests.cpp b35ce72 

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


Testing
---


Thanks,

Felix Abecassis

Re: Review Request 38570: Change documentation image links to absolute paths.

[Ben Mahler]

> On Oct. 6, 2015, 9:29 p.m., Ben Mahler wrote:
> > docs/external-containerizer.md, line 92
> > 
> >
> > How will this work when we add multiple versions of documents to the 
> > website? Now this hardcodes "latest" as the version?
> 
> Joseph Wu wrote:
> I'm not sure.
> 
> From what I've seen of the website generation code, we currently wipe out 
> older versions of the docs during generation.  We'd first have to change the 
> site's Rakefile (and probably the release process) to keep older versions.
> 
> After that, we'd still have to deal with the problem of relative URLs and 
> trailing slashes:
> 
> http://stackoverflow.com/questions/5457885/relative-urls-and-trailing-slashes
> 
> Perhaps we could change the markdown parser to replace image links with a 
> versioned absolute path.  (I'm not sure what that would entail.)
> 
> OR
> 
> We could change all the image links to relative links (i.e. 
> `images/ec_launch_seqdiag.png`) and remove trailing slashes from all 
> documentation links.  (Maybe add redirects from trailing slash pages to 
> non-trailing slash pages.)
> 
> 
> What do you think?
> 
> Niklas Nielsen wrote:
> BenM: ping ^^

Removing the trailing slashes on documentation links sounds good to me. Also, 
why did you remove raw=true, was it a no-op, or?


- Ben


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


On Oct. 6, 2015, 4:45 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38570/
> ---
> 
> (Updated Oct. 6, 2015, 4:45 p.m.)
> 
> 
> Review request for mesos, Adam B, Dave Lester, Artem Harutyunyan, Kapil Arya, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-3183
> https://issues.apache.org/jira/browse/MESOS-3183
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also removes extraneous `?raw=true` from the links.
> 
> 
> Diffs
> -
> 
>   docs/external-containerizer.md 96932189b74dce1ae28e0bd73d5543d1afaffb0b 
>   docs/fetcher-cache-internals.md e8a68d1230420d1afca61f92c1ab6be12a70dbf2 
>   docs/maintenance.md a5831ffa092a9ea6decbe2e640bae637c759a308 
>   docs/networking-for-mesos-managed-containers.md 
> 33568a8233523ff3805f962371c94346e608208b 
>   docs/oversubscription.md 5a31b1ff7f003307817732a71f3b0a7c7d60cd24 
> 
> Diff: https://reviews.apache.org/r/38570/diff/
> 
> 
> Testing
> ---
> 
> Patched `Rakefile` to include images in the website (See JIRA for the patch).
> Then rendered with: https://github.com/mesosphere/mesos-website-container
> 
> Confirmed that images show up on the modified docs.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39213: Windows: Moved `bootId` to is own file, `stout/os/windows/bootid.hpp`.

[Joseph Wu]

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

Ship it!



3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/bootid.hpp (line 47)


Maybe add a `// __linux__` here, even though it wasn't there originally.


- Joseph Wu


On Oct. 11, 2015, 11:16 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39213/
> ---
> 
> (Updated Oct. 11, 2015, 11:16 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Bugs: MESOS-3632
> https://issues.apache.org/jira/browse/MESOS-3632
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Moved `bootId` to is own file, `stout/os/windows/bootid.hpp`.
> 
> (NOTE: this does not close MESOS-3632, but does contribute to its resolution.)
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 9e9c3119ad18f4cbc70c70095c71dc4fd19553df 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 5141c1369af60afd6cd5c70c6295d575ea960a83 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/bootid.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/bootid.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/bootid.hpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39213/diff/
> 
> 
> Testing
> ---
> 
> Built CMake solution and ran `make check` (or equivalent) on Windows 10, 
> Ubuntu 15, OS X 10.10. Built autotools solution and ran `make check` on 
> Ubuntu 15.
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 37996: Added InheritanceTree a tree based container where children nodes inherit the values associated with their parent.

[Ben Mahler]

> On Oct. 6, 2015, 12:39 p.m., Bernd Mathiske wrote:
> > Ship It!
> 
> Ben Mahler wrote:
> I don't think we should introduce this into stout in its current form. I 
> realize you're planning to use this for authentication stuff, but looking at 
> this on its own, it seems like a confusing abstraction. Why would we couple 
> the notion of a Tree with the semantics around properties and property 
> inheritance?
> 
> Till Toenshoff wrote:
> This code is being used in libprocess. So the options are libprocess or 
> stout for introducing it. I believe it would be a better fit for stout than 
> for libprocess as it is a data structure implementation that has no threading 
> or process specifics. Given that it already is in a reusable state, I think 
> that we should go as proposed by this RR.
> 
> Bernd Mathiske wrote:
> @bmahler, what would you rather have us do? This structure is quite close 
> to Boost's PropertyTree 
> (http://www.boost.org/doc/libs/1_59_0/doc/html/property_tree.html), and we 
> would have loved to use that one. But it does not feature inheritance, which 
> seems a natural, essential thing to have in at least some trees.
> 
> Ben Mahler wrote:
> Hm... the "inheritance" stuff that you guys need seems to just be a walk 
> of the tree from the root to the leaf you're interested in, collecting 
> properties of nodes along the way. Any reason you can't layer that 
> functionality on top or, even better, on the side via a free standing 
> function so that you don't need to introduce a new data structure?
> 
> Jie Yu wrote:
> +1 if we can compose/inherit from boost property tree rather than 
> implementing our own.
> 
> Alexander Rojas wrote:
> Property tree as of the moment is not distributed with Mesos. However, 
> there is a reason why it wasn't added, and it is that it will require a 
> double parsing of the path (one when asking the ptree, the other manually to 
> verify every subpath).

In other words, you were concerned about performance?


- Ben


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


On Oct. 7, 2015, 9:54 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37996/
> ---
> 
> (Updated Oct. 7, 2015, 9:54 a.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 `InheritanceTree` class which allows to create a tree where 
> nodes can be tag with some property. This property is then inherited by 
> children nodes.
> 
> Two behaviors are implemented, overriding, i.e. Adding a property to the 
> child node of another node with a property already will result in the 
> ancestor property being lost. The second behavior, accumulating, takes a 
> function and accumulates
> properties of all ancestors.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 
> 76e1674e08bbe65a4fdf86731823a61f231d6d12 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 9e9c3119ad18f4cbc70c70095c71dc4fd19553df 
>   3rdparty/libprocess/3rdparty/stout/include/stout/inheritancetree.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
> 94292f8a46ec31bbaf6e52f48109322bbe123f70 
>   3rdparty/libprocess/3rdparty/stout/tests/inheritancetree_tests.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37996/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 39205: Deprecate resource_monitoring_interval flag

[Niklas Nielsen]

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

Ship it!


Ship It!

- Niklas Nielsen


On Oct. 11, 2015, 8:32 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39205/
> ---
> 
> (Updated Oct. 11, 2015, 8:32 p.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3700
> https://issues.apache.org/jira/browse/MESOS-3700
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This flag should be deprecated after 0.23.0 as it is no use now.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2ab476a2d2c43e309b570d73ecac80e27b296e7e 
>   docs/oversubscription.md 5a31b1ff7f003307817732a71f3b0a7c7d60cd24 
>   src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341 
>   src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55 
>   src/slave/flags.hpp f76f0f6a0c7a01c7f061a225d7f6ef52be0ee7b5 
>   src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 
> 
> Diff: https://reviews.apache.org/r/39205/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

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

[Joseph Wu]

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



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


Don't you need to include `windows.hpp` to get `PATH_MAX` (on Windows, of 
course)?


- Joseph Wu


On Oct. 10, 2015, 5:23 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39210/
> ---
> 
> (Updated Oct. 10, 2015, 5:23 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Bugs: MESOS-3632
> https://issues.apache.org/jira/browse/MESOS-3632
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Moved `realpath` to its own file, `stout/os/realpath.hpp`.
> 
> (NOTE: This does not resolve MESOS-3632, it's only a partial solution.)
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 9e9c3119ad18f4cbc70c70095c71dc4fd19553df 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 5141c1369af60afd6cd5c70c6295d575ea960a83 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/realpath.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/exists.hpp 
> ddcda7bdb294272a7a64a7a46e0576e8c4430eec 
> 
> Diff: https://reviews.apache.org/r/39210/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 38443: Added layerid information to ManifestResponse

[Timothy Chen]

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

Ship it!


Ship It!

- Timothy Chen


On Oct. 9, 2015, 6:34 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38443/
> ---
> 
> (Updated Oct. 9, 2015, 6:34 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added layerid information to ManifestResponse
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/provisioner/docker/registry_client.hpp 
> fdb68b675582f603ffb3e96f31c1c9405e238567 
>   src/slave/containerizer/provisioner/docker/registry_client.cpp 
> 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> d895eb9d0723e52cff8b21ef2deeaef1911d019c 
> 
> Diff: https://reviews.apache.org/r/38443/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>