Re: Review Request 39889: Windows: Added support for `files/files.hpp`.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39537, 39538, 39539, 39540, 39541, 39383, 39559, 39219, 
39560, 39583, 39584, 39620, 39621, 39622, 39623, 39019, 39802, 39803, 39804, 
39805, 39834, 39850, 39851, 39852, 39888, 39889]

All tests passed.

- Mesos ReviewBot


On Nov. 3, 2015, 5:38 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39889/
> ---
> 
> (Updated Nov. 3, 2015, 5:38 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Added support for `files/files.hpp`.
> 
> 
> Diffs
> -
> 
>   src/files/files.hpp 818087b13cc787d0bd3186bb3e8a069751629bf9 
> 
> Diff: https://reviews.apache.org/r/39889/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Review Request 39890: Slave do not support multiple masters when start up

[Guangya Liu]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

The configuration document needs to be updated.


Diffs
-

  docs/configuration.md e66013ad0b951049df645ff90907f19ae8081cc2 

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


Testing
---


Thanks,

Guangya Liu

Review Request 39889: Windows: Added support for `files/files.hpp`.

[Alex Clemmer]

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

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


Repository: mesos


Description
---

Windows: Added support for `files/files.hpp`.


Diffs
-

  src/files/files.hpp 818087b13cc787d0bd3186bb3e8a069751629bf9 

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


Testing
---


Thanks,

Alex Clemmer

Review Request 39888: Windows: Added compatibility code for `grp.h` and `pwd.h`.

[Alex Clemmer]

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

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


Repository: mesos


Description
---

This code will be particularly useful when we expand Windows support for
`files/files.hpp`.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
67b142bbc2d80f40a1e893cc5813d58dd2aa8381 
  3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/grp.hpp 
PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/pwd.hpp 
PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
1a7037d64afeedc340258c92067e95d1d3caa027 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 39493: Added `yum install nss` to CentOS 6.6 install docs.

[Adam B]

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

Ship it!


Ship It!


docs/getting-started.md (line 57)


Looks like we're wrapping the "script" comments in this file, but I can do 
that before I commit.


- Adam B


On Nov. 2, 2015, 12:54 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39493/
> ---
> 
> (Updated Nov. 2, 2015, 12:54 p.m.)
> 
> 
> Review request for mesos, Adam B and haosdent huang.
> 
> 
> Bugs: MESOS-3506
> https://issues.apache.org/jira/browse/MESOS-3506
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `yum install nss` to CentOS 6.6 install docs.
> 
> 
> Diffs
> -
> 
>   docs/getting-started.md 35c8c566fff83abfcfcce177bfb9a35454f26494 
> 
> Diff: https://reviews.apache.org/r/39493/diff/
> 
> 
> Testing
> ---
> 
> Viewed in the Mesos Website Container: 
> https://github.com/mesosphere/mesos-website-container
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39886: Added documentation about roles.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39886]

All tests passed.

- Mesos ReviewBot


On Nov. 3, 2015, 1:51 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39886/
> ---
> 
> (Updated Nov. 3, 2015, 1:51 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-3819
> https://issues.apache.org/jira/browse/MESOS-3819
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also removed a redundant (and arguably slightly wrong) assertion about the
> status of static reservations.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md f712d094f14426515dabde45f98d6c1ae36c3447 
>   docs/configuration.md e66013ad0b951049df645ff90907f19ae8081cc2 
>   docs/home.md af886257348f24cfeecfc726255d8d45d68af2db 
>   docs/reservation.md 69bde760ece59c68b04a903026b5903e2091ceb0 
>   docs/roles.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39886/diff/
> 
> 
> Testing
> ---
> 
> Previewed with the support/site Docker container.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 39827: Made slave version required in master.cpp.

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 十一月 2, 2015, 7:34 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39827/
> ---
> 
> (Updated 十一月 2, 2015, 7:34 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Slave version is being set by the slave since 0.21.0. Cleaned up the code in 
> master that expected it to be optional.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac 
>   src/master/master.cpp 9f4586e668a2141f4937497d42853fbdea7751a5 
> 
> Diff: https://reviews.apache.org/r/39827/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

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

[Timothy Chen]

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

i think alternatifely i can also let Rootfs clasd create a sandbox mount point 
too


- Timothy


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


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

Re: Review Request 39285: Added Quota Request Validation.

[Guangya Liu]

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



src/master/quota_handler.cpp (lines 157 - 158)


one line?


- Guangya Liu


On 十月 24, 2015, 7:42 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated 十月 24, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Guangya Liu]

> On 十一月 2, 2015, 4:43 p.m., Alexander Rukletsov wrote:
> > src/master/quota_handler.cpp, line 84
> > 
> >
> > Mind adding a comment this is a "reference" role which is deduced from 
> > the resources in the request and is not specified directly? For example:
> > 
> > ```
> > // A role for which quota request is sent. Currently only one role per 
> > request is allowed. Since an operator does not specify role explicitly, it 
> > is deduced from the provided resources.
> > ```

Alex, I think that we should treat the role as explicitly specified in the http 
request as the http request does include roles for the quota. The operator does 
specify the role explicitly in the http request, comments?


- Guangya


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


On 十月 24, 2015, 7:42 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated 十月 24, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

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

[Timothy Chen]

> On Nov. 3, 2015, 1:30 a.m., Jie Yu wrote:
> > src/tests/containerizer/filesystem_isolator_tests.cpp, line 286
> > 
> >
> > any reason you use 'copy'. Can we do bind to speed up tests?

it failed when the filesystem isolator tries to mkdir sandbox


- Timothy


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


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

Re: Review Request 39886: Added documentation about roles.

[Guangya Liu]

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



docs/attributes-resources.md (line 49)


MESOS-3177 is planning to enable adding role dynamically, we may need to 
add a TODO here to reflect this.


- Guangya Liu


On Nov. 3, 2015, 1:51 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39886/
> ---
> 
> (Updated Nov. 3, 2015, 1:51 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-3819
> https://issues.apache.org/jira/browse/MESOS-3819
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also removed a redundant (and arguably slightly wrong) assertion about the
> status of static reservations.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md f712d094f14426515dabde45f98d6c1ae36c3447 
>   docs/configuration.md e66013ad0b951049df645ff90907f19ae8081cc2 
>   docs/home.md af886257348f24cfeecfc726255d8d45d68af2db 
>   docs/reservation.md 69bde760ece59c68b04a903026b5903e2091ceb0 
>   docs/roles.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39886/diff/
> 
> 
> Testing
> ---
> 
> Previewed with the support/site Docker container.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

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

[Michael Park]

> On Sept. 29, 2015, 3:48 p.m., Alexander Rukletsov wrote:
> > src/tests/mesos.hpp, line 1844
> > 
> >
> > We tend not to use `typedef`s in the codebase. However, this looks like 
> > a good idea, but I would place it in `Resources`, e.g. 
> > `Resources::ProtoType`.
> > 
> > However, I'll leave the decision whether to use `typedef` or not to you 
> > and your shepherd.
> 
> Klaus Ma wrote:
> It's related to test only, so keep it in test module for now.

This doesn't seem like an intuitive typedef nor is it all that helpful. I think 
it would be better to use a `using` declaration instead.

`using google::protobuf::RepeatedPtrField;` then we can use 
`RepeatedPtrField`. I would also suggest breaking it out of the 
single expression to reduce the clutterness. For example:

```cpp
string body = "resources=" +
  stringify(JSON::protobuf(
  static_cast(dynamicallyReserved)));
```
could be:
```cpp
auto protobuf = JSON::protobuf(
static_cast&>(dynamicallyReserved));

string body = "resources=" + stringify(protobuf);
```

Sidenote: the `static_cast` to `const &` is to avoid an unnecessary copy.


- Michael


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


On Oct. 20, 2015, 5:37 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38335/
> ---
> 
> (Updated Oct. 20, 2015, 5:37 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
> -
> 
>   src/common/http.cpp f56d8a1 
>   src/docker/executor.cpp 1e49013 
>   src/examples/persistent_volume_framework.cpp 176ac3d 
>   src/launcher/executor.cpp 50b3c6e 
>   src/master/contender.cpp c641305 
>   src/master/http.cpp 093f793 
>   src/master/maintenance.cpp 5fe9358 
>   src/master/registrar.cpp 1117232 
>   src/slave/containerizer/fetcher.cpp e0d02d5 
>   src/slave/containerizer/mesos/containerizer.cpp d1fc5a4 
>   src/slave/monitor.cpp aa6e958 
>   src/tests/containerizer/launch_tests.cpp de655ec 
>   src/tests/fault_tolerance_tests.cpp f78a291 
>   src/tests/master_maintenance_tests.cpp e89ce3b 
>   src/tests/master_tests.cpp ee24739 
>   src/tests/mesos.hpp 3e58b45 
>   src/tests/mesos.cpp ab2d85b 
>   src/tests/monitor_tests.cpp 583e711 
>   src/tests/reservation_endpoints_tests.cpp f5f9c48 
>   src/tests/resources_tests.cpp 6584fc6 
>   src/tests/scheduler_http_api_tests.cpp d338a1b 
>   src/tests/script.cpp bcc1fab 
>   src/tests/slave_tests.cpp 10a4fa7 
>   src/usage/main.cpp 86fd796 
> 
> Diff: https://reviews.apache.org/r/38335/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 39611: Remove binary `data` fields from state endpoints.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39611]

All tests passed.

- Mesos ReviewBot


On Nov. 3, 2015, 1:06 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39611/
> ---
> 
> (Updated Nov. 3, 2015, 1:06 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-3771
> https://issues.apache.org/jira/browse/MESOS-3771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not encode or otherwise preprocess the raw binary in the ExecutorInfo 
> or TaskInfo `data` fields before outputting them to JSON via the state 
> endpoints.  This means that the outputted JSON may be invalid (i.e. non-UTF8) 
> if `data` was filled in with arbitrary bytes.
> 
> The `data` fields are being removed because the state endpoints should not be 
> writing binary data in the first place.
> 
> 
> Diffs
> -
> 
>   docs/upgrades.md 3854e59f68d53892b2673a4542ea2aaf96936cbb 
>   src/common/http.cpp f56d8a178e1f61d91adfeaad32b47718adbc4379 
>   src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 
> 
> Diff: https://reviews.apache.org/r/39611/diff/
> 
> 
> Testing
> ---
> 
> `make check` and visually checked markdown for correctness.
> 
> Apparently, none of our tests check these fields.  
> 
> (Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
> enough to catch the invalid-JSON that results.  It will happily parse the 
> binary blobs (roundtripping successfully).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

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

[Michael Park]

> On Oct. 22, 2015, 8:53 p.m., Jan Schlicht wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 800
> > 
> >
> > Not yours, but please s/push_back/emplace_back

I'm curious as to what your reasoning is?


- Michael


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


On Oct. 20, 2015, 5:37 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38335/
> ---
> 
> (Updated Oct. 20, 2015, 5:37 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
> -
> 
>   src/common/http.cpp f56d8a1 
>   src/docker/executor.cpp 1e49013 
>   src/examples/persistent_volume_framework.cpp 176ac3d 
>   src/launcher/executor.cpp 50b3c6e 
>   src/master/contender.cpp c641305 
>   src/master/http.cpp 093f793 
>   src/master/maintenance.cpp 5fe9358 
>   src/master/registrar.cpp 1117232 
>   src/slave/containerizer/fetcher.cpp e0d02d5 
>   src/slave/containerizer/mesos/containerizer.cpp d1fc5a4 
>   src/slave/monitor.cpp aa6e958 
>   src/tests/containerizer/launch_tests.cpp de655ec 
>   src/tests/fault_tolerance_tests.cpp f78a291 
>   src/tests/master_maintenance_tests.cpp e89ce3b 
>   src/tests/master_tests.cpp ee24739 
>   src/tests/mesos.hpp 3e58b45 
>   src/tests/mesos.cpp ab2d85b 
>   src/tests/monitor_tests.cpp 583e711 
>   src/tests/reservation_endpoints_tests.cpp f5f9c48 
>   src/tests/resources_tests.cpp 6584fc6 
>   src/tests/scheduler_http_api_tests.cpp d338a1b 
>   src/tests/script.cpp bcc1fab 
>   src/tests/slave_tests.cpp 10a4fa7 
>   src/usage/main.cpp 86fd796 
> 
> Diff: https://reviews.apache.org/r/38335/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

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

[Michael Park]

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

Ship it!


Ship It!

- Michael Park


On Oct. 20, 2015, 2:39 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 20, 2015, 2:39 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 f0e7870 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 39886: Added documentation about roles.

[Neil Conway]

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


Lots more we could discuss: ACLs, more details on DRF, etc. -- but this is a 
start.

- Neil Conway


On Nov. 3, 2015, 1:51 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39886/
> ---
> 
> (Updated Nov. 3, 2015, 1:51 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-3819
> https://issues.apache.org/jira/browse/MESOS-3819
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also removed a redundant (and arguably slightly wrong) assertion about the
> status of static reservations.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md f712d094f14426515dabde45f98d6c1ae36c3447 
>   docs/configuration.md e66013ad0b951049df645ff90907f19ae8081cc2 
>   docs/home.md af886257348f24cfeecfc726255d8d45d68af2db 
>   docs/reservation.md 69bde760ece59c68b04a903026b5903e2091ceb0 
>   docs/roles.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39886/diff/
> 
> 
> Testing
> ---
> 
> Previewed with the support/site Docker container.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 39886: Added documentation about roles.

[Neil Conway]

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

Review request for mesos and Adam B.


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


Repository: mesos


Description
---

Also removed a redundant (and arguably slightly wrong) assertion about the
status of static reservations.


Diffs
-

  docs/attributes-resources.md f712d094f14426515dabde45f98d6c1ae36c3447 
  docs/configuration.md e66013ad0b951049df645ff90907f19ae8081cc2 
  docs/home.md af886257348f24cfeecfc726255d8d45d68af2db 
  docs/reservation.md 69bde760ece59c68b04a903026b5903e2091ceb0 
  docs/roles.md PRE-CREATION 

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


Testing
---

Previewed with the support/site Docker container.


Thanks,

Neil Conway

Re: Review Request 39611: Remove binary `data` fields from state endpoints.

[Guangya Liu]

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



docs/upgrades.md (line 16)


Shall we highlight `data` here?


- Guangya Liu


On 十一月 3, 2015, 1:06 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39611/
> ---
> 
> (Updated 十一月 3, 2015, 1:06 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-3771
> https://issues.apache.org/jira/browse/MESOS-3771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not encode or otherwise preprocess the raw binary in the ExecutorInfo 
> or TaskInfo `data` fields before outputting them to JSON via the state 
> endpoints.  This means that the outputted JSON may be invalid (i.e. non-UTF8) 
> if `data` was filled in with arbitrary bytes.
> 
> The `data` fields are being removed because the state endpoints should not be 
> writing binary data in the first place.
> 
> 
> Diffs
> -
> 
>   docs/upgrades.md 3854e59f68d53892b2673a4542ea2aaf96936cbb 
>   src/common/http.cpp f56d8a178e1f61d91adfeaad32b47718adbc4379 
>   src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 
> 
> Diff: https://reviews.apache.org/r/39611/diff/
> 
> 
> Testing
> ---
> 
> `make check` and visually checked markdown for correctness.
> 
> Apparently, none of our tests check these fields.  
> 
> (Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
> enough to catch the invalid-JSON that results.  It will happily parse the 
> binary blobs (roundtripping successfully).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

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

[Jie Yu]

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



src/slave/containerizer/mesos/containerizer.cpp 


Any reason remove this check?


- Jie Yu


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

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

[Jie Yu]

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


Have you updated the Makefile since provisioner.hpp is removed in this patch?


src/tests/containerizer/filesystem_isolator_tests.cpp (lines 119 - 124)


Can we just use Backend::create(flags) instead?



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


any reason you use 'copy'. Can we do bind to speed up tests?



src/tests/containerizer/filesystem_isolator_tests.cpp (lines 319 - 327)


I think we can leverage the test utility `createTask` here to create the 
command line task.


- Jie Yu


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

Re: Review Request 39869: Added provisioner TestStore.

[Jie Yu]

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

Ship it!



src/tests/containerizer/store.hpp (line 80)


Add parathesis around initializing list:

```
return vector({rootfs.get()->root});
```


- Jie Yu


On Nov. 2, 2015, 7:04 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39869/
> ---
> 
> (Updated Nov. 2, 2015, 7:04 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added provisioner TestStore.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/store.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39869/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Jie Yu]

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



src/slave/containerizer/mesos/provisioner/provisioner.hpp (line 49)


Sort them alphebetically


- Jie Yu


On Nov. 2, 2015, 7:04 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39868/
> ---
> 
> (Updated Nov. 2, 2015, 7:04 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved ProvisionerProcess to header for testing.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 912fc5abadb1219fc4baec1a751010522bc7a7d0 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> e99c1c9876b836be5d3efcef7408b5ed01d8984e 
> 
> Diff: https://reviews.apache.org/r/39868/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Jie Yu]

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

Ship it!



src/slave/containerizer/mesos/provisioner/provisioner.hpp (line 37)


Kill one extra line here.


- Jie Yu


On Nov. 2, 2015, 7:04 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39868/
> ---
> 
> (Updated Nov. 2, 2015, 7:04 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved ProvisionerProcess to header for testing.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 912fc5abadb1219fc4baec1a751010522bc7a7d0 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> e99c1c9876b836be5d3efcef7408b5ed01d8984e 
> 
> Diff: https://reviews.apache.org/r/39868/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 39611: Remove binary `data` fields from state endpoints.

[Joseph Wu]

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

(Updated Nov. 2, 2015, 5:06 p.m.)


Review request for mesos, Ben Mahler and Artem Harutyunyan.


Changes
---

Add a note to `upgrades.md`.


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


Repository: mesos


Description
---

We do not encode or otherwise preprocess the raw binary in the ExecutorInfo or 
TaskInfo `data` fields before outputting them to JSON via the state endpoints.  
This means that the outputted JSON may be invalid (i.e. non-UTF8) if `data` was 
filled in with arbitrary bytes.

The `data` fields are being removed because the state endpoints should not be 
writing binary data in the first place.


Diffs (updated)
-

  docs/upgrades.md 3854e59f68d53892b2673a4542ea2aaf96936cbb 
  src/common/http.cpp f56d8a178e1f61d91adfeaad32b47718adbc4379 
  src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 

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


Testing (updated)
---

`make check` and visually checked markdown for correctness.

Apparently, none of our tests check these fields.  

(Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
enough to catch the invalid-JSON that results.  It will happily parse the 
binary blobs (roundtripping successfully).


Thanks,

Joseph Wu

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

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39791, 39792, 39827, 39878, 39873]

All tests passed.

- Mesos ReviewBot


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

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

[Jie Yu]

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


As Alex suggested, please update cmakelist as well.


src/Makefile.am (line 860)


Probably call this mount.hpp|cpp. See comments below.



src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (lines 469 - 470)


```
MesosContainerizerMount::Flags mountFlags;
mountFlags.operation = MesosContainerizerMount::MAKE_RSLAVE;
mountFlags.path = "/";

...
```



src/slave/containerizer/mesos/mark_mounts_rslave.hpp (line 36)


I am thinking about making this more general and can be extended in case we 
need to (e.g., supporting make-slave, make-rshared, or other mount operation).

How about calling this Subcommand:
`MesosContainerizerMount`

```
class MesosContainerizerMount : public Subcommand
{
public:
  static constexpr char NAME[] = ""
  static constexpr char MAKE_RSLAVE[] = "make-rslave";
  
  struct Flags : public flags:: FlagsBase
  {
Flags();

std::string operation;
Option path;
  };
};

int MesosContainerizerMount::execute()
{
  if (flags.operation == MAKE_RSLAVE) {
if (flags.path.isNone()) {
  ..
}

fs::mount(...);
  } else {
cerr << "Unknown operation: " << flags.operation;
return 1;
  }
}
```


- Jie Yu


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

Re: Review Request 39410: Added support for github to apply-reviews.py.

[Vinod Kone]

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



support/apply-reviews.py (line 47)


i have seen this in the previous reviews too. so wanted to confirm.

why do you use urlparse.urljoin() for the latter part of this url, but use 
"/" for the former part?



support/apply-reviews.py (lines 104 - 108)


this is really confusing. why do you need both a 'dry_run' and a 
'ignore_dry_run' argument?

honestly i liked the previous version.



support/apply-reviews.py (line 125)


this is weird too. a 'remove patch' function which takes optional patch as 
an argument.



support/apply-reviews.py (lines 130 - 133)


This log shouldn't be in the caller instead of this function.



support/apply-reviews.py (line 152)


does this only fetch from review board or github too?



support/apply-reviews.py (lines 162 - 164)


instead of a second parameter, have shell() function take one argument 
'dry_run' and act according to it, instead of looking into the global 'options'.

also, this says "in case of github" but it is doing the same for 
reviewboard too? either the 1) comment needs to be fixed or 2) this should be 
in a 'if else'.


- Vinod Kone


On Oct. 30, 2015, 8:55 a.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39410/
> ---
> 
> (Updated Oct. 30, 2015, 8:55 a.m.)
> 
> 
> Review request for mesos, Adam B, Joris Van Remoortere, Joseph Wu, Marco 
> Massenzio, and Vinod Kone.
> 
> 
> Bugs: MESOS-3468
> https://issues.apache.org/jira/browse/MESOS-3468
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for github to apply-reviews.py.
> 
> 
> Diffs
> -
> 
>   support/apply-reviews.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39410/diff/
> 
> 
> Testing
> ---
> 
> Tested with python 2.7
> 
> 
> Thanks,
> 
> Artem Harutyunyan
> 
>

Re: Review Request 37336: Simplified the caller interface to process::Subprocess

[Michael Park]

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



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


Could you explain in what situations and why the list of arguments 
could/would be modified...?



3rdparty/libprocess/include/process/subprocess.hpp (lines 62 - 67)


`= default;` works fine here.

Just a future note: `for(std::string arg : other.args)` creates a copy for 
every string, which is then copied again on `args.push_back(arg)`.

Using `std::vector`'s copy constructor would've been better, `= default;` 
would be better.

You could even just omit it entirely if you wish, in which case it's 
equivalent to `= default;`



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


`= {}` works for default construction.
`:` should be on the next line.



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


Please differentiate the name between `this->args` and `args`, and simply 
use `std::vector`'s copy constructor rather than manually looping.



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


This should be just `f.get();` right?



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


`:` on the next line.



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


This suggests to me either that `Command` should be named `Invocation`, or 
that this variable should be named `command`. Given that `Command` is storing 
both the command and the arguments, it seems more fitting to name it 
`Invocation`.

Additionally, I think it would make a lot of sense to simply name it 
`Result` and move it into the `Subprocess` class. Similar to `Subprocess::IO`, 
we would have `Subprocess::Result` which captures the result of a subprocess 
call.

What do you think?



3rdparty/libprocess/include/process/subprocess.hpp (lines 135 - 152)


Could you explain why these aren't symmetric? That is, why is `stdout_` not 
a `Option` or why is `stderr_` not a `std::string`?



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


`s/writing to the stream/writing to stderr/`

To keep consistency with the `stdout` version above.



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


Is there a reason why this is wrapped in a `std::shared_ptr`?



3rdparty/libprocess/src/subprocess.cpp (line 219)


Can we not just call `cleanup()` here?



3rdparty/libprocess/src/subprocess.cpp (line 225)


Remove newline.



3rdparty/libprocess/src/subprocess.cpp (line 254)


Indented 1 level too deep.



3rdparty/libprocess/src/subprocess.cpp (lines 499 - 503)


Rather than exposing the `invokedWith_` member variable like this, can we 
introduce a `Subprocess` constructor that takes a `Command` instead? At which 
point we can do something like:

`Subprocess process(path == "sh" ? Command(argv[2]) : Command(path, argv));`


- Michael Park


On Nov. 2, 2015, 7:22 a.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37336/
> ---
> 
> (Updated Nov. 2, 2015, 7:22 a.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 an `execute()` method
> that returns a `Future` (also introduced with this patch) 
> which
> contains useful information about the command invocation; the exit code;
> stdout and, optionally, stderr too.
> 
> Once the Future completes, if successful, the caller will be able to 
> retrieve
> stdout/stderr; whether the command was successful; and whether it 
> received a signal
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/su

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

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


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

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

[Vinod Kone]

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

Ship it!


Ship It!

- Vinod Kone


On Oct. 30, 2015, 8:54 a.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> ---
> 
> (Updated Oct. 30, 2015, 8:54 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-3468
> https://issues.apache.org/jira/browse/MESOS-3468
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   support/apply-reviews.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38883/diff/
> 
> 
> Testing
> ---
> 
> Tested the script with python 2.7.
> 
> 
> Thanks,
> 
> Artem Harutyunyan
> 
>

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

[Vinod Kone]

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



support/apply-reviews.py (line 63)


kill the period at the end of the log statement.

here and everywhere else.



support/apply-reviews.py (line 75)


ditto.

kill the period at the end of the log statement.



support/apply-reviews.py (line 120)


i would put the "."on the next line.



support/apply-reviews.py (line 133)


ditto. "." on the nextline.



support/apply-reviews.py (line 144)


dot on the next line.



support/apply-reviews.py (line 155)


".format" should be aligned with '{summary..' ?


- Vinod Kone


On Oct. 30, 2015, 8:54 a.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> ---
> 
> (Updated Oct. 30, 2015, 8:54 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-3468
> https://issues.apache.org/jira/browse/MESOS-3468
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   support/apply-reviews.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38883/diff/
> 
> 
> Testing
> ---
> 
> Tested the script with python 2.7.
> 
> 
> Thanks,
> 
> Artem Harutyunyan
> 
>

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

[Alexander Rukletsov]

> On Nov. 2, 2015, 4:11 p.m., Vinod Kone wrote:
> > What's the plan for slave to agent rename in the logs? Having just the 
> > allocator output agent when the rest of the code base outputs slave will be 
> > confusing IMO.

I think we change as we go. I was introducing new log messages and decided not 
to use a deprecated term "slave", but rather change whole allocator output for 
consistency.


- Alexander


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


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

Re: Review Request 39611: Remove binary `data` fields from state endpoints.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39611]

All tests passed.

- Mesos ReviewBot


On Nov. 2, 2015, 8:21 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39611/
> ---
> 
> (Updated Nov. 2, 2015, 8:21 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-3771
> https://issues.apache.org/jira/browse/MESOS-3771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not encode or otherwise preprocess the raw binary in the ExecutorInfo 
> or TaskInfo `data` fields before outputting them to JSON via the state 
> endpoints.  This means that the outputted JSON may be invalid (i.e. non-UTF8) 
> if `data` was filled in with arbitrary bytes.
> 
> The `data` fields are being removed because the state endpoints should not be 
> writing binary data in the first place.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp f56d8a178e1f61d91adfeaad32b47718adbc4379 
>   src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 
> 
> Diff: https://reviews.apache.org/r/39611/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Apparently, none of our tests check these fields.  
> 
> (Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
> enough to catch the invalid-JSON that results.  It will happily parse the 
> binary blobs (roundtripping successfully).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39611: Remove binary `data` fields from state endpoints.

[Guangya Liu]

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

Ship it!


I think that the upgrade documentshould also be updated if this field is 
removed.

- Guangya Liu


On 十一月 2, 2015, 8:21 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39611/
> ---
> 
> (Updated 十一月 2, 2015, 8:21 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-3771
> https://issues.apache.org/jira/browse/MESOS-3771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not encode or otherwise preprocess the raw binary in the ExecutorInfo 
> or TaskInfo `data` fields before outputting them to JSON via the state 
> endpoints.  This means that the outputted JSON may be invalid (i.e. non-UTF8) 
> if `data` was filled in with arbitrary bytes.
> 
> The `data` fields are being removed because the state endpoints should not be 
> writing binary data in the first place.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp f56d8a178e1f61d91adfeaad32b47718adbc4379 
>   src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 
> 
> Diff: https://reviews.apache.org/r/39611/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Apparently, none of our tests check these fields.  
> 
> (Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
> enough to catch the invalid-JSON that results.  It will happily parse the 
> binary blobs (roundtripping successfully).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39634: FreeBSD: Enable mesos build and start fixing some tests

[David Forsythe]

> On Nov. 2, 2015, 6:58 p.m., Alex Clemmer wrote:
> > configure.ac, lines 612-621
> > 
> >
> > If it's not too much trouble, it would be great to see this logic added 
> > also the `cmake/CompilationCOnfigure.cmake`. It should only be a couple 
> > lines of code.

Mind if that happens after/if this change lands? I'm not sure what the 
ettiquite is for filing tickets on unsupported platforms, but I have no problem 
bringing the cmake stuff up to date if this gets merged.


- David


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


On Oct. 30, 2015, 5:05 a.m., David Forsythe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39634/
> ---
> 
> (Updated Oct. 30, 2015, 5:05 a.m.)
> 
> 
> Review request for mesos and Ian Downes.
> 
> 
> Bugs: MESOS-1563
> https://issues.apache.org/jira/browse/MESOS-1563
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> FreeBSD: Enable mesos build and start fixing some tests
> 
> 
> Diffs
> -
> 
>   configure.ac 656766430e2720c6a407e282521102b54547fd2d 
>   src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db 
>   src/tests/attributes_tests.cpp 4fc0c31c3b2eb745432818c99746a097fde65df3 
>   src/tests/resources_tests.cpp 6584fc6c39e6ffe9f8085576677dcc669f127697 
>   src/tests/values_tests.cpp e9b1079bbadf05390b39bedd5ad5677f3d4ec0d8 
> 
> Diff: https://reviews.apache.org/r/39634/diff/
> 
> 
> Testing
> ---
> 
> make check on ubuntu 14.04
> 
> 
> Thanks,
> 
> David Forsythe
> 
>

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

[Joseph Wu]

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


Partial review.  (Haven't looked at the new files yet).

`stat.hpp` looks mostly fine to me.


3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 17)


Is this necessary?



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 21)


This appears to be unused.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 26)


This appears to be unused.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 144)


I think we still need an `rdev` function.  (Although it'll have to be some 
sort of no-op on Windows.)


- Joseph Wu


On Oct. 31, 2015, 6:31 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39803/
> ---
> 
> (Updated Oct. 31, 2015, 6:31 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Implemented stout/os/stat.hpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 67b142bbc2d80f40a1e893cc5813d58dd2aa8381 
>   
> 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp
>  PRE-CREATION 
>   
> 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp 
> 675b2e712358a55b3580026936890eaf80e5af71 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
> 1a7037d64afeedc340258c92067e95d1d3caa027 
> 
> Diff: https://reviews.apache.org/r/39803/diff/
> 
> 
> Testing
> ---
> 
> `make check` from autotools on Ubuntu 15.
> `make check` from CMake on OS X 10.10.
> Ran `check` project in VS on Windows 10.
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 39493: Added `yum install nss` to CentOS 6.6 install docs.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39493]

All tests passed.

- Mesos ReviewBot


On Nov. 2, 2015, 8:54 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39493/
> ---
> 
> (Updated Nov. 2, 2015, 8:54 p.m.)
> 
> 
> Review request for mesos, Adam B and haosdent huang.
> 
> 
> Bugs: MESOS-3506
> https://issues.apache.org/jira/browse/MESOS-3506
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `yum install nss` to CentOS 6.6 install docs.
> 
> 
> Diffs
> -
> 
>   docs/getting-started.md 35c8c566fff83abfcfcce177bfb9a35454f26494 
> 
> Diff: https://reviews.apache.org/r/39493/diff/
> 
> 
> Testing
> ---
> 
> Viewed in the Mesos Website Container: 
> https://github.com/mesosphere/mesos-website-container
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

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

[Jojy Varghese]

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

(Updated Nov. 2, 2015, 10:46 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

rebased.


Repository: mesos


Description
---

RegistryClient refactor: expanded abbreviated names.


Diffs (updated)
-

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

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


Testing
---

make check.


Thanks,

Jojy Varghese

Review Request 39882: Fixed unsafe hashmap.at call

[Jojy Varghese]

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

Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

Fixed unsafe hashmap.at call


Diffs
-

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

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


Testing
---


Thanks,

Jojy Varghese

Re: Review Request 39839: RegistryClient refactor: Changed getManifest interface

[Jojy Varghese]

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

(Updated Nov. 2, 2015, 10:45 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

rebased.


Repository: mesos


Description
---

Replaced path and tag parameters with Image::Name


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
ad57d8592c5f9df5115b575855ed2e99a0597359 
  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
  src/tests/containerizer/provisioner_docker_tests.cpp 
99c2af7d06dd7908e8d77e1bfb3e20622fb3eb4c 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

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

(Updated Nov. 2, 2015, 10:45 p.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

rebased.


Repository: mesos


Description
---

- Cleanup and broke big methods into smaller chunks.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
ad57d8592c5f9df5115b575855ed2e99a0597359 
  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
  src/tests/containerizer/provisioner_docker_tests.cpp 
99c2af7d06dd7908e8d77e1bfb3e20622fb3eb4c 

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


Testing
---

make check.


Thanks,

Jojy Varghese

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

[Vinod Kone]

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

(Updated Nov. 2, 2015, 10:34 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

attached the bug.


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


Repository: mesos


Description
---

The master now doesn't change the latest state of a task if it has already 
terminated. But it still updates the status update state and uuid.


Diffs
-

  src/master/master.cpp 2bc5a97a5b50c8a8a9902c47b2e9e3b5216d97ea 

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


Testing
---

make check

Ran the "RecoverCompletedExecutor" test 100 times as it was failing most of the 
time without this fix.


Thanks,

Vinod Kone

Re: Review Request 39802: Windows: Implemented `stout/os/windows/ls.hpp`.

[Joseph Wu]

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



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/ls.hpp (lines 20 - 
21)


Add `#include `?



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/ls.hpp (line 22)


I might be blind, but I don't see where this is used.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/ls.hpp (lines 66 - 
69)


This seems weird.  `readdir_r`, per your implementation, only returns 0 or 
1.  But in the error case, you set `errno` to `EBADF` within `readdir_r`.

I'm guessing you'll want to "preserve" the error, just like you did above 
for malloc.


- Joseph Wu


On Oct. 29, 2015, 11:46 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39802/
> ---
> 
> (Updated Oct. 29, 2015, 11:46 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Implemented `stout/os/windows/ls.hpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/ls.hpp 
> 5b6fba13ce215af5801fd0867f6e774e100689ca 
> 
> Diff: https://reviews.apache.org/r/39802/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

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

[Vinod Kone]

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

(Updated Nov. 2, 2015, 9:50 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

split 0.21.0 slave related changes into its own dependent patch.


Repository: mesos


Description
---

The master now doesn't change the latest state of a task if it has already 
terminated. But it still updates the status update state and uuid.


Diffs (updated)
-

  src/master/master.cpp 2bc5a97a5b50c8a8a9902c47b2e9e3b5216d97ea 

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


Testing
---

make check

Ran the "RecoverCompletedExecutor" test 100 times as it was failing most of the 
time without this fix.


Thanks,

Vinod Kone

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

[Vinod Kone]

> On Nov. 2, 2015, 8:13 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 6032-6036
> > 
> >
> > The movement around this seems to be the key part of this patch, is it 
> > possible to isolate the fix in a patch, on top of a patch where the logging 
> > / 0.21.0 bits are updated?

done.


- Vinod


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


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

Review Request 39878: Updated code and comments Master::updateTask() that deals with 0.21.0 slaves.

[Vinod Kone]

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

Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

The only functional change here is that updateTask() doesn't check for uuid 
being set to empty string.


Diffs
-

  src/master/master.cpp 2bc5a97a5b50c8a8a9902c47b2e9e3b5216d97ea 

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


Testing
---

make check


Thanks,

Vinod Kone

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

[Gilbert Song]

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

Ship it!


LGTM.

- Gilbert Song


On Nov. 2, 2015, 11:04 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39868/
> ---
> 
> (Updated Nov. 2, 2015, 11:04 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved ProvisionerProcess to header for testing.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 912fc5abadb1219fc4baec1a751010522bc7a7d0 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> e99c1c9876b836be5d3efcef7408b5ed01d8984e 
> 
> Diff: https://reviews.apache.org/r/39868/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 39325: Fixed race between coordinator election and recovery in replicated log.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39463, 39325]

All tests passed.

- Mesos ReviewBot


On Nov. 2, 2015, 7:42 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39325/
> ---
> 
> (Updated Nov. 2, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Bugs: MESOS-3280
> https://issues.apache.org/jira/browse/MESOS-3280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3280. The basic problem is that replicas silently ignore inbound Promise
> and Write requests if they have not finished the recovery protocol yet 
> (because
> they can't safely vote on such requests). Hence, if we try to do a Paxos round
> while a quorum of nodes have not finished recovering, the Paxos round will 
> never
> complete. In particular, this might happen during coordinator election:
> coordinator election (which is implemented as performing a full Paxos round)
> starts as soon as the candidate coordinator replica has finished the recovery
> protocol. If several nodes start concurrently, a quorum of those nodes might
> still be executing the recovery protocol, and hence the coordinator will never
> be elected.
> 
> To address this, add "ignored" responses to the Promise and Write 
> sub-protocols:
> if a proposer sees a quorum of "ignored" responses to a promise or write 
> request
> it has issued, it knows the request will never succeed.  When used for
> coordinator election, the current coding will retry immediately (without a
> backoff).
> 
> Note that replicas will still silently drop promise/write requests if another
> kind of problem occurs (e.g., an I/O error prevents reading/writing log
> data). We might consider changing this, although it will require some thought:
> e.g., if a replica's disk is broken, sending an "ignored" message on every
> request might flood the network.
> 
> REVIEWER NOTES:
> 
> * GMock code is ugly, but see notes below.
> * Should we add exponential backoff when retrying coordinator election?
> 
> 
> Diffs
> -
> 
>   src/log/consensus.cpp 71cd5f39c02f583c1ea53d1ab4569115b0cee2a3 
>   src/log/coordinator.cpp e1df8b01f28447955e1e8bbd764fce3e1a948d88 
>   src/log/replica.hpp 70f415f4f81465dbb7d1e4d4f65c7965cd432415 
>   src/log/replica.cpp 414e1165ae969c1f9492e7b8361c72df2c4fbc16 
>   src/messages/log.proto d73b33f865963292af580945659ad0e800f2a204 
>   src/tests/log_tests.cpp 222a12e10032082e6315ca7b43d3393738edd312 
> 
> Diff: https://reviews.apache.org/r/39325/diff/
> 
> 
> Testing
> ---
> 
> "make check" passes, including a new test that uses a newly constructed mock 
> to ensure we're testing the message schedule described above.
> 
> I also wrote a script stops and starts mesos-master in a loop, removing the 
> replicated log each time. Without the patch, this occasionally fails with a 
> "registry fetch" timeout; with the patch, you can observe several scenarios 
> where coordinator election is reborted and retried because a quorum of 
> ignored responses is seen. Note that in some cases, we need to retry 
> coordinator election up to ~70 times (!), because we don't currently use a 
> backoff. On a more realistic LAN configuration, many fewer retries will 
> likely be needed, but we could also use a backoff instead. But the important 
> point is that election eventually succeeds and we don't hang.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

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

[Jojy Varghese]

> On Nov. 2, 2015, 8:19 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 605
> > 
> >
> > Wondering if this means that we can use ContainerInfo as a predicate 
> > for "launch" inside the "composing" containerizer? Today we call "launch" 
> > on the containerizer(docker or mesos) and let the "luanch" decide if it can 
> > handle the task. Now that we have the information right away at the 
> > composing containerizer, maybe we can use that?
> 
> Timothy Chen wrote:
> we cant since we cant distinguish between mesos containerizer and 
> external containerizer

Ah. i thought external containerizer is to be discontinued.


- Jojy


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


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

Re: Review Request 39611: Remove binary `data` fields from state endpoints.

[Joseph Wu]

> On Nov. 2, 2015, 12:52 p.m., Neil Conway wrote:
> > How about backward compatibility? Adding a note to docs/upgrades.md seems a 
> > good idea, at the very least. Are we pretty confident that no one else is 
> > looking at this data, and/or we're happy to break anyone that is?
> 
> Kapil Arya wrote:
> Good point about backward compatibility. However, if the `data` field 
> contains random bytes that might result into the entire json becoming 
> "unrenderable", then it's broken anyways. Also note that we don't expose 
> `TaskInfo::data` and `TaskStatus::data` fields over state.json either. We can 
> use the dev/user mailing lists to get a feel for it.

Yeah, I emailed the dev list a while ago about this: 
http://www.mail-archive.com/dev@mesos.apache.org/msg33536.html

Note: The `TaskInfo::data` fields are part of the agent's state endpoint.


- Joseph


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


On Nov. 2, 2015, 12:21 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39611/
> ---
> 
> (Updated Nov. 2, 2015, 12:21 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-3771
> https://issues.apache.org/jira/browse/MESOS-3771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not encode or otherwise preprocess the raw binary in the ExecutorInfo 
> or TaskInfo `data` fields before outputting them to JSON via the state 
> endpoints.  This means that the outputted JSON may be invalid (i.e. non-UTF8) 
> if `data` was filled in with arbitrary bytes.
> 
> The `data` fields are being removed because the state endpoints should not be 
> writing binary data in the first place.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp f56d8a178e1f61d91adfeaad32b47718adbc4379 
>   src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 
> 
> Diff: https://reviews.apache.org/r/39611/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Apparently, none of our tests check these fields.  
> 
> (Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
> enough to catch the invalid-JSON that results.  It will happily parse the 
> binary blobs (roundtripping successfully).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39559: Windows: Implemented `os::mkdtemp`.

[Joris Van Remoortere]

> On Nov. 2, 2015, 8:38 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/mkdtemp.hpp, 
> > lines 34-71
> > 
> >
> > Can you please explain why we can't just use `mktemp` here?
> > I acknowledge that the idea behind `mktemp` is to guarantee that the 
> > directory name can't collide with an existing one, but the custom 
> > implementation you provide also doesn't do that while generating the name, 
> > correct?
> 
> Joseph Wu wrote:
> Windows doesn't have a temp-directory function.  Are you suggesting 
> calling `mktemp` and then using the file's name as the argument to `mkdir`?

Indeed. It seems like that's what we are doing manually right now?


- Joris


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


On Oct. 30, 2015, 5:53 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39559/
> ---
> 
> (Updated Oct. 30, 2015, 5:53 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Implemented `os::mkdtemp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 67b142bbc2d80f40a1e893cc5813d58dd2aa8381 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> fc2df6831ae2cb1a91c7a8cc92965939576e575d 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/mkdtemp.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/mkdtemp.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/mkdtemp.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> e26df59d9b837e1f4a4b92577f0a3de4b9076cb4 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> edf17d5ad8efbc988e909bfb8ffa5a015ecdc89d 
> 
> Diff: https://reviews.apache.org/r/39559/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 39611: Remove binary `data` fields from state endpoints.

[Kapil Arya]

> On Nov. 2, 2015, 3:52 p.m., Neil Conway wrote:
> > How about backward compatibility? Adding a note to docs/upgrades.md seems a 
> > good idea, at the very least. Are we pretty confident that no one else is 
> > looking at this data, and/or we're happy to break anyone that is?

Good point about backward compatibility. However, if the `data` field contains 
random bytes that might result into the entire json becoming "unrenderable", 
then it's broken anyways. Also note that we don't expose `TaskInfo::data` and 
`TaskStatus::data` fields over state.json either. We can use the dev/user 
mailing lists to get a feel for it.


- Kapil


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


On Nov. 2, 2015, 3:21 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39611/
> ---
> 
> (Updated Nov. 2, 2015, 3:21 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-3771
> https://issues.apache.org/jira/browse/MESOS-3771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not encode or otherwise preprocess the raw binary in the ExecutorInfo 
> or TaskInfo `data` fields before outputting them to JSON via the state 
> endpoints.  This means that the outputted JSON may be invalid (i.e. non-UTF8) 
> if `data` was filled in with arbitrary bytes.
> 
> The `data` fields are being removed because the state endpoints should not be 
> writing binary data in the first place.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp f56d8a178e1f61d91adfeaad32b47718adbc4379 
>   src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 
> 
> Diff: https://reviews.apache.org/r/39611/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Apparently, none of our tests check these fields.  
> 
> (Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
> enough to catch the invalid-JSON that results.  It will happily parse the 
> binary blobs (roundtripping successfully).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39597: Add Newbie guide.

[Neil Conway]

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



docs/NewbieQuickStart.md (line 61)


Can we mention 3rdparty/libprocess/README.md and 
3rdparty/libprocess/3rdparty/stout/README.md?



docs/NewbieQuickStart.md (line 65)


Re: "detailed steps", can we link to the appropriate part of the docs? 
(e.g., getting started)



docs/NewbieQuickStart.md (line 69)


Rather than saying "A link to...", seems simpler to just include a link to 
JIRA and remove this text.



docs/NewbieQuickStart.md (line 86)


Links to other docs pages should take the form "(anchor-text)[foo.md]", 
rather than using the full URL.



docs/NewbieQuickStart.md (line 99)


See above re: link. Same changes also needed below in a few places.



docs/NewbieQuickStart.md (line 118)


This could be cleaned up: no need to mention JIRA URL twice.


- Neil Conway


On Oct. 30, 2015, 9 p.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39597/
> ---
> 
> (Updated Oct. 30, 2015, 9 p.m.)
> 
> 
> Review request for mesos, Timothy Chen and Vinod Kone.
> 
> 
> Bugs: MESOS-3224
> https://issues.apache.org/jira/browse/MESOS-3224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Newbie guide.
> 
> 
> Diffs
> -
> 
>   docs/NewbieQuickStart.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39597/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Re: Review Request 39493: Added `yum install nss` to CentOS 6.6 install docs.

[Greg Mann]

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

(Updated Nov. 2, 2015, 8:54 p.m.)


Review request for mesos, Adam B and haosdent huang.


Changes
---

Updated comment in docs.


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


Repository: mesos


Description
---

Added `yum install nss` to CentOS 6.6 install docs.


Diffs (updated)
-

  docs/getting-started.md 35c8c566fff83abfcfcce177bfb9a35454f26494 

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


Testing
---

Viewed in the Mesos Website Container: 
https://github.com/mesosphere/mesos-website-container


Thanks,

Greg Mann

Re: Review Request 39611: Remove binary `data` fields from state endpoints.

[Neil Conway]

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


How about backward compatibility? Adding a note to docs/upgrades.md seems a 
good idea, at the very least. Are we pretty confident that no one else is 
looking at this data, and/or we're happy to break anyone that is?

- Neil Conway


On Nov. 2, 2015, 8:21 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39611/
> ---
> 
> (Updated Nov. 2, 2015, 8:21 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-3771
> https://issues.apache.org/jira/browse/MESOS-3771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not encode or otherwise preprocess the raw binary in the ExecutorInfo 
> or TaskInfo `data` fields before outputting them to JSON via the state 
> endpoints.  This means that the outputted JSON may be invalid (i.e. non-UTF8) 
> if `data` was filled in with arbitrary bytes.
> 
> The `data` fields are being removed because the state endpoints should not be 
> writing binary data in the first place.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp f56d8a178e1f61d91adfeaad32b47718adbc4379 
>   src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 
> 
> Diff: https://reviews.apache.org/r/39611/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Apparently, none of our tests check these fields.  
> 
> (Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
> enough to catch the invalid-JSON that results.  It will happily parse the 
> binary blobs (roundtripping successfully).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39559: Windows: Implemented `os::mkdtemp`.

[Joseph Wu]

> On Nov. 2, 2015, 12:38 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/mkdtemp.hpp, 
> > lines 34-71
> > 
> >
> > Can you please explain why we can't just use `mktemp` here?
> > I acknowledge that the idea behind `mktemp` is to guarantee that the 
> > directory name can't collide with an existing one, but the custom 
> > implementation you provide also doesn't do that while generating the name, 
> > correct?

Windows doesn't have a temp-directory function.  Are you suggesting calling 
`mktemp` and then using the file's name as the argument to `mkdir`?


- Joseph


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


On Oct. 29, 2015, 10:53 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39559/
> ---
> 
> (Updated Oct. 29, 2015, 10:53 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Implemented `os::mkdtemp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 67b142bbc2d80f40a1e893cc5813d58dd2aa8381 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> fc2df6831ae2cb1a91c7a8cc92965939576e575d 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/mkdtemp.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/mkdtemp.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/mkdtemp.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> e26df59d9b837e1f4a4b92577f0a3de4b9076cb4 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> edf17d5ad8efbc988e909bfb8ffa5a015ecdc89d 
> 
> Diff: https://reviews.apache.org/r/39559/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

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

[Timothy Chen]

> On Nov. 2, 2015, 8:19 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 605
> > 
> >
> > Wondering if this means that we can use ContainerInfo as a predicate 
> > for "launch" inside the "composing" containerizer? Today we call "launch" 
> > on the containerizer(docker or mesos) and let the "luanch" decide if it can 
> > handle the task. Now that we have the information right away at the 
> > composing containerizer, maybe we can use that?

we cant since we cant distinguish between mesos containerizer and external 
containerizer


- Timothy


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


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

Re: Review Request 39019: Windows: Added dirent compat code for non-Unix systems.

[Joseph Wu]

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


Where's the `Makefile` change?


3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/dirent.hpp 
(line 43)


Presumably, this will always be true for Stout.  So do we need the `#ifdef`?



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/dirent.hpp 
(lines 50 - 52)


Maybe add a `// Forward declarations.` comment here.


- Joseph Wu


On Oct. 29, 2015, 10:59 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39019/
> ---
> 
> (Updated Oct. 29, 2015, 10:59 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Bugs: MESOS-3441
> https://issues.apache.org/jira/browse/MESOS-3441
> 
> 
> 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 
> 1a7037d64afeedc340258c92067e95d1d3caa027 
> 
> Diff: https://reviews.apache.org/r/39019/diff/
> 
> 
> Testing
> ---
> 
> `make check` from autotools on Ubuntu 15.
> `make check` from CMake on OS X 10.10.
> Ran `check` project in VS on Windows 10.
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 39559: Windows: Implemented `os::mkdtemp`.

[Joris Van Remoortere]

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



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


``?



3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/mkdtemp.hpp (line 18)


``
``



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/mkdtemp.hpp (lines 
19 - 21)


order?



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/mkdtemp.hpp (line 
34)


Let's try to be consistent about capitalizing after `NOTE:`



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/mkdtemp.hpp (lines 
34 - 71)


Can you please explain why we can't just use `mktemp` here?
I acknowledge that the idea behind `mktemp` is to guarantee that the 
directory name can't collide with an existing one, but the custom 
implementation you provide also doesn't do that while generating the name, 
correct?



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/mkdtemp.hpp (line 
52)


Let's try to be consistent about capitalizing after `NOTE:`


- Joris Van Remoortere


On Oct. 30, 2015, 5:53 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39559/
> ---
> 
> (Updated Oct. 30, 2015, 5:53 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Implemented `os::mkdtemp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 67b142bbc2d80f40a1e893cc5813d58dd2aa8381 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> fc2df6831ae2cb1a91c7a8cc92965939576e575d 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/mkdtemp.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/mkdtemp.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/mkdtemp.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> e26df59d9b837e1f4a4b92577f0a3de4b9076cb4 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> edf17d5ad8efbc988e909bfb8ffa5a015ecdc89d 
> 
> Diff: https://reviews.apache.org/r/39559/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Review Request 39611: Remove binary `data` fields from state endpoints.

[Joseph Wu]

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

Review request for mesos, Ben Mahler and Artem Harutyunyan.


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


Repository: mesos


Description
---

We do not encode or otherwise preprocess the raw binary in the ExecutorInfo or 
TaskInfo `data` fields before outputting them to JSON via the state endpoints.  
This means that the outputted JSON may be invalid (i.e. non-UTF8) if `data` was 
filled in with arbitrary bytes.

The `data` fields are being removed because the state endpoints should not be 
writing binary data in the first place.


Diffs
-

  src/common/http.cpp f56d8a178e1f61d91adfeaad32b47718adbc4379 
  src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 

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


Testing
---

`make check`

Apparently, none of our tests check these fields.  

(Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
enough to catch the invalid-JSON that results.  It will happily parse the 
binary blobs (roundtripping successfully).


Thanks,

Joseph Wu

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

[Jojy Varghese]

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



src/slave/containerizer/mesos/containerizer.cpp (line 578)


Wondering if this means that we can use ContainerInfo as a predicate for 
"launch" inside the "composing" containerizer? Today we call "launch" on the 
containerizer(docker or mesos) and let the "luanch" decide if it can handle the 
task. Now that we have the information right away at the composing 
containerizer, maybe we can use that?


- Jojy Varghese


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

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

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [39719, 39866, 38900, 39868, 39869, 39258]

Failed command: make -j3 distcheck

Error:
 make  dist-gzip am__post_remove_distdir='@:'
make[1]: Entering directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot'
if test -d "mesos-0.26.0"; then find "mesos-0.26.0" -type d ! -perm -200 -exec 
chmod u+w {} ';' && rm -rf "mesos-0.26.0" || { sleep 5 && rm -rf 
"mesos-0.26.0"; }; else :; fi
test -d "mesos-0.26.0" || mkdir "mesos-0.26.0"
 (cd 3rdparty && make  top_distdir=../mesos-0.26.0 
distdir=../mesos-0.26.0/3rdparty \
 am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[2]: Entering directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty'
 (cd libprocess && make  top_distdir=../../mesos-0.26.0 
distdir=../../mesos-0.26.0/3rdparty/libprocess \
 am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[3]: Entering directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess'
:
test -d "../../mesos-0.26.0/3rdparty/libprocess" || mkdir 
"../../mesos-0.26.0/3rdparty/libprocess"
 (cd 3rdparty && make  top_distdir=../../../mesos-0.26.0 
distdir=../../../mesos-0.26.0/3rdparty/libprocess/3rdparty \
 am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[4]: Entering directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty'
 (cd stout && make  top_distdir=../../../../mesos-0.26.0 
distdir=../../../../mesos-0.26.0/3rdparty/libprocess/3rdparty/stout \
 am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[5]: Entering directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout'
:
test -d "../../../../mesos-0.26.0/3rdparty/libprocess/3rdparty/stout" || mkdir 
"../../../../mesos-0.26.0/3rdparty/libprocess/3rdparty/stout"
 (cd include && make  top_distdir=../../../../../mesos-0.26.0 
distdir=../../../../../mesos-0.26.0/3rdparty/libprocess/3rdparty/stout/include \
 am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[6]: Entering directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/include'
make[6]: Leaving directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/include'
test -n ":" \
|| find "../../../../mesos-0.26.0/3rdparty/libprocess/3rdparty/stout" 
-type d ! -perm -755 \
-exec chmod u+rwx,go+rx {} \; -o \
  ! -type d ! -perm -444 -links 1 -exec chmod a+r {} \; -o \
  ! -type d ! -perm -400 -exec chmod a+r {} \; -o \
  ! -type d ! -perm -444 -exec /bin/bash 
/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/install-sh
 -c -m a+r {} {} \; \
|| chmod -R a+r 
"../../../../mesos-0.26.0/3rdparty/libprocess/3rdparty/stout"
make[5]: Leaving directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout'
make[4]: Leaving directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty'
 (cd include && make  top_distdir=../../../mesos-0.26.0 
distdir=../../../mesos-0.26.0/3rdparty/libprocess/include \
 am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[4]: Entering directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/include'
make[4]: Leaving directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/include'
test -n ":" \
|| find "../../mesos-0.26.0/3rdparty/libprocess" -type d ! -perm -755 \
-exec chmod u+rwx,go+rx {} \; -o \
  ! -type d ! -perm -444 -links 1 -exec chmod a+r {} \; -o \
  ! -type d ! -perm -400 -exec chmod a+r {} \; -o \
  ! -type d ! -perm -444 -exec /bin/bash 
/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/install-sh
 -c -m a+r {} {} \; \
|| chmod -R a+r "../../mesos-0.26.0/3rdparty/libprocess"
make[3]: Leaving directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess'
make[2]: Leaving directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty'
 (cd src && make  top_distdir=../mesos-0.26.0 distdir=../mesos-0.26.0/src \
 am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[2]: Entering directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/src'
make[2]: *** No rule to make target `tests/containerizer/provisioner.hpp', 
needed by `distdir'.  Stop.
make[2]: Leaving directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/src'
make[1]: *** [distdir] Error 1
make[1]: Leaving

Re: Review Request 39502: [DO NOT COMMIT] Sync v1/mesos.proto with docker, QoS, and AppC changes.

[Kapil Arya]

> On Oct. 21, 2015, 3:31 p.m., Kapil Arya wrote:
> > Ship It!
> 
> Joseph Wu wrote:
> Just to confirm, https://reviews.apache.org/r/38367/diff/4#0 was omitted 
> from V1 unintentionally?

Yes, that was an oversight on my end. Thanks for fixing this, Joseph!!


- Kapil


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


On Oct. 26, 2015, 5:27 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39502/
> ---
> 
> (Updated Oct. 26, 2015, 5:27 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, Joris Van Remoortere, 
> Kapil Arya, Niklas Nielsen, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added in reviews:
> 
> * https://reviews.apache.org/r/37196/
> * https://reviews.apache.org/r/37308/ <- Confirmed as unintentional
> * https://reviews.apache.org/r/38253/
> * https://reviews.apache.org/r/38367/
> ** NetworkInfo moved up the review chain.
> * 
> https://github.com/apache/mesos/commit/140311f263a6ae54d3d211c9c91e4bf55d2eb0f1
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/mesos.proto 8131778fe5c5f3a47ae9300a811e3d857a22da6a 
> 
> Diff: https://reviews.apache.org/r/39502/diff/
> 
> 
> Testing
> ---
> 
> `make`
> 
> For now, this review is meant for checking which of these differences are 
> intentional.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

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

[Ben Mahler]

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



src/master/master.cpp (lines 6024 - 6028)


The movement around this seems to be the key part of this patch, is it 
possible to isolate the fix in a patch, on top of a patch where the logging / 
0.21.0 bits are updated?


- Ben Mahler


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

Re: Review Request 39851: Windows: fixed ambiguousity error in `process/owned.hpp`.

[Alex Clemmer]

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

(Updated Nov. 2, 2015, 7:56 p.m.)


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


Repository: mesos


Description
---

Windows: fixed ambiguousity error in `process/owned.hpp`.


Diffs (updated)
-

  3rdparty/libprocess/include/process/owned.hpp 
1b41477d555ce567c58033f8993dc2ae0ac80a05 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 39493: Added `yum install nss` to CentOS 6.6 install docs.

[Greg Mann]

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

(Updated Nov. 2, 2015, 7:52 p.m.)


Review request for mesos, Adam B and haosdent huang.


Changes
---

Addressed comment, changed to `yum install nss`.


Summary (updated)
-

Added `yum install nss` to CentOS 6.6 install docs.


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


Repository: mesos


Description (updated)
---

Added `yum install nss` to CentOS 6.6 install docs.


Diffs (updated)
-

  docs/getting-started.md 35c8c566fff83abfcfcce177bfb9a35454f26494 

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


Testing
---

Viewed in the Mesos Website Container: 
https://github.com/mesosphere/mesos-website-container


Thanks,

Greg Mann

Re: Review Request 39493: Added `yum install nss` to CentOS 6.6 install docs.

[Greg Mann]

> On Oct. 24, 2015, 5:12 a.m., haosdent huang wrote:
> > docs/getting-started.md, line 61
> > 
> >
> > How about add nss through install. Because install also would update if 
> > the package installed.

Thanks haosdent, this makes the instructions a bit more concise. I just 
confirmed that it works.


- Greg


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


On Nov. 2, 2015, 7:52 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39493/
> ---
> 
> (Updated Nov. 2, 2015, 7:52 p.m.)
> 
> 
> Review request for mesos, Adam B and haosdent huang.
> 
> 
> Bugs: MESOS-3506
> https://issues.apache.org/jira/browse/MESOS-3506
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `yum install nss` to CentOS 6.6 install docs.
> 
> 
> Diffs
> -
> 
>   docs/getting-started.md 35c8c566fff83abfcfcce177bfb9a35454f26494 
> 
> Diff: https://reviews.apache.org/r/39493/diff/
> 
> 
> Testing
> ---
> 
> Viewed in the Mesos Website Container: 
> https://github.com/mesosphere/mesos-website-container
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39623: CMake: Added protobuf compilation to Windows builds.

[Joseph Wu]

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

Ship it!


Double-checked non-Windows builds.

- Joseph Wu


On Oct. 29, 2015, 10:58 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39623/
> ---
> 
> (Updated Oct. 29, 2015, 10:58 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake: Added protobuf compilation to Windows builds.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289 
> 
> Diff: https://reviews.apache.org/r/39623/diff/
> 
> 
> Testing
> ---
> 
> `make check` from CMake on OS X 10.10.
> `make check` from Autotools on OS X 10.10.
> Ran `check` project in VS on Windows 10.
> 
> PLEASE NOTE: I am on a terrible network with proxy problems and I can't SSH 
> into my Ubuntu box to test this from Ubuntu
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 39622: CMake: Pointed Stout test linker flags at correct gtest directory.

[Joseph Wu]

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

Ship it!


Ship It!

- Joseph Wu


On Oct. 29, 2015, 10:57 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39622/
> ---
> 
> (Updated Oct. 29, 2015, 10:57 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake: Pointed Stout test linker flags at correct gtest directory.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/cmake/StoutTestsConfigure.cmake 
> 9893d741cd7c611dc65eba76be03e06dac618132 
> 
> Diff: https://reviews.apache.org/r/39622/diff/
> 
> 
> Testing
> ---
> 
> `make check` from CMake on OS X 10.10.
> `make check` from Autotools on OS X 10.10.
> Ran `check` project in VS on Windows 10.
> 
> PLEASE NOTE: I am on a terrible network with proxy problems and I can't SSH 
> into my Ubuntu box to test this from Ubuntu
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 39621: CMake: Corrected linking path for gmock libraries on Windows builds.

[Joseph Wu]

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

Ship it!


Ship It!

- Joseph Wu


On Oct. 29, 2015, 10:57 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39621/
> ---
> 
> (Updated Oct. 29, 2015, 10:57 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake: Corrected linking path for gmock libraries on Windows builds.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 
> 6eda46b0e08829b269b13a78f22510e6d03940d9 
> 
> Diff: https://reviews.apache.org/r/39621/diff/
> 
> 
> Testing
> ---
> 
> `make check` from CMake on OS X 10.10.
> `make check` from Autotools on OS X 10.10.
> Ran `check` project in VS on Windows 10.
> 
> PLEASE NOTE: I am on a terrible network with proxy problems and I can't SSH 
> into my Ubuntu box to test this from Ubuntu
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 39584: Windows: Implemented `os::rmdir.hpp`.

[Joseph Wu]

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

Ship it!


Double-checked non-Windows builds.

- Joseph Wu


On Oct. 29, 2015, 11:11 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39584/
> ---
> 
> (Updated Oct. 29, 2015, 11:11 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Implemented `os::rmdir.hpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 67b142bbc2d80f40a1e893cc5813d58dd2aa8381 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> fc2df6831ae2cb1a91c7a8cc92965939576e575d 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/rmdir.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> e26df59d9b837e1f4a4b92577f0a3de4b9076cb4 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> edf17d5ad8efbc988e909bfb8ffa5a015ecdc89d 
> 
> Diff: https://reviews.apache.org/r/39584/diff/
> 
> 
> Testing
> ---
> 
> `make check` from autotools on Ubuntu 15.
> `make check` from CMake on OS X 10.10.
> Ran `check` project in VS on Windows 10.
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 39325: Fixed race between coordinator election and recovery in replicated log.

[Neil Conway]

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

(Updated Nov. 2, 2015, 7:42 p.m.)


Review request for mesos, Jie Yu, Joris Van Remoortere, and Timothy Chen.


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


Repository: mesos


Description
---

MESOS-3280. The basic problem is that replicas silently ignore inbound Promise
and Write requests if they have not finished the recovery protocol yet (because
they can't safely vote on such requests). Hence, if we try to do a Paxos round
while a quorum of nodes have not finished recovering, the Paxos round will never
complete. In particular, this might happen during coordinator election:
coordinator election (which is implemented as performing a full Paxos round)
starts as soon as the candidate coordinator replica has finished the recovery
protocol. If several nodes start concurrently, a quorum of those nodes might
still be executing the recovery protocol, and hence the coordinator will never
be elected.

To address this, add "ignored" responses to the Promise and Write sub-protocols:
if a proposer sees a quorum of "ignored" responses to a promise or write request
it has issued, it knows the request will never succeed.  When used for
coordinator election, the current coding will retry immediately (without a
backoff).

Note that replicas will still silently drop promise/write requests if another
kind of problem occurs (e.g., an I/O error prevents reading/writing log
data). We might consider changing this, although it will require some thought:
e.g., if a replica's disk is broken, sending an "ignored" message on every
request might flood the network.

REVIEWER NOTES:

* GMock code is ugly, but see notes below.
* Should we add exponential backoff when retrying coordinator election?


Diffs
-

  src/log/consensus.cpp 71cd5f39c02f583c1ea53d1ab4569115b0cee2a3 
  src/log/coordinator.cpp e1df8b01f28447955e1e8bbd764fce3e1a948d88 
  src/log/replica.hpp 70f415f4f81465dbb7d1e4d4f65c7965cd432415 
  src/log/replica.cpp 414e1165ae969c1f9492e7b8361c72df2c4fbc16 
  src/messages/log.proto d73b33f865963292af580945659ad0e800f2a204 
  src/tests/log_tests.cpp 222a12e10032082e6315ca7b43d3393738edd312 

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


Testing (updated)
---

"make check" passes, including a new test that uses a newly constructed mock to 
ensure we're testing the message schedule described above.

I also wrote a script stops and starts mesos-master in a loop, removing the 
replicated log each time. Without the patch, this occasionally fails with a 
"registry fetch" timeout; with the patch, you can observe several scenarios 
where coordinator election is reborted and retried because a quorum of ignored 
responses is seen. Note that in some cases, we need to retry coordinator 
election up to ~70 times (!), because we don't currently use a backoff. On a 
more realistic LAN configuration, many fewer retries will likely be needed, but 
we could also use a backoff instead. But the important point is that election 
eventually succeeds and we don't hang.


Thanks,

Neil Conway

Re: Review Request 39583: Windows: Added `WindowsError` to parallel `ErrnoError`.

[Joseph Wu]

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

Ship it!


Double-checked build on non-Windows.

- Joseph Wu


On Oct. 29, 2015, 10:55 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39583/
> ---
> 
> (Updated Oct. 29, 2015, 10:55 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Added `WindowsError` to parallel `ErrnoError`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 67b142bbc2d80f40a1e893cc5813d58dd2aa8381 
>   3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp 
> fdd33512c8d8752093f72f597a7d647eb5e3c285 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/error.hpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39583/diff/
> 
> 
> Testing
> ---
> 
> `make check` from autotools on Ubuntu 15.
> `make check` from CMake on OS X 10.10.
> Ran `check` project in VS on Windows 10.
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

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

[Vinod Kone]

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

Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

The master now doesn't change the latest state of a task if it has already 
terminated. But it still updates the status update state and uuid.


Diffs
-

  src/master/master.cpp 9f4586e668a2141f4937497d42853fbdea7751a5 

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


Testing
---

make check

Ran the "RecoverCompletedExecutor" test 100 times as it was failing most of the 
time without this fix.


Thanks,

Vinod Kone

Re: Review Request 39358: Network monitoring metrics table has common style

[Joris Van Remoortere]

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

Ship it!


Ship It!

- Joris Van Remoortere


On Oct. 29, 2015, 10:48 a.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39358/
> ---
> 
> (Updated Oct. 29, 2015, 10:48 a.m.)
> 
> 
> Review request for mesos, Dave Lester, Jie Yu, and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Network monitoring metrics table has common style
> 
> 
> Diffs
> -
> 
>   docs/network-monitoring.md b266ae5 
> 
> Diff: https://reviews.apache.org/r/39358/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 39827: Made slave version required in master.cpp.

[Vinod Kone]

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

(Updated Nov. 2, 2015, 7:34 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

fixed constness. NNFR.


Repository: mesos


Description
---

Slave version is being set by the slave since 0.21.0. Cleaned up the code in 
master that expected it to be optional.


Diffs (updated)
-

  src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac 
  src/master/master.cpp 9f4586e668a2141f4937497d42853fbdea7751a5 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 39862: Add documentation for newer metrics

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39862]

All tests passed.

- Mesos ReviewBot


On Nov. 2, 2015, 6:19 p.m., Bhuvan Arumugam wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39862/
> ---
> 
> (Updated Nov. 2, 2015, 6:19 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, and Vinod Kone.
> 
> 
> Bugs: MESOS-3569
> https://issues.apache.org/jira/browse/MESOS-3569
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Add notes for newer metrics
>   - Fix slave port number
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 22e2fa8e188b8e367fb67f67ce4b9f37ec285841 
> 
> Diff: https://reviews.apache.org/r/39862/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bhuvan Arumugam
> 
>

Re: Review Request 39792: Updated master and slave to properly set task status uuid.

[Vinod Kone]

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

(Updated Nov. 2, 2015, 7:32 p.m.)


Review request for mesos, Anand Mazumdar and Ben Mahler.


Changes
---

updated comments. still needs a ship it.


Repository: mesos


Description
---

This just makes sure master and slave properly set the uuid in task status to 
setup the stage for deprecating the messy logic in scheduler driver in a future 
release.


Diffs (updated)
-

  src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac 
  src/master/master.cpp 9f4586e668a2141f4937497d42853fbdea7751a5 
  src/sched/sched.cpp 9c5e3b8e42605f4647d897ba02ea3a17e494f355 
  src/slave/slave.cpp 5f9b52b41eaab0c24965f28e192074340e00bde5 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 39792: Updated master and slave to properly set task status uuid.

[Vinod Kone]

> On Oct. 30, 2015, 9:43 p.m., Ben Mahler wrote:
> > Hm.. we're still relying on the update uuid, shouldn't we be trying to move 
> > off of it onto the status uuid?

As mentioned in the comments, we can't yet remove uuid because of old 
checkpointed updates :(


> On Oct. 30, 2015, 9:43 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 4365
> > 
> >
> > Just a side note, it would be great to have a status update benchmark 
> > for throughput, since we're introducing an extra copy of the status update 
> > here (which might be expensive for large updates). Ideally libprocess could 
> > move construct this 'update' field (but it doesn't support this currently).

agreed. added a TODO for now. will hopefully follow up on a review soon with a 
benchmark test.


> On Oct. 30, 2015, 9:43 p.m., Ben Mahler wrote:
> > src/sched/sched.cpp, line 903
> > 
> >
> > Mind adding a newline to separate the TODO from the rest of the 
> > comment? I find that clearer to read, especially when the TODO is at the 
> > top and it becomes ambiguous where a multi-line TODO ends and the comment 
> > starts.

done.


> On Oct. 30, 2015, 9:43 p.m., Ben Mahler wrote:
> > src/sched/sched.cpp, lines 903-904
> > 
> >
> > Hm.. why wasn't it enough that the slave was setting it? I'm guessing 
> > the concern was due to the old checkpointed updates per your change below? 
> > Seems helpful to spell out the slave side of this in the comment.

removed the mention of slave because master is the only one that sends updates 
the driver.


> On Oct. 30, 2015, 9:43 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, lines 3025-3027
> > 
> >
> > Hm.. could you reduce jaggedness here? I like how you formatted your 
> > comment in master.cpp above, easy on my eyes.

done.

as an aside, there should be a way to automate the jaggedness, otherwise it is 
tedious to get it right. the master.cpp jagedness was coincidental, i was just 
wrapping them up at 80.


> On Oct. 30, 2015, 9:43 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, lines 3028-3029
> > 
> >
> > Hm.. not immediately obvious to me why we set the status uuid from the 
> > update uuid, should we spell that out here?

expanded the comment.


- Vinod


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


On Oct. 30, 2015, 12:23 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39792/
> ---
> 
> (Updated Oct. 30, 2015, 12:23 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This just makes sure master and slave properly set the uuid in task status to 
> setup the stage for deprecating the messy logic in scheduler driver in a 
> future release.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac 
>   src/master/master.cpp 9f4586e668a2141f4937497d42853fbdea7751a5 
>   src/sched/sched.cpp 9c5e3b8e42605f4647d897ba02ea3a17e494f355 
>   src/slave/slave.cpp 5f9b52b41eaab0c24965f28e192074340e00bde5 
> 
> Diff: https://reviews.apache.org/r/39792/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 39791: Updated createStatusUpdate() to unset StatusUpdate.uuid instead of setting it to an empty string.

[Vinod Kone]

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

(Updated Nov. 2, 2015, 7:31 p.m.)


Review request for mesos, Anand Mazumdar and Ben Mahler.


Changes
---

adjusted jaggedness of the comment. NNFR.


Repository: mesos


Description
---

As the deleted comment says, we were setting it to emtpy because the 'uuid' was 
a required field until 0.22.x. Since we are at 0.26 now, it should be safe to 
just unset it.


Diffs (updated)
-

  src/common/protobuf_utils.cpp 1e795dcc90b2225ad7b0a124dbdcdabdc9d2e6aa 
  src/messages/messages.proto 9a1564ff38fa1b984e92cb0abfde2108385f9e2a 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 39560: CMake: Add state.cpp, flags.cpp to Windows agent build.

[Joseph Wu]

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

Ship it!


Checked that this doesn't break the non-Windows CMake build.

- Joseph Wu


On Oct. 29, 2015, 10:54 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39560/
> ---
> 
> (Updated Oct. 29, 2015, 10:54 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake: Add state.cpp, flags.cpp to Windows agent build.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289 
> 
> Diff: https://reviews.apache.org/r/39560/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 39559: Windows: Implemented `os::mkdtemp`.

[Joseph Wu]

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

Ship it!


LGTM.  

Note: I didn't actually run the code on Windows.


3rdparty/libprocess/3rdparty/stout/include/Makefile.am (line 84)


Missing tab for alignment.


- Joseph Wu


On Oct. 29, 2015, 10:53 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39559/
> ---
> 
> (Updated Oct. 29, 2015, 10:53 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Implemented `os::mkdtemp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 67b142bbc2d80f40a1e893cc5813d58dd2aa8381 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> fc2df6831ae2cb1a91c7a8cc92965939576e575d 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/mkdtemp.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/mkdtemp.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/mkdtemp.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> e26df59d9b837e1f4a4b92577f0a3de4b9076cb4 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> edf17d5ad8efbc988e909bfb8ffa5a015ecdc89d 
> 
> Diff: https://reviews.apache.org/r/39559/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 39559: Windows: Implemented `os::mkdtemp`.

[Joseph Wu]

> On Oct. 23, 2015, 11:45 a.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/mkdtemp.hpp, 
> > line 34
> > 
> >
> > `strlen()` might be better/more-readable.
> 
> Alex Clemmer wrote:
> So, I could definitely be mistaken, but it looks like `strlen` would have 
> to be recomputed every time we call the function, right? And it looks like 
> this code is normally run at initialization time?
> 
> If so it seems like this way has a clear benefit over the `strlen` 
> approach. What do you think? Maybe there is another way to make `strlen` an 
> init-time expression? (I already looked at `constexpr`.)
> 
> Alex Clemmer wrote:
> Post script: this exchange actually convinced me that we should compute 
> the size of `alphabet` statically to! Let me know if you have a problem with 
> this given the constraints I mentioned. :)

After taking a second pass at your code, I'd say `strlen` isn't really more 
readable.  Dropping.


- Joseph


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


On Oct. 29, 2015, 10:53 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39559/
> ---
> 
> (Updated Oct. 29, 2015, 10:53 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Implemented `os::mkdtemp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 67b142bbc2d80f40a1e893cc5813d58dd2aa8381 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> fc2df6831ae2cb1a91c7a8cc92965939576e575d 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/mkdtemp.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/mkdtemp.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/mkdtemp.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> e26df59d9b837e1f4a4b92577f0a3de4b9076cb4 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> edf17d5ad8efbc988e909bfb8ffa5a015ecdc89d 
> 
> Diff: https://reviews.apache.org/r/39559/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

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

[Alex Clemmer]

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



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


`contain a bug that might where` seems to be missing a word?



3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp (lines 1045 - 1076)


Can we please move this to its own tests file so that we can run it on 
Windows too? `os_test.cpp` has is too big and full of unimplemented stuff to 
add Windows support right now.


- Alex Clemmer


On Oct. 23, 2015, 7:49 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39005/
> ---
> 
> (Updated Oct. 23, 2015, 7:49 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
> ---
> 
> This adds a thread-safe wrapper around strerror_r which has semantics similar 
> to strerror. We plan to use this at call sites currently relying on strerror.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> ba2836a72ceee948cb43364e80ada9f132f33d04 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/strerror.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
> 5a1da57f7e27cf8154f0d5f6efd47dcee8a430ff 
> 
> Diff: https://reviews.apache.org/r/39005/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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

[Timothy Chen]

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

(Updated Nov. 2, 2015, 7:05 p.m.)


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


Repository: mesos


Description
---

Add filesystem isolator with command executor test.


Diffs (updated)
-

  src/tests/containerizer/filesystem_isolator_tests.cpp 
39008f620183d242407fea5377bfceffc57b 
  src/tests/containerizer/provisioner.hpp 
507e1413470ef4f36ead657203073115d5324bef 

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


Testing
---

make check


Thanks,

Timothy Chen

Review Request 39869: Added provisioner TestStore.

[Timothy Chen]

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

Review request for mesos, Jie Yu and Jojy Varghese.


Repository: mesos


Description
---

Added provisioner TestStore.


Diffs
-

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

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


Testing
---

make check


Thanks,

Timothy Chen

Review Request 39868: Moved ProvisionerProcess to header for testing.

[Timothy Chen]

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

Review request for mesos, Jie Yu and Jojy Varghese.


Repository: mesos


Description
---

Moved ProvisionerProcess to header for testing.


Diffs
-

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

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


Testing
---

make check


Thanks,

Timothy Chen

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

[Timothy Chen]

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

(Updated Nov. 2, 2015, 7:02 p.m.)


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


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


Repository: mesos


Description
---

Update command executor to support rootfs.


Diffs (updated)
-

  src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
  src/slave/constants.hpp de6b58a93346c618a9214032d891c1004203ca56 
  src/slave/constants.cpp b69471b2d57aad0c254ef3bb7dce9405abeab93a 
  src/slave/slave.cpp 5f9b52b41eaab0c24965f28e192074340e00bde5 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 39597: Add Newbie guide.

[Timothy Chen]

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


Thanks for doing this Diana! Sorry for the delay, we need to figure out a place 
to put this on the website as well. You have suggestions?

- Timothy Chen


On Oct. 30, 2015, 9 p.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39597/
> ---
> 
> (Updated Oct. 30, 2015, 9 p.m.)
> 
> 
> Review request for mesos, Timothy Chen and Vinod Kone.
> 
> 
> Bugs: MESOS-3224
> https://issues.apache.org/jira/browse/MESOS-3224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Newbie guide.
> 
> 
> Diffs
> -
> 
>   docs/NewbieQuickStart.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39597/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

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

[Alex Clemmer]

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



src/Makefile.am (line 735)


It looks like it might make sense to add this to `src/CMakeLists.txt` also?


- Alex Clemmer


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

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

[Timothy Chen]

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

Review request for mesos, Jie Yu and Jojy Varghese.


Repository: mesos


Description
---

Added containerInfo support for tasks in mesos containerizer.


Diffs
-

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

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 39634: FreeBSD: Enable mesos build and start fixing some tests

[Alex Clemmer]

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



configure.ac (lines 612 - 621)


If it's not too much trouble, it would be great to see this logic added 
also the `cmake/CompilationCOnfigure.cmake`. It should only be a couple lines 
of code.


- Alex Clemmer


On Oct. 30, 2015, 5:05 a.m., David Forsythe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39634/
> ---
> 
> (Updated Oct. 30, 2015, 5:05 a.m.)
> 
> 
> Review request for mesos and Ian Downes.
> 
> 
> Bugs: MESOS-1563
> https://issues.apache.org/jira/browse/MESOS-1563
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> FreeBSD: Enable mesos build and start fixing some tests
> 
> 
> Diffs
> -
> 
>   configure.ac 656766430e2720c6a407e282521102b54547fd2d 
>   src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db 
>   src/tests/attributes_tests.cpp 4fc0c31c3b2eb745432818c99746a097fde65df3 
>   src/tests/resources_tests.cpp 6584fc6c39e6ffe9f8085576677dcc669f127697 
>   src/tests/values_tests.cpp e9b1079bbadf05390b39bedd5ad5677f3d4ec0d8 
> 
> Diff: https://reviews.apache.org/r/39634/diff/
> 
> 
> Testing
> ---
> 
> make check on ubuntu 14.04
> 
> 
> Thanks,
> 
> David Forsythe
> 
>

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

[Timothy Chen]

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

(Updated Nov. 2, 2015, 6:52 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

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


Diffs (updated)
-

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

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 39230: Added prevention of SASL deprecation warnings all around its invocations on OS X.

[Alex Clemmer]

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



configure.ac (line 758)


Sorry I'm super late to the party. Does it make sense to add these to the 
CMake build system as well?


- Alex Clemmer


On Oct. 22, 2015, 10:02 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39230/
> ---
> 
> (Updated Oct. 22, 2015, 10:02 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3030
> https://issues.apache.org/jira/browse/MESOS-3030
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   configure.ac 66f9b32 
>   src/authentication/cram_md5/authenticatee.cpp b03b44a 
>   src/authentication/cram_md5/authenticator.cpp f238872 
>   src/authentication/cram_md5/auxprop.cpp abf0f8d 
> 
> Diff: https://reviews.apache.org/r/39230/diff/
> 
> 
> Testing
> ---
> 
> ./configure && make check 
> OSX, clang3.7 + gcc4.9
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 39325: Fixed race between coordinator election and recovery in replicated log.

[Neil Conway]

> On Nov. 2, 2015, 6:16 p.m., Timothy Chen wrote:
> > src/tests/log_tests.cpp, line 703
> > 
> >
> > Looks much more better!
> > Have you tried to run this test repeatedly?

Yep -- running with "--gtest_repeat=5000" doesn't reveal any failures.


- Neil


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


On Nov. 2, 2015, 6:38 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39325/
> ---
> 
> (Updated Nov. 2, 2015, 6:38 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Bugs: MESOS-3280
> https://issues.apache.org/jira/browse/MESOS-3280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3280. The basic problem is that replicas silently ignore inbound Promise
> and Write requests if they have not finished the recovery protocol yet 
> (because
> they can't safely vote on such requests). Hence, if we try to do a Paxos round
> while a quorum of nodes have not finished recovering, the Paxos round will 
> never
> complete. In particular, this might happen during coordinator election:
> coordinator election (which is implemented as performing a full Paxos round)
> starts as soon as the candidate coordinator replica has finished the recovery
> protocol. If several nodes start concurrently, a quorum of those nodes might
> still be executing the recovery protocol, and hence the coordinator will never
> be elected.
> 
> To address this, add "ignored" responses to the Promise and Write 
> sub-protocols:
> if a proposer sees a quorum of "ignored" responses to a promise or write 
> request
> it has issued, it knows the request will never succeed.  When used for
> coordinator election, the current coding will retry immediately (without a
> backoff).
> 
> Note that replicas will still silently drop promise/write requests if another
> kind of problem occurs (e.g., an I/O error prevents reading/writing log
> data). We might consider changing this, although it will require some thought:
> e.g., if a replica's disk is broken, sending an "ignored" message on every
> request might flood the network.
> 
> REVIEWER NOTES:
> 
> * GMock code is ugly, but see notes below.
> * Should we add exponential backoff when retrying coordinator election?
> 
> 
> Diffs
> -
> 
>   src/log/consensus.cpp 71cd5f39c02f583c1ea53d1ab4569115b0cee2a3 
>   src/log/coordinator.cpp e1df8b01f28447955e1e8bbd764fce3e1a948d88 
>   src/log/replica.hpp 70f415f4f81465dbb7d1e4d4f65c7965cd432415 
>   src/log/replica.cpp 414e1165ae969c1f9492e7b8361c72df2c4fbc16 
>   src/messages/log.proto d73b33f865963292af580945659ad0e800f2a204 
>   src/tests/log_tests.cpp 222a12e10032082e6315ca7b43d3393738edd312 
> 
> Diff: https://reviews.apache.org/r/39325/diff/
> 
> 
> Testing
> ---
> 
> "make check" passes, including a new test that uses a newly constructed mock 
> to ensure we're testing the message schedule described above.
> 
> I also wrote a script stops and starts mesos-master in a loop, removing the 
> replicated log each time. Without the patch, this occasionally fails with a 
> "registry fetch" timeout; with the patch, you can observe several scenarios 
> where coordinator election is reborted and retried because a quorum of 
> ignored responses is seen. Note that in some cases, we need to retry 
> coordinator election up to ~70 times (!), because we don't currently use a 
> backoff; that should probably be fixed, per comments above. But the important 
> point is that election eventually succeeds and we don't hang.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 39325: Fixed race between coordinator election and recovery in replicated log.

[Neil Conway]

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

(Updated Nov. 2, 2015, 6:38 p.m.)


Review request for mesos, Jie Yu, Joris Van Remoortere, and Timothy Chen.


Changes
---

Fix Linux compile.


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


Repository: mesos


Description
---

MESOS-3280. The basic problem is that replicas silently ignore inbound Promise
and Write requests if they have not finished the recovery protocol yet (because
they can't safely vote on such requests). Hence, if we try to do a Paxos round
while a quorum of nodes have not finished recovering, the Paxos round will never
complete. In particular, this might happen during coordinator election:
coordinator election (which is implemented as performing a full Paxos round)
starts as soon as the candidate coordinator replica has finished the recovery
protocol. If several nodes start concurrently, a quorum of those nodes might
still be executing the recovery protocol, and hence the coordinator will never
be elected.

To address this, add "ignored" responses to the Promise and Write sub-protocols:
if a proposer sees a quorum of "ignored" responses to a promise or write request
it has issued, it knows the request will never succeed.  When used for
coordinator election, the current coding will retry immediately (without a
backoff).

Note that replicas will still silently drop promise/write requests if another
kind of problem occurs (e.g., an I/O error prevents reading/writing log
data). We might consider changing this, although it will require some thought:
e.g., if a replica's disk is broken, sending an "ignored" message on every
request might flood the network.

REVIEWER NOTES:

* GMock code is ugly, but see notes below.
* Should we add exponential backoff when retrying coordinator election?


Diffs (updated)
-

  src/log/consensus.cpp 71cd5f39c02f583c1ea53d1ab4569115b0cee2a3 
  src/log/coordinator.cpp e1df8b01f28447955e1e8bbd764fce3e1a948d88 
  src/log/replica.hpp 70f415f4f81465dbb7d1e4d4f65c7965cd432415 
  src/log/replica.cpp 414e1165ae969c1f9492e7b8361c72df2c4fbc16 
  src/messages/log.proto d73b33f865963292af580945659ad0e800f2a204 
  src/tests/log_tests.cpp 222a12e10032082e6315ca7b43d3393738edd312 

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


Testing
---

"make check" passes, including a new test that uses a newly constructed mock to 
ensure we're testing the message schedule described above.

I also wrote a script stops and starts mesos-master in a loop, removing the 
replicated log each time. Without the patch, this occasionally fails with a 
"registry fetch" timeout; with the patch, you can observe several scenarios 
where coordinator election is reborted and retried because a quorum of ignored 
responses is seen. Note that in some cases, we need to retry coordinator 
election up to ~70 times (!), because we don't currently use a backoff; that 
should probably be fixed, per comments above. But the important point is that 
election eventually succeeds and we don't hang.


Thanks,

Neil Conway

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

[Alex Clemmer]

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

(Updated Nov. 2, 2015, 6:36 p.m.)


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


Repository: mesos


Description (updated)
---

Made `path_tests.cpp` standalone.

NB, more of these tests will move when `Hopcroft` is done with his 
`os::symlink` changeset.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
ad9ce324eaf940f68d04c6db7ba37d05efb1216a 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 39851: Windows: fixed ambiguousity error in `process/owned.hpp`.

[Alex Clemmer]

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

(Updated Nov. 2, 2015, 6:35 p.m.)


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


Repository: mesos


Description
---

Windows: fixed ambiguousity error in `process/owned.hpp`.


Diffs (updated)
-

  3rdparty/libprocess/include/process/owned.hpp 
1b41477d555ce567c58033f8993dc2ae0ac80a05 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 39851: Windows: fixed ambiguousity error in `process/owned.hpp`.

[Alex Clemmer]

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

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


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


Repository: mesos


Description
---

Windows: fixed ambiguousity error in `process/owned.hpp`.


Diffs (updated)
-

  3rdparty/libprocess/include/process/owned.hpp 
1b41477d555ce567c58033f8993dc2ae0ac80a05 

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


Testing
---


Thanks,

Alex Clemmer