Re: Review Request 58038: Removed redundant call to `std::string::c_str()` in Mesos.

[Benjamin Bannier]

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


Ship it!




- Benjamin Bannier


On March 30, 2017, 3:15 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58038/
> ---
> 
> (Updated March 30, 2017, 3:15 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Spotted using the "readability-redundant-string-cstr" clang-tidy check.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> 13a1f131973d15225e100b04f8e24c1f9f4d8539 
> 
> 
> Diff: https://reviews.apache.org/r/58038/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58037: Removed redundant call to `std::string::c_str()` in stout.

[Benjamin Bannier]

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


Ship it!




- Benjamin Bannier


On March 30, 2017, 3:14 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58037/
> ---
> 
> (Updated March 30, 2017, 3:14 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Spotted using the "readability-redundant-string-cstr" clang-tidy check.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/elf.hpp 
> da2ce0c83372070a1050b8adb4c16a636f63e5b4 
> 
> 
> Diff: https://reviews.apache.org/r/58037/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58060: Added a check to default clang-tidy check list.

[Benjamin Bannier]

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


Ship it!




Note that this patch only requires https://reviews.apache.org/r/58038/.

- Benjamin Bannier


On March 30, 2017, 3:17 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58060/
> ---
> 
> (Updated March 30, 2017, 3:17 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a check to default clang-tidy check list.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy.sh a56ae78b5839dfdacd0d8793f1a9dccb097f0393 
> 
> 
> Diff: https://reviews.apache.org/r/58060/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58061: Added MULTI_ROLE support to the upgrades documentation.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58055, 58061]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On March 30, 2017, 3:38 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58061/
> ---
> 
> (Updated March 30, 2017, 3:38 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6762
> https://issues.apache.org/jira/browse/MESOS-6762
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/upgrades.md c1e6524884723c37e3b804a505b75068910be472 
> 
> 
> Diff: https://reviews.apache.org/r/58061/diff/1/
> 
> 
> Testing
> ---
> 
> See gist here:
> https://gist.github.com/bmahler/2eaf9025f3d4d877db098c3fe3b4a91a
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 58059: Changed libprocess SocketManager to refer to HttpProxy by PID.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58056, 58057, 58058, 58059]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On March 29, 2017, 6:20 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58059/
> ---
> 
> (Updated March 29, 2017, 6:20 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Greg Mann.
> 
> 
> Bugs: MESOS-6919
> https://issues.apache.org/jira/browse/MESOS-6919
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> HttpProxy actors are spawned to manage incoming HTTP connections.
> These actors are themselves garbage collected, meaning it is unsafe
> to refer to an HttpProxy by pointer (which is what is currently done).
> 
> During libprocess finalization, it is possible for an incoming
> connection to spawn an HttpProxy, whose pointer is then deleted by
> finalization.  This leads to a potential segfault when cleaning up
> the incoming connection, as the SocketManager will dereference
> any related HttpProxy actors by pointer.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> f6ee24e2db43d63d91222549efee85421bbf9bf3 
> 
> 
> Diff: https://reviews.apache.org/r/58059/diff/1/
> 
> 
> Testing
> ---
> 
> With the additional test here: https://reviews.apache.org/r/58056/
> 
> make check
> 
> 3rdparty/libprocess/src/tests/libprocess-tests 
> --gtest_filter="*RapidReconnect*" --gtest_repeat=1000
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 58065: Added initial code for the python mesoshttp package.

[Eric Chung]

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

Review request for mesos and Kevin Klues.


Repository: mesos


Description
---

Added initial code for the python mesoshttp package.


Diffs
-

  src/cli_new/bootstrap 6d62e9adf1d543ed00a3a2cf2484edf1c33ee443 
  src/cli_new/pip-requirements.txt 9cc8d096ac353f4ed2d4318f2d0e439a6edf 
  src/python/.gitignore PRE-CREATION 
  src/python/lib/mesoshttp/__init__.py PRE-CREATION 
  src/python/lib/mesoshttp/exceptions.py PRE-CREATION 
  src/python/lib/mesoshttp/http.py PRE-CREATION 
  src/python/lib/tests/mesoshttp/test_exceptions.py PRE-CREATION 
  src/python/lib/tests/mesoshttp/test_http.py PRE-CREATION 
  support/mesos-style.py 6489c2de8cc8e14f28ef89c7604c94d3edff 


Diff: https://reviews.apache.org/r/58065/diff/1/


Testing
---

unit tests:
$ source src/cli_new/activate
$ pytest --cov src/python/lib/mesoshttp --cov-report term-missing src/python/lib


Thanks,

Eric Chung

Re: Review Request 58055: Documentation updates to reflect multi-role framework support.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58055]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On March 30, 2017, 1:13 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58055/
> ---
> 
> (Updated March 30, 2017, 1:13 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-7324
> https://issues.apache.org/jira/browse/MESOS-7324
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This also updates the roles documentation to reflect the vision
> of roles representing the resource consumer.
> 
> 
> Diffs
> -
> 
>   docs/authentication.md 1574db981d5f8ddd7d1f6bef1c2b032823d17297 
>   docs/authorization.md b019120b1dde35d2cd5f613ddc3d6db32715c9ca 
>   docs/persistent-volume.md 410993fb69eb73db6ad17ef361f9b72cba5dc84d 
>   docs/quota.md 931542b37ef398c015e33ca004650b7689e03adf 
>   docs/reservation.md ace5cef4bccb56126a5338ecde04b91c1a90ce58 
>   docs/roles.md 344a0e375210e5e60e225276b715650dd0934d47 
>   docs/scheduler-http-api.md 7f808f10a9f71fba44574079238fd6028ff6520c 
>   docs/shared-resources.md 1b490e253447a9e263cb5a6bda10f360ea7138ce 
> 
> 
> Diff: https://reviews.apache.org/r/58055/diff/1/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 57513: Cleaned up `strings::tokenize` and `strings::split`.

[Neil Conway]

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

(Updated March 30, 2017, 2:10 a.m.)


Review request for mesos and Michael Park.


Changes
---

Improve comments.


Repository: mesos


Description (updated)
---

The previous coding used a `while` loop condition that was invariant
over the body of the loop; it is clearer to write this as an `if`
statement instead. Also clarified comments.


Diffs (updated)
-

  3rdparty/stout/include/stout/strings.hpp 
fe469fa492f179d42794a8ce8f75c0654be7ef58 


Diff: https://reviews.apache.org/r/57513/diff/2/

Changes: https://reviews.apache.org/r/57513/diff/1-2/


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 56155: Implemented the 'moveImage()' method of OCI store.

[Qian Zhang]

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

(Updated March 30, 2017, 10:08 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Implemented the 'moveImage()' method of OCI store.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/oci/store.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/56155/diff/2/

Changes: https://reviews.apache.org/r/56155/diff/1-2/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 56154: Implemented the 'fetchLayers()' method of prefix puller.

[Qian Zhang]

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

(Updated March 30, 2017, 10:07 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Summary (updated)
-

Implemented the 'fetchLayers()' method of prefix puller.


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


Repository: mesos


Description (updated)
---

Implemented the 'fetchLayers()' method of prefix puller.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/oci/prefix_puller.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/56154/diff/2/

Changes: https://reviews.apache.org/r/56154/diff/1-2/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 56153: Implemented the 'fetchConfig()' method of prefix puller.

[Qian Zhang]

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

(Updated March 30, 2017, 10:07 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Summary (updated)
-

Implemented the 'fetchConfig()' method of prefix puller.


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


Repository: mesos


Description (updated)
---

Implemented the 'fetchConfig()' method of prefix puller.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/oci/prefix_puller.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/56153/diff/2/

Changes: https://reviews.apache.org/r/56153/diff/1-2/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 56152: Implemented the 'fetchManifest()' method of prefix puller.

[Qian Zhang]

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

(Updated March 30, 2017, 10:06 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Summary (updated)
-

Implemented the 'fetchManifest()' method of prefix puller.


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


Repository: mesos


Description (updated)
---

Implemented the 'fetchManifest()' method of prefix puller.


Diffs (updated)
-

  include/mesos/oci/spec.hpp d8eef84b5770608c359285d9168f7ea5de4eba12 
  src/slave/containerizer/mesos/provisioner/oci/paths.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/paths.cpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/prefix_puller.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/56152/diff/2/

Changes: https://reviews.apache.org/r/56152/diff/1-2/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 57167: Updated quota handler logic for hierarchical roles.

[Neil Conway]

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

(Updated March 30, 2017, 2:06 a.m.)


Review request for mesos and Michael Park.


Changes
---

Simplify request URL parsing logic for quota removal, per discussion.


Repository: mesos


Description (updated)
---

The quota'd resources for a nested role are "included" within the
quota'd resources for that role's parent. Hence, the quota of a node
must always be greater than or equal to the sum of the quota'd resources
of that role's children. When creating and removing quota, we must
ensure that this invariant is not violated.

When computing the cluster capacity heuristic, we must ensure that we do
not "double-count" quota'd resources: e.g., if the cluster has a total
capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
cluster capacity heuristic.


Diffs (updated)
-

  src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67 
  src/tests/hierarchical_allocator_tests.cpp 
e343dc37bd7136f0f6dd5dbc22a25cabe715038d 
  src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837 


Diff: https://reviews.apache.org/r/57167/diff/10/

Changes: https://reviews.apache.org/r/57167/diff/9-10/


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 56150: Implemented the 'fetchIndex()' method of prefix puller.

[Qian Zhang]

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

(Updated March 30, 2017, 10:05 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Summary (updated)
-

Implemented the 'fetchIndex()' method of prefix puller.


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


Repository: mesos


Description (updated)
---

Implemented the 'fetchIndex()' method of prefix puller.


Diffs (updated)
-

  include/mesos/oci/spec.hpp d8eef84b5770608c359285d9168f7ea5de4eba12 
  src/slave/containerizer/mesos/provisioner/oci/paths.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/paths.cpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/prefix_puller.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/56150/diff/2/

Changes: https://reviews.apache.org/r/56150/diff/1-2/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 57167: Updated quota handler logic for hierarchical roles.

[Neil Conway]

> On March 28, 2017, 11:06 p.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 469-487 (original), 605-618 (patched)
> > 
> >
> > Couldn't this be just:
> > 
> > ```cpp
> > vector components = strings::split(request.url.path, "/", 3u);
> > if (components.size() < 3u) {
> >   return BadRequest(
> >   "Failed to parse request path '" + request.url.path +
> >   "': 3 tokens ('master', 'quota', 'role') required, found " +
> >   stringify(tokens.size()) + " token(s)"));
> > }
> > 
> > CHECK_EQ(3u, components.size());
> > const string& role = components.back();
> > /* ... */
> > ```
> 
> Neil Conway wrote:
> I'm not sure this is actually more readable; leaving as-is for now, will 
> discuss offline.
> 
> Michael Park wrote:
> A summary of our discussion yesterday: the `components.begin() + 3` part 
> looks wrong, but is indeed correct
> because `split` returns empty tokens, and the first token will be an 
> empty string.
> This means that my suggestion of `split(..., 3u)` actually has a bug... 
> subtle :(.
> 
> We also didn't like the splitting/joining of the "rest", because it's not 
> obvious that we get back what we put in.
> 
> Also, based on the code that I see in libprocess for `route`, it looks 
> like we would allow extraneous slashes such as `/master///quota`.
> This would mean that the use of `split` actually introduces a 
> backwards-incompatibility here.
> 
> I personally think it's simplest for us to do 
> `strings::tokenize(request.url.path, "/", 3u)`.
> 
> What do you think?

sg!


- Neil


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


On March 28, 2017, 11:42 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> ---
> 
> (Updated March 28, 2017, 11:42 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children.
> 
> When creating and removing quota, we must ensure that this invariant is
> not violated.
> 
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
> role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
> cluster capacity heuristic.
> 
> 
> Diffs
> -
> 
>   src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67 
>   src/tests/hierarchical_allocator_tests.cpp 
> e343dc37bd7136f0f6dd5dbc22a25cabe715038d 
>   src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837 
> 
> 
> Diff: https://reviews.apache.org/r/57167/diff/9/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 56149: Implemented the 'pull()' method of prefix puller.

[Qian Zhang]

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

(Updated March 30, 2017, 10:04 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Summary (updated)
-

Implemented the 'pull()' method of prefix puller.


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


Repository: mesos


Description (updated)
---

Implemented the 'pull()' method of prefix puller.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/oci/prefix_puller.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/56149/diff/2/

Changes: https://reviews.apache.org/r/56149/diff/1-2/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 56148: Implemented the '_get()' method of OCI store.

[Qian Zhang]

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

(Updated March 30, 2017, 10:03 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Updated the code with the puller implementation.


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


Repository: mesos


Description
---

Implemented the '_get()' method of OCI store.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/oci/paths.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/paths.cpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/store.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/56148/diff/3/

Changes: https://reviews.apache.org/r/56148/diff/2-3/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 55333: Implemented the 'get()' and '__get()' methods of OCI store.

[Qian Zhang]

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

(Updated March 30, 2017, 10:02 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Implemented the 'get()' and '__get()' methods of OCI store.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/oci/paths.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/paths.cpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/store.cpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/store.hpp 
82a9be64264ae829773c1e2e8a4360f78641cbf6 


Diff: https://reviews.apache.org/r/55333/diff/3/

Changes: https://reviews.apache.org/r/55333/diff/2-3/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 56147: Implemented metadata manager for OCI store.

[Qian Zhang]

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

(Updated March 30, 2017, 10:01 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Updated the code with the latest `Image` message.


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


Repository: mesos


Description
---

Implemented metadata manager for OCI store.


Diffs (updated)
-

  src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
  src/Makefile.am 071656ad7354a802e8292140a7181cb70b68fe9e 
  src/slave/containerizer/mesos/provisioner/oci/metadata_manager.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/metadata_manager.cpp 
PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/paths.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/paths.cpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/store.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/56147/diff/2/

Changes: https://reviews.apache.org/r/56147/diff/1-2/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 55332: Added 'message.proto' for OCI store.

[Qian Zhang]

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

(Updated March 30, 2017, 9:59 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Replaced `config_id` and `layer_ids` with `manifest_id` in the `Image` message.


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


Repository: mesos


Description
---

Added 'message.proto' for OCI store.


Diffs (updated)
-

  src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
  src/Makefile.am 071656ad7354a802e8292140a7181cb70b68fe9e 
  src/slave/containerizer/mesos/provisioner/oci/message.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/message.proto PRE-CREATION 


Diff: https://reviews.apache.org/r/55332/diff/3/

Changes: https://reviews.apache.org/r/55332/diff/2-3/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 54639: Implemented the 'create()' method of OCI store.

[Qian Zhang]

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

(Updated March 30, 2017, 9:56 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Changed the filesystem layout of OCI store.


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


Repository: mesos


Description
---

Implemented the 'create()' method of OCI store.


Diffs (updated)
-

  src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
  src/Makefile.am 071656ad7354a802e8292140a7181cb70b68fe9e 
  src/slave/containerizer/mesos/provisioner/oci/paths.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/paths.cpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/store.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/54639/diff/4/

Changes: https://reviews.apache.org/r/54639/diff/3-4/


Testing
---


Thanks,

Qian Zhang

Review Request 58064: Implemented the 'create()' method of puller interface.

[Qian Zhang]

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

Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Implemented the 'create()' method of puller interface.


Diffs
-

  src/slave/containerizer/mesos/provisioner/oci/puller.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/58064/diff/1/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 58062: Added puller interface for OCI store.

[Qian Zhang]

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

(Updated March 30, 2017, 9:53 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Added puller interface for OCI store.


Diffs
-

  include/mesos/type_utils.hpp 90b0227fd61d791e7bc791e6e68e11bf0f9ecac4 
  src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
  src/Makefile.am 071656ad7354a802e8292140a7181cb70b68fe9e 
  src/slave/containerizer/mesos/provisioner/oci/puller.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/puller.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/58062/diff/1/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 58063: Added stubs for prefix puller.

[Qian Zhang]

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

(Updated March 30, 2017, 9:53 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Added stubs for prefix puller.


Diffs
-

  src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
  src/Makefile.am 071656ad7354a802e8292140a7181cb70b68fe9e 
  src/slave/containerizer/mesos/provisioner/oci/prefix_puller.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/prefix_puller.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/58063/diff/1/


Testing
---


Thanks,

Qian Zhang

Review Request 58063: Added stubs for prefix puller.

[Qian Zhang]

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

Review request for mesos.


Repository: mesos


Description
---

Added stubs for prefix puller.


Diffs
-

  src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
  src/Makefile.am 071656ad7354a802e8292140a7181cb70b68fe9e 
  src/slave/containerizer/mesos/provisioner/oci/prefix_puller.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/prefix_puller.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/58063/diff/1/


Testing
---


Thanks,

Qian Zhang

Review Request 58062: Added puller interface for OCI store.

[Qian Zhang]

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

Review request for mesos.


Repository: mesos


Description
---

Added puller interface for OCI store.


Diffs
-

  include/mesos/type_utils.hpp 90b0227fd61d791e7bc791e6e68e11bf0f9ecac4 
  src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
  src/Makefile.am 071656ad7354a802e8292140a7181cb70b68fe9e 
  src/slave/containerizer/mesos/provisioner/oci/puller.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/puller.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/58062/diff/1/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 52382: Added stubs for OCI store.

[Qian Zhang]

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

(Updated March 30, 2017, 9:42 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Addressed Jie's comments.


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


Repository: mesos


Description
---

Added stubs for OCI store.


Diffs (updated)
-

  src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
  src/Makefile.am 071656ad7354a802e8292140a7181cb70b68fe9e 
  src/slave/containerizer/mesos/provisioner/oci/store.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/store.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/52382/diff/9/

Changes: https://reviews.apache.org/r/52382/diff/8-9/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 54638: Added agent flag '--oci_default_locator'.

[Qian Zhang]

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

(Updated March 30, 2017, 9:40 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Merged the flags `--oci_discovery` and `--oci_discovery_prefix` into 
`--oci_default_locator`.


Summary (updated)
-

Added agent flag '--oci_default_locator'.


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


Repository: mesos


Description (updated)
---

Added agent flag '--oci_default_locator'.


Diffs (updated)
-

  docs/configuration.md 452478eab36f352a31f62f0c1f4da2f43f89bef9 
  include/mesos/type_utils.hpp 90b0227fd61d791e7bc791e6e68e11bf0f9ecac4 
  src/common/parse.hpp e90738a91161e26a48a6e381765e631492294641 
  src/common/type_utils.cpp 9d87a6d310a521f88cb744a9bd7e9f9a7b3d39c0 
  src/slave/flags.hpp 224fac1d06d5a3914d4d1408e880458ac5be010e 
  src/slave/flags.cpp 76881536e0058880f5720fbf3c1cebc684508235 


Diff: https://reviews.apache.org/r/54638/diff/4/

Changes: https://reviews.apache.org/r/54638/diff/3-4/


Testing
---


Thanks,

Qian Zhang

Review Request 58061: Added MULTI_ROLE support to the upgrades documentation.

[Benjamin Mahler]

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

Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  docs/upgrades.md c1e6524884723c37e3b804a505b75068910be472 


Diff: https://reviews.apache.org/r/58061/diff/1/


Testing
---

See gist here:
https://gist.github.com/bmahler/2eaf9025f3d4d877db098c3fe3b4a91a


Thanks,

Benjamin Mahler

Re: Review Request 52379: Added agent flag '--oci_store_dir'.

[Qian Zhang]

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

(Updated March 30, 2017, 9:37 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Added agent flag '--oci_store_dir'.


Diffs
-

  docs/configuration.md 6a30471d325f35e171e81a687c23fd69e9ec97ee 
  docs/endpoints/slave/state.json.md 0f82c1926404e79b281b2ea5f4d0ca21323aeded 
  docs/endpoints/slave/state.md b34459e8624f0b29e927ff79be7fc845ac88080b 
  src/slave/flags.hpp 2c4bd6ae628a272a4c6c2f02670baef011df4505 
  src/slave/flags.cpp b48678bae8b3f5bab62710043eaca14ab8370183 
  src/slave/http.cpp d68c9adc1db43c2e853c8b2290705fdc7739d45c 


Diff: https://reviews.apache.org/r/52379/diff/7/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 57358: Implemented discard behavior in process::Queue.

[Joseph Wu]

> On March 24, 2017, 9:09 a.m., Greg Mann wrote:
> > 3rdparty/libprocess/include/process/queue.hpp
> > Line 58 (original), 58 (patched)
> > 
> >
> > I was just testing this patch locally, and noticed that after applying 
> > it, the NetSocketTests will deadlock. If I run 
> > `3rdparty/libprocess/libprocess-tests --gtest_filter="*NetSocketTest.*"` 
> > the deadlock appears. Haven't had a chance to dig in yet.

If you apply this: https://reviews.apache.org/r/57821/
The deadlock should go away.


- Joseph


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


On March 21, 2017, 2:40 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57358/
> ---
> 
> (Updated March 21, 2017, 2:40 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Greg Mann.
> 
> 
> Bugs: MESOS-6919
> https://issues.apache.org/jira/browse/MESOS-6919
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When a consumer calls `Queue::get` on an empty Queue, the Queue creates
> a Promise to back the Future return value.  This Promise currently
> does not have a discard handler, which may cause problems if the
> Future is chained to multiple continuations.  For example, see
> the scenario in MESOS-6919.
> 
> This commit implements an (potentially expensive) discard handler
> on the Queue's Promise.  If the Future return value is discarded,
> the Queue will remove the corresponding Promise from its internal
> queue of promises.  The operation is expensive because it needs
> to reconstruct the entire internal queue of promises.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/queue.hpp 
> ab08e30df742412f22a06202526f8b55350ed435 
>   3rdparty/libprocess/src/tests/queue_tests.cpp 
> 95b738133fa50641f8f9b83014837d2808e0e4ff 
> 
> 
> Diff: https://reviews.apache.org/r/57358/diff/4/
> 
> 
> Testing
> ---
> 
> cmake .. -DENABLE_LIBEVENT=1 -DENABLE_SSL=1
> 
> make libprocess-tests
> 
> 3rdparty/libprocess/src/tests/libprocess-tests 
> --gtest_filter="Scheme/HTTPTest.Endpoints/0" --gtest_repeat="`ulimit -n`"
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 58059: Changed libprocess SocketManager to refer to HttpProxy by PID.

[Joseph Wu]

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

Review request for mesos, Benjamin Mahler and Greg Mann.


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


Repository: mesos


Description
---

HttpProxy actors are spawned to manage incoming HTTP connections.
These actors are themselves garbage collected, meaning it is unsafe
to refer to an HttpProxy by pointer (which is what is currently done).

During libprocess finalization, it is possible for an incoming
connection to spawn an HttpProxy, whose pointer is then deleted by
finalization.  This leads to a potential segfault when cleaning up
the incoming connection, as the SocketManager will dereference
any related HttpProxy actors by pointer.


Diffs
-

  3rdparty/libprocess/src/process.cpp f6ee24e2db43d63d91222549efee85421bbf9bf3 


Diff: https://reviews.apache.org/r/58059/diff/1/


Testing
---

With the additional test here: https://reviews.apache.org/r/58056/

make check

3rdparty/libprocess/src/tests/libprocess-tests 
--gtest_filter="*RapidReconnect*" --gtest_repeat=1000


Thanks,

Joseph Wu

Review Request 58057: Changed SSL Socket destruction into a synchronous operation.

[Joseph Wu]

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

Review request for mesos, Benjamin Mahler and Greg Mann.


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


Repository: mesos


Description
---

Previously, the destruction of SSL Sockets was changed from a relatively
simple synchronous delete of data structures, to one where the data
structures are deleted on the event loop.

This implementation lead to the possibility of the socket's file
descriptor out-living the Socket wrapper.  In the case of server
sockets, this potentially allows incoming connections to be made
against a server socket that is considered "closed" (but the FD is
still accepting).

This commit adds a Gate to the SSL Socket to wait for destruction
to finish before returning.  If the SSL Socket's destructor is itself
run on the event loop, the destructor logic is allowed to short-circuit
and run synchronously.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
7d493301bd5c0f24bf89e0b213f07ffe7801508b 


Diff: https://reviews.apache.org/r/58057/diff/1/


Testing
---

See end of chain


Thanks,

Joseph Wu

Review Request 58056: Temp: Added rapid-fire socket test for reinitialization testing.

[Joseph Wu]

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

Review request for mesos, Benjamin Mahler and Greg Mann.


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


Repository: mesos


Description
---

This test was mostly used to re-create a segfault found in Mesos
tests, but at the libprocess level.


Diffs (updated)
-

  3rdparty/libprocess/src/tests/http_tests.cpp 
09c7297aa0e80accd09ce6d81390a0f662bec6f2 


Diff: https://reviews.apache.org/r/58056/diff/1/


Testing
---

See end of chain


Thanks,

Joseph Wu

Review Request 58058: Moved libprocess initialization of worker threads later.

[Joseph Wu]

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

Review request for mesos, Benjamin Mahler and Greg Mann.


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


Repository: mesos


Description
---

This commit moves the creation of all libprocess worker threads
after the creation of the garbage collector process.

This deals with a test-only case where:
  1) Events are queued on the event loop.
  2) Libprocess is finalized as part of the test,
 before processing all events.
  3) Libprocess is reinitialized and the previously queued events
 are allowed to resume.

Because the events were queued in a previous incarnation of
libprocess, they potentially bypass the synchronization variables
in `process::initialize` (i.e. `initialize_complete`) and can
spawn garbage-collected processes before the garbage collector
has been spawned.


Diffs
-

  3rdparty/libprocess/src/process.cpp f6ee24e2db43d63d91222549efee85421bbf9bf3 


Diff: https://reviews.apache.org/r/58058/diff/1/


Testing
---

See end of chain


Thanks,

Joseph Wu

Re: Review Request 55331: Added 'OCI' message into 'Image' message.

[Qian Zhang]

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

(Updated March 30, 2017, 9:19 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Added 'OCI' message into 'Image' message.


Diffs
-

  include/mesos/mesos.proto 9a66967ab459f75f21faf21be644f39b3fad670b 
  include/mesos/v1/mesos.proto 115f1b4d9e129f83a3aed62a95eb11faa12e04d1 


Diff: https://reviews.apache.org/r/55331/diff/3/


Testing
---


Thanks,

Qian Zhang

Review Request 58060: Added a check to default clang-tidy check list.

[Neil Conway]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

Added a check to default clang-tidy check list.


Diffs
-

  support/mesos-tidy.sh a56ae78b5839dfdacd0d8793f1a9dccb097f0393 


Diff: https://reviews.apache.org/r/58060/diff/1/


Testing
---


Thanks,

Neil Conway

Re: Review Request 55331: Added 'OCI' message into 'Image' message.

[Qian Zhang]

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

(Updated March 30, 2017, 9:16 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Introduced `Locator` message.


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


Repository: mesos


Description
---

Added 'OCI' message into 'Image' message.


Diffs (updated)
-

  include/mesos/mesos.proto 9a66967ab459f75f21faf21be644f39b3fad670b 
  include/mesos/v1/mesos.proto 115f1b4d9e129f83a3aed62a95eb11faa12e04d1 


Diff: https://reviews.apache.org/r/55331/diff/3/

Changes: https://reviews.apache.org/r/55331/diff/2-3/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 58039: Avoid redundant copies when using `std::get` on a tuple.

[Neil Conway]

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

(Updated March 30, 2017, 1:15 a.m.)


Review request for mesos and Benjamin Bannier.


Repository: mesos


Description (updated)
---

`std::get` returns a reference; in most cases we can avoid a copy at the
call site by assigning the result to a const reference.

Spotted using the "performance-unnecessary-copy-initialization"
clang-tidy check.


Diffs (updated)
-

  src/checks/health_checker.cpp 2211228f7aa0228af64d8fce6c5f2dd1847328f9 
  src/common/command_utils.cpp cb9de720814c7c69758d167fbe6292f12cd73224 
  src/hdfs/hdfs.cpp 93450b9c18198c5e841ba9dc7ec6e903cf119f89 
  src/linux/perf.cpp 2271564eeb6c8e5b7dca1461f3f94f1c12b35a1a 
  src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp 
8cc1b12624ce92afa9e2d535cfbdebf5cc3cae35 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
6e95315b70a5d9d3b4b21c4cf235b0a483760190 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 1be8c23e62dee7fb2f9bca23e550ab5884ad8b63 
  src/slave/containerizer/mesos/isolators/posix/disk.cpp 
805566ca9c346e9c1987f804bcc5d111886eb729 
  src/uri/fetchers/copy.cpp 9c79ac6075a8350b52676d5e219491ded3d3625e 
  src/uri/fetchers/curl.cpp a592eb501fda859764d5f14d866032ca8ca3dc97 
  src/uri/fetchers/docker.cpp d051a4d534c24ef6c4dd98181a0f2a175c69d0f9 


Diff: https://reviews.apache.org/r/58039/diff/2/

Changes: https://reviews.apache.org/r/58039/diff/1-2/


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 58037: Removed redundant call to `std::string::c_str()` in stout.

[Neil Conway]

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

(Updated March 30, 2017, 1:14 a.m.)


Review request for mesos and Benjamin Bannier.


Repository: mesos


Description (updated)
---

Spotted using the "readability-redundant-string-cstr" clang-tidy check.


Diffs (updated)
-

  3rdparty/stout/include/stout/elf.hpp da2ce0c83372070a1050b8adb4c16a636f63e5b4 


Diff: https://reviews.apache.org/r/58037/diff/2/

Changes: https://reviews.apache.org/r/58037/diff/1-2/


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 58040: Avoid various redundant copies.

[Neil Conway]

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

(Updated March 30, 2017, 1:15 a.m.)


Review request for mesos and Benjamin Bannier.


Repository: mesos


Description (updated)
---

Spotted using the "performance-unnecessary-copy-initialization"
clang-tidy check.


Diffs (updated)
-

  src/examples/dynamic_reservation_framework.cpp 
c1650dc978dddf14cf1e22a33752b58b06914ff4 
  src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
  src/master/master.cpp ab071f56a66ae66c97a185b2377a9cfe9a5c1ade 
  src/oci/spec.cpp 06fceb4f0441431d756eb45c8aaf9730ef9e248a 
  src/tests/default_executor_tests.cpp 6dadd8937eb6809bcb0aca55fab1cb7f17c3262f 


Diff: https://reviews.apache.org/r/58040/diff/2/

Changes: https://reviews.apache.org/r/58040/diff/1-2/


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 58038: Removed redundant call to `std::string::c_str()` in Mesos.

[Neil Conway]

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

(Updated March 30, 2017, 1:15 a.m.)


Review request for mesos and Benjamin Bannier.


Repository: mesos


Description (updated)
---

Spotted using the "readability-redundant-string-cstr" clang-tidy check.


Diffs (updated)
-

  src/tests/containerizer/docker_volume_isolator_tests.cpp 
13a1f131973d15225e100b04f8e24c1f9f4d8539 


Diff: https://reviews.apache.org/r/58038/diff/2/

Changes: https://reviews.apache.org/r/58038/diff/1-2/


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 58041: Don't mark a value parameter `const`.

[Neil Conway]

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

(Updated March 30, 2017, 1:15 a.m.)


Review request for mesos and Benjamin Bannier.


Repository: mesos


Description (updated)
---

This has no effect on the function's signature.

Spotted using the "readability-avoid-const-params-in-decls" clang-tidy
check.


Diffs (updated)
-

  src/tests/fetcher_cache_tests.cpp 49173af9fa1537b28784ae7543458e5b57e5180f 


Diff: https://reviews.apache.org/r/58041/diff/2/

Changes: https://reviews.apache.org/r/58041/diff/1-2/


Testing
---

`make check`

Note that we could make similar changes in more places, but I just fixed this 
one spot for now.


Thanks,

Neil Conway

Review Request 58055: Documentation updates to reflect multi-role framework support.

[Benjamin Mahler]

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

Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

This also updates the roles documentation to reflect the vision
of roles representing the resource consumer.


Diffs
-

  docs/authentication.md 1574db981d5f8ddd7d1f6bef1c2b032823d17297 
  docs/authorization.md b019120b1dde35d2cd5f613ddc3d6db32715c9ca 
  docs/persistent-volume.md 410993fb69eb73db6ad17ef361f9b72cba5dc84d 
  docs/quota.md 931542b37ef398c015e33ca004650b7689e03adf 
  docs/reservation.md ace5cef4bccb56126a5338ecde04b91c1a90ce58 
  docs/roles.md 344a0e375210e5e60e225276b715650dd0934d47 
  docs/scheduler-http-api.md 7f808f10a9f71fba44574079238fd6028ff6520c 
  docs/shared-resources.md 1b490e253447a9e263cb5a6bda10f360ea7138ce 


Diff: https://reviews.apache.org/r/58055/diff/1/


Testing
---

N/A


Thanks,

Benjamin Mahler

Re: Review Request 57994: Fixed a regression hiding previously exposed master and agent flags.

[Michael Park]

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


Fix it, then Ship it!





src/master/flags.hpp
Line 98 (original), 114 (patched)


Why this change? There are bunch of references to `::size_t` just above. 
Seems unnecessary for this patch.


- Michael Park


On March 28, 2017, 7:01 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57994/
> ---
> 
> (Updated March 28, 2017, 7:01 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Michael Park.
> 
> 
> Bugs: MESOS-7316
> https://issues.apache.org/jira/browse/MESOS-7316
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In f441eb9 we in a number of places changed  how 'Flag's were added to
> 'Flags' by moving from ad-hoc invocations of 'FlagsBase::add' on
> particular instances to proper 'Flags' member variables. This was needed
> to ensure 'Flags' instances could always safely be copied. For that we
> introduced local derived 'Flags' classes to support localized parsing
> needs. At the same time, this implementation strategy led to these these
> local variables not being accessible through instances of the original
> class anymore (this was inevitable when making 'Flags' classes properly
> copyable), which e.g., causes a regression in the flags displayed in a
> master's '/flags' endpoint.
> 
> This commit moves the flags into the respective base class removing the
> local classes so that all passed flags are exposed to users.
> 
> 
> Diffs
> -
> 
>   src/master/flags.hpp 41a0edfaecf04759f1efa62a9851fbeeb214e84c 
>   src/master/flags.cpp b7a129b27bf752bf238d214534364403853c1b36 
>   src/master/main.cpp fa7ba1310142a3bef71379ba37fded9b8390aae9 
>   src/slave/flags.hpp 224fac1d06d5a3914d4d1408e880458ac5be010e 
>   src/slave/flags.cpp 76881536e0058880f5720fbf3c1cebc684508235 
>   src/slave/main.cpp 81d61b14accca7611d84db92663a63d5777edd83 
> 
> 
> Diff: https://reviews.apache.org/r/57994/diff/1/
> 
> 
> Testing
> ---
> 
> Tested on a number of platforms in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 57788: Changed allocator to skip allocation on weight and quota changes.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On March 22, 2017, 11:36 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57788/
> ---
> 
> (Updated March 22, 2017, 11:36 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Bannier, 
> Benjamin Mahler, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changing weight or quota will no longer trigger a batch allocation.
> Since the allocator does not rebalance currently offered resources to
> reflect changes to weight or quota, doing a batch allocation is not
> useful; instead, the updated quota/weight values will be reflected on
> the next allocation.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8d54a8cca1bb478f4437f68c5e14f66a9f9bb9e9 
>   src/tests/hierarchical_allocator_tests.cpp 
> e343dc37bd7136f0f6dd5dbc22a25cabe715038d 
> 
> 
> Diff: https://reviews.apache.org/r/57788/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57564: Changed DRFSorter's representation of inactive clients.

[Benjamin Mahler]

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




src/master/allocator/sorter/drf/sorter.hpp
Lines 214-216 (original), 215-217 (patched)


This comment is no longer accurate, right? We used to store a distinct 
structure purely because of the deactivated clients being absent from the 
`clients` map. Now that this is no longer the case, it seems like this patch 
should be removing this additional data structure (which needs to be kept in 
sync) and just storing `Allocation` within Client.

Won't we need to do this anyway for the hierarchy change? Seems clearer to 
me to make that change in this patch.



src/tests/sorter_tests.cpp
Lines 243 (patched)


Thanks for adding these comments!



src/tests/sorter_tests.cpp
Line 267 (original), 268 (patched)


Looks like c is supposed to have a count of 5 here?



src/tests/sorter_tests.cpp
Lines 282 (patched)


Ditto here?


- Benjamin Mahler


On March 13, 2017, 6:04 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57564/
> ---
> 
> (Updated March 13, 2017, 6:04 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> DRFSorter previously removed inactive clients from the `clients`
> collection, and then re-added clients when they were reactivated. This
> resulted in resetting the allocation count for the client, which is
> unfortunate. This scheme would also be more difficult to adapt to
> hierarchical sorting.
> 
> This commit changes DRFSorter to continue to store inactive clients in
> the `clients`; inactive clients are indicated by a new field in the
> `Client` struct, and are omitted from the return value of
> `DRFSorter::sort`.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 76329220e1115c1de7810fb69b943c78c078be59 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ed54680cecb637931fc344fbcf8fd3b14cc24295 
>   src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6 
> 
> 
> Diff: https://reviews.apache.org/r/57564/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57975: Windows: Use Subprocess in default executor.

[Andrew Schwartzmeyer]

> On March 29, 2017, 9:40 p.m., Joseph Wu wrote:
> > src/launcher/CMakeLists.txt
> > Lines 29-39 (original), 29-39 (patched)
> > 
> >
> > No need for ifdef-ing here anymore.

It's taken care of in a later commit.


> On March 29, 2017, 9:40 p.m., Joseph Wu wrote:
> > src/launcher/executor.hpp
> > Lines 21-27 (original), 21 (patched)
> > 
> >
> > There's going to be a bunch of code movement...
> > 
> > But now that the `launcher/windows/executor.*` are disappearing, all 
> > the common code should be moved into this file.  And a corresponding .cpp 
> > file.
> 
> Joseph Wu wrote:
> And now I see the next review :D https://reviews.apache.org/r/57976
> 
> Consider squashing the two together.

If we squash, we lose the actual code changes due to movement.


> On March 29, 2017, 9:40 p.m., Joseph Wu wrote:
> > src/launcher/executor.cpp
> > Line 519 (original), 506 (patched)
> > 
> >
> > This can probably be called `launchTaskAsSubprocess` now.

Yeah it's `launchTaskSubprocess` in the next commit.


- Andrew


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


On March 27, 2017, 10:39 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57975/
> ---
> 
> (Updated March 27, 2017, 10:39 p.m.)
> 
> 
> Review request for mesos, John Kordich, Joseph Wu, Li Li, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By encapsulating the job object logic inside the Windows libprocess
> actor, we're able to reuse `subprocess` for launching tasks on Windows.
> This allows us to remove the entirety of `launchTaskWindows` and instead
> reuse `launchTaskPosix`, which just uses `subprocess`. This also fixed
> the `CommitSuicideOnTaskFailure` test, which is now enabled.
> 
> 
> Diffs
> -
> 
>   src/launcher/CMakeLists.txt f63f544f92924b92ef41382c40acabef59a56d8b 
>   src/launcher/executor.hpp c7c134aed26d2116295d66100b3d6efaf610736c 
>   src/launcher/executor.cpp a4bac9d932b6574eb2e1b4a27aa44eaaca6aaa62 
>   src/launcher/posix/executor.hpp 2dd9766aa5b6e0550269ccaa79209d0a483fee76 
>   src/launcher/posix/executor.cpp 7c4ef10390e7ecfe63e2fd0c813f91c896fc7a8d 
>   src/launcher/windows/executor.hpp 6f02912c477105819b4c27ddf248b7289799eaa0 
>   src/launcher/windows/executor.cpp b51fde7376c2083119f342ea599b446944ede000 
>   src/tests/default_executor_tests.cpp 
> 6dadd8937eb6809bcb0aca55fab1cb7f17c3262f 
> 
> 
> Diff: https://reviews.apache.org/r/57975/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 57973: Windows: Add `JobObjectManager` actor.

[Andrew Schwartzmeyer]

> On March 29, 2017, 9:36 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/include/process/windows/jobobject.hpp
> > Lines 33 (patched)
> > 
> >
> > Can you move this comment down to the `extern` below?

Whoops.


> On March 29, 2017, 9:36 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/include/process/windows/jobobject.hpp
> > Lines 48-49 (patched)
> > 
> >
> > Try this:
> > ```
> > process::reap(pid)
> >   .onAny(defer(self(), &Self::cleanup, lambda::_1, pid));
> > ```

I'll give it a shot... Michael and I had a fun time with this.


> On March 29, 2017, 9:36 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/include/process/windows/jobobject.hpp
> > Lines 53 (patched)
> > 
> >
> > This shouldn't be a virtual function, presumably.

True!


> On March 29, 2017, 9:36 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/include/process/windows/jobobject.hpp
> > Lines 61-65 (patched)
> > 
> >
> > Hm... we need to clean these up eventually...
> > 
> > What sort of statistics will be gathered?  And how long does the job 
> > object need to live once gathered?

So, this ties into the resource limit APIs etc. Presumably a task can crash 
(process ends) but the status still needs to be able to report I/O used etc. To 
answer your question of what sort of statistics, I would need to move foward 
with the TaskInfo mapping issue. Once gathered, the job object can be fully 
closed.


> On March 29, 2017, 9:36 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/include/process/windows/jobobject.hpp
> > Lines 91-92 (patched)
> > 
> >
> > Outdated comment referring to the launcher here.

Whoops.


> On March 29, 2017, 9:36 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Lines 1211 (patched)
> > 
> >
> > Perhaps add `(Windows only)` here.

Ah, yes.


- Andrew


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


On March 27, 2017, 10:47 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57973/
> ---
> 
> (Updated March 27, 2017, 10:47 p.m.)
> 
> 
> Review request for mesos, John Kordich, Joseph Wu, Li Li, and Michael Park.
> 
> 
> Bugs: MESOS-6868 and MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6868
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit adds a Windows specific actor for managing job objects.
> 
> A subprocess launched with the `ParentHook::CREATE_JOB()` is created
> within the context of a named Windows job object. The `JobObjectManager`
> takes ownership of the handle to the job object.
> 
> It is necessary to tie the lifetime of the job object to the actor by
> ownership of the open handle so that the job object can be queried for
> usage information even after the processes that were running within the
> job object have ended. These semantics were not changed; previously the
> same was achieved by leaking the handle and tieing it to the lifetime of
> the actual Mesos agent process, and implicitly depending on the
> operating system to close the open handle at the death of the process.
> 
> We ensure the proper death of the job object process group by defering a
> call to `cleanup()` to the process reaper for the given PID. This
> function uses the Windows system call to terminate the job object via
> `os::kill_job()`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/windows/jobobject.hpp PRE-CREATION 
>   3rdparty/libprocess/src/process.cpp 
> f6ee24e2db43d63d91222549efee85421bbf9bf3 
>   3rdparty/libprocess/src/subprocess.cpp 
> 6dfb939ec151f724d383870a806ca3fc8fb0d931 
> 
> 
> Diff: https://reviews.apache.org/r/57973/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 57975: Windows: Use Subprocess in default executor.

[Joseph Wu]

> On March 29, 2017, 2:40 p.m., Joseph Wu wrote:
> > src/launcher/executor.hpp
> > Lines 21-27 (original), 21 (patched)
> > 
> >
> > There's going to be a bunch of code movement...
> > 
> > But now that the `launcher/windows/executor.*` are disappearing, all 
> > the common code should be moved into this file.  And a corresponding .cpp 
> > file.

And now I see the next review :D https://reviews.apache.org/r/57976

Consider squashing the two together.


- Joseph


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


On March 27, 2017, 3:39 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57975/
> ---
> 
> (Updated March 27, 2017, 3:39 p.m.)
> 
> 
> Review request for mesos, John Kordich, Joseph Wu, Li Li, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By encapsulating the job object logic inside the Windows libprocess
> actor, we're able to reuse `subprocess` for launching tasks on Windows.
> This allows us to remove the entirety of `launchTaskWindows` and instead
> reuse `launchTaskPosix`, which just uses `subprocess`. This also fixed
> the `CommitSuicideOnTaskFailure` test, which is now enabled.
> 
> 
> Diffs
> -
> 
>   src/launcher/CMakeLists.txt f63f544f92924b92ef41382c40acabef59a56d8b 
>   src/launcher/executor.hpp c7c134aed26d2116295d66100b3d6efaf610736c 
>   src/launcher/executor.cpp a4bac9d932b6574eb2e1b4a27aa44eaaca6aaa62 
>   src/launcher/posix/executor.hpp 2dd9766aa5b6e0550269ccaa79209d0a483fee76 
>   src/launcher/posix/executor.cpp 7c4ef10390e7ecfe63e2fd0c813f91c896fc7a8d 
>   src/launcher/windows/executor.hpp 6f02912c477105819b4c27ddf248b7289799eaa0 
>   src/launcher/windows/executor.cpp b51fde7376c2083119f342ea599b446944ede000 
>   src/tests/default_executor_tests.cpp 
> 6dadd8937eb6809bcb0aca55fab1cb7f17c3262f 
> 
> 
> Diff: https://reviews.apache.org/r/57975/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 57975: Windows: Use Subprocess in default executor.

[Joseph Wu]

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




src/launcher/CMakeLists.txt
Lines 29-39 (original), 29-39 (patched)


No need for ifdef-ing here anymore.



src/launcher/executor.hpp
Lines 21-27 (original), 21 (patched)


There's going to be a bunch of code movement...

But now that the `launcher/windows/executor.*` are disappearing, all the 
common code should be moved into this file.  And a corresponding .cpp file.



src/launcher/executor.cpp
Line 519 (original), 506 (patched)


This can probably be called `launchTaskAsSubprocess` now.


- Joseph Wu


On March 27, 2017, 3:39 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57975/
> ---
> 
> (Updated March 27, 2017, 3:39 p.m.)
> 
> 
> Review request for mesos, John Kordich, Joseph Wu, Li Li, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By encapsulating the job object logic inside the Windows libprocess
> actor, we're able to reuse `subprocess` for launching tasks on Windows.
> This allows us to remove the entirety of `launchTaskWindows` and instead
> reuse `launchTaskPosix`, which just uses `subprocess`. This also fixed
> the `CommitSuicideOnTaskFailure` test, which is now enabled.
> 
> 
> Diffs
> -
> 
>   src/launcher/CMakeLists.txt f63f544f92924b92ef41382c40acabef59a56d8b 
>   src/launcher/executor.hpp c7c134aed26d2116295d66100b3d6efaf610736c 
>   src/launcher/executor.cpp a4bac9d932b6574eb2e1b4a27aa44eaaca6aaa62 
>   src/launcher/posix/executor.hpp 2dd9766aa5b6e0550269ccaa79209d0a483fee76 
>   src/launcher/posix/executor.cpp 7c4ef10390e7ecfe63e2fd0c813f91c896fc7a8d 
>   src/launcher/windows/executor.hpp 6f02912c477105819b4c27ddf248b7289799eaa0 
>   src/launcher/windows/executor.cpp b51fde7376c2083119f342ea599b446944ede000 
>   src/tests/default_executor_tests.cpp 
> 6dadd8937eb6809bcb0aca55fab1cb7f17c3262f 
> 
> 
> Diff: https://reviews.apache.org/r/57975/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 57973: Windows: Add `JobObjectManager` actor.

[Joseph Wu]

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




3rdparty/libprocess/include/process/windows/jobobject.hpp
Lines 33 (patched)


Can you move this comment down to the `extern` below?



3rdparty/libprocess/include/process/windows/jobobject.hpp
Lines 48-49 (patched)


Try this:
```
process::reap(pid)
  .onAny(defer(self(), &Self::cleanup, lambda::_1, pid));
```



3rdparty/libprocess/include/process/windows/jobobject.hpp
Lines 53 (patched)


This shouldn't be a virtual function, presumably.



3rdparty/libprocess/include/process/windows/jobobject.hpp
Lines 61-65 (patched)


Hm... we need to clean these up eventually...

What sort of statistics will be gathered?  And how long does the job object 
need to live once gathered?



3rdparty/libprocess/include/process/windows/jobobject.hpp
Lines 91-92 (patched)


Outdated comment referring to the launcher here.



3rdparty/libprocess/src/process.cpp
Lines 1211 (patched)


Perhaps add `(Windows only)` here.


- Joseph Wu


On March 27, 2017, 3:47 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57973/
> ---
> 
> (Updated March 27, 2017, 3:47 p.m.)
> 
> 
> Review request for mesos, John Kordich, Joseph Wu, Li Li, and Michael Park.
> 
> 
> Bugs: MESOS-6868 and MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6868
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit adds a Windows specific actor for managing job objects.
> 
> A subprocess launched with the `ParentHook::CREATE_JOB()` is created
> within the context of a named Windows job object. The `JobObjectManager`
> takes ownership of the handle to the job object.
> 
> It is necessary to tie the lifetime of the job object to the actor by
> ownership of the open handle so that the job object can be queried for
> usage information even after the processes that were running within the
> job object have ended. These semantics were not changed; previously the
> same was achieved by leaking the handle and tieing it to the lifetime of
> the actual Mesos agent process, and implicitly depending on the
> operating system to close the open handle at the death of the process.
> 
> We ensure the proper death of the job object process group by defering a
> call to `cleanup()` to the process reaper for the given PID. This
> function uses the Windows system call to terminate the job object via
> `os::kill_job()`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/windows/jobobject.hpp PRE-CREATION 
>   3rdparty/libprocess/src/process.cpp 
> f6ee24e2db43d63d91222549efee85421bbf9bf3 
>   3rdparty/libprocess/src/subprocess.cpp 
> 6dfb939ec151f724d383870a806ca3fc8fb0d931 
> 
> 
> Diff: https://reviews.apache.org/r/57973/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 56367: Windows: Stout: Shim `os::killtree` to terminate job objects.

[Andrew Schwartzmeyer]

> On March 29, 2017, 9:09 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/os/windows/killtree.hpp
> > Lines 41 (patched)
> > 
> >
> > A little note on Error conventions:
> > 
> > We generally construct error messages like:
> > ```
> > return Error("Failed to determine job object name: " + name.error());
> > ```
> > 
> > Basically, we concatenate messages with colons (no period at the end 
> > either).

Oh, thanks.


> On March 29, 2017, 9:09 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/os/windows/killtree.hpp
> > Line 41 (original), 44 (patched)
> > 
> >
> > NOTE: For notes, the convention is to format notes like:
> > ```
> > // NOTE: Text of thing to note goes here.
> > ```

Ah thanks.


- Andrew


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


On March 27, 2017, 10:42 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56367/
> ---
> 
> (Updated March 27, 2017, 10:42 p.m.)
> 
> 
> Review request for mesos, John Kordich, Joseph Wu, Li Li, and Michael Park.
> 
> 
> Bugs: MESOS-6868
> https://issues.apache.org/jira/browse/MESOS-6868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> On Windows, a "process tree" is in fact a job object. Thus, the
> implementation of `os::killtree` on Windows is a shim which terminates
> the job object corresponding to the process group represented by the
> given PID.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/killtree.hpp 
> 2be2f8c3d58ee64410f87ee4a4b2bb54fe014748 
> 
> 
> Diff: https://reviews.apache.org/r/56367/diff/2/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 56364: Windows: Stout: Rewrite job object wrappers.

[Andrew Schwartzmeyer]

> On March 29, 2017, 9 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp
> > Lines 781 (patched)
> > 
> >
> > This comment is a tiny bit too long (> 80 characters).

Ah I really need to get VS Code to highlight lines longer than 80 char for me 
(and use our clang format rules, and highlight syntax on the fly... so much).


- Andrew


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


On March 27, 2017, 10:43 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56364/
> ---
> 
> (Updated March 27, 2017, 10:43 p.m.)
> 
> 
> Review request for mesos, John Kordich, Joseph Wu, Li Li, and Michael Park.
> 
> 
> Bugs: MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `os::create_job` now returns a `Try` instead of a raw
> `HANDLE`, forcing ownership of the job object handle onto the caller
> of the function. `create_job` requires a `std::string name` for the
> job object, which is mapped from a PID using `os::name_job`.
> 
> The assignment of a process to the job object is now done via
> `Try os::assign_job(SharedHandle, pid_t)`.
> 
> The equivalent of killing a process tree with job object semantics
> is simply to terminate the job object. This is done via
> `os::kill_job(SharedHandle)`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 0bedb2d63f5b36afdac2b5a29986f38be96b7c16 
> 
> 
> Diff: https://reviews.apache.org/r/56364/diff/4/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 56367: Windows: Stout: Shim `os::killtree` to terminate job objects.

[Joseph Wu]

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




3rdparty/stout/include/stout/os/windows/killtree.hpp
Lines 26-27 (original), 28-29 (patched)


These comments (break the 4th wall ;) talk about things above Stout.  I can 
generalize them.



3rdparty/stout/include/stout/os/windows/killtree.hpp
Lines 41 (patched)


A little note on Error conventions:

We generally construct error messages like:
```
return Error("Failed to determine job object name: " + name.error());
```

Basically, we concatenate messages with colons (no period at the end 
either).



3rdparty/stout/include/stout/os/windows/killtree.hpp
Line 41 (original), 44 (patched)


NOTE: For notes, the convention is to format notes like:
```
// NOTE: Text of thing to note goes here.
```



3rdparty/stout/include/stout/os/windows/killtree.hpp
Lines 46-47 (patched)


Similar comment that speaks about libprocess.



3rdparty/stout/include/stout/os/windows/killtree.hpp
Lines 51 (patched)


Ditto about error messages here.

```
return Error("Failed to open job object: " + handle.error());
```


- Joseph Wu


On March 27, 2017, 3:42 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56367/
> ---
> 
> (Updated March 27, 2017, 3:42 p.m.)
> 
> 
> Review request for mesos, John Kordich, Joseph Wu, Li Li, and Michael Park.
> 
> 
> Bugs: MESOS-6868
> https://issues.apache.org/jira/browse/MESOS-6868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> On Windows, a "process tree" is in fact a job object. Thus, the
> implementation of `os::killtree` on Windows is a shim which terminates
> the job object corresponding to the process group represented by the
> given PID.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/killtree.hpp 
> 2be2f8c3d58ee64410f87ee4a4b2bb54fe014748 
> 
> 
> Diff: https://reviews.apache.org/r/56367/diff/2/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 58044: Removed unused "using" statements.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58037, 58038, 58039, 58040, 58041, 58042, 58043, 58044]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On March 29, 2017, 7:43 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58044/
> ---
> 
> (Updated March 29, 2017, 7:43 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed unused "using" statements.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  1be8c23e62dee7fb2f9bca23e550ab5884ad8b63 
>   src/slave/slave.cpp c8479d7e8eb915284f0ea8cf75f47acd679dee7e 
>   src/tests/http_fault_tolerance_tests.cpp 
> 4040d24e4b910ebda9337f760633e64ede06acf8 
>   src/tests/partition_tests.cpp 3de37d272389f4e33cc246ac2654728ddf59016d 
>   src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6 
> 
> 
> Diff: https://reviews.apache.org/r/58044/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 56364: Windows: Stout: Rewrite job object wrappers.

[Joseph Wu]

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


Ship it!




Looks great!  

I can fix the one little thing below.


3rdparty/stout/include/stout/windows/os.hpp
Lines 781 (patched)


This comment is a tiny bit too long (> 80 characters).


- Joseph Wu


On March 27, 2017, 3:43 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56364/
> ---
> 
> (Updated March 27, 2017, 3:43 p.m.)
> 
> 
> Review request for mesos, John Kordich, Joseph Wu, Li Li, and Michael Park.
> 
> 
> Bugs: MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `os::create_job` now returns a `Try` instead of a raw
> `HANDLE`, forcing ownership of the job object handle onto the caller
> of the function. `create_job` requires a `std::string name` for the
> job object, which is mapped from a PID using `os::name_job`.
> 
> The assignment of a process to the job object is now done via
> `Try os::assign_job(SharedHandle, pid_t)`.
> 
> The equivalent of killing a process tree with job object semantics
> is simply to terminate the job object. This is done via
> `os::kill_job(SharedHandle)`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 0bedb2d63f5b36afdac2b5a29986f38be96b7c16 
> 
> 
> Diff: https://reviews.apache.org/r/56364/diff/4/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 57911: Added UNKNOWN DiskInfo.Source type.

[Benjamin Bannier]

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

(Updated March 29, 2017, 10:53 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Removed a stray `if` block left after rebase.


Repository: mesos


Description
---

We introduce an explicit UNKNOWN enum kind to allow explicit handling
of unknown enum values (e.g., when the sending and receiving end use
different versions of a message using the enum).

This commit also migrates pattern matching of values of this enum from
if statements to switch statements so that compiler diagnostics can be
used to identify unhandled cases when other types are added in the
future.


Diffs (updated)
-

  include/mesos/mesos.proto 9a66967ab459f75f21faf21be644f39b3fad670b 
  include/mesos/v1/mesos.proto 115f1b4d9e129f83a3aed62a95eb11faa12e04d1 
  src/common/resources.cpp ca1add1dbbe04fa775004010ebc847254c198bd7 
  src/slave/paths.cpp 38ad1993aa36a627ec97a7865488677495ee4c5a 
  src/slave/slave.cpp c8479d7e8eb915284f0ea8cf75f47acd679dee7e 
  src/v1/resources.cpp 6008a9d2572d04612f0da2d54834be548d366f29 


Diff: https://reviews.apache.org/r/57911/diff/4/

Changes: https://reviews.apache.org/r/57911/diff/3-4/


Testing
---

make check (OS X, Linux)


Thanks,

Benjamin Bannier

Re: Review Request 58038: Removed redundant call to `std::string::c_str()` in Mesos.

[Benjamin Bannier]

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



Could you add `readability-redundant-string-cstr` to the default `CHECKS` in 
`support/mesos-tidy.sh`?

I am not exactly sure what checks you used in other patches in this chain, 
could you update their summaries to mention them?

- Benjamin Bannier


On March 29, 2017, 9:41 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58038/
> ---
> 
> (Updated March 29, 2017, 9:41 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Spotted via clang-tidy.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> 13a1f131973d15225e100b04f8e24c1f9f4d8539 
> 
> 
> Diff: https://reviews.apache.org/r/58038/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58014: CMake: Setup cotire for 'mesos-agent' target.

[Joseph Wu]

> On March 29, 2017, 12:30 p.m., Andrew Schwartzmeyer wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 38 (patched)
> > 
> >
> > Are we going to enable verbose Cotire logging? It's useful.
> 
> Andrew Schwartzmeyer wrote:
> (Or note for users that `-DCOTIRE_VERBOSE=1` does the trick.

I'll add `set(COTIRE_VERBOSE ${VERBOSE})`


> On March 29, 2017, 12:30 p.m., Andrew Schwartzmeyer wrote:
> > src/CMakeLists.txt
> > Lines 541 (patched)
> > 
> >
> > Can we add a comment explaining why these specific files are excluded? 
> > (As the reasons are various.)

I can add this before committing.


> On March 29, 2017, 12:30 p.m., Andrew Schwartzmeyer wrote:
> > src/CMakeLists.txt
> > Lines 549 (patched)
> > 
> >
> > Can there be a comment explaining why we use this particular header 
> > path versus alternatives?

I can add this before committing.

(Basically, we mostly care about the agent on Windows, so the sources come from 
the agent.)


> On March 29, 2017, 12:30 p.m., Andrew Schwartzmeyer wrote:
> > src/CMakeLists.txt
> > Lines 551 (patched)
> > 
> >
> > When are we going to cotire other targets too?

This can be done on a target-by-target basis.  Libmesos is the biggest chunk 
though, so who knows?


- Joseph


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


On March 28, 2017, 5:56 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58014/
> ---
> 
> (Updated March 28, 2017, 5:56 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This sets several cotire variables in the 'src' directory to enable
> pre-compiled headers for the 'mesos-agent' target. The excluded
> headers were removed due to namespace issues or breaking incremental
> builds.
> 
> All cotire variables are conditionally set if option
> ENABLE_PRECOMPILED_HEADERS is set. In this commit, this is ON for
> WIN32 builds, off for all others.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
>   src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
> 
> 
> Diff: https://reviews.apache.org/r/58014/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58014: CMake: Setup cotire for 'mesos-agent' target.

[Andrew Schwartzmeyer]

> On March 29, 2017, 7:30 p.m., Andrew Schwartzmeyer wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 38 (patched)
> > 
> >
> > Are we going to enable verbose Cotire logging? It's useful.

(Or note for users that `-DCOTIRE_VERBOSE=1` does the trick.


- Andrew


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


On March 29, 2017, 12:56 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58014/
> ---
> 
> (Updated March 29, 2017, 12:56 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This sets several cotire variables in the 'src' directory to enable
> pre-compiled headers for the 'mesos-agent' target. The excluded
> headers were removed due to namespace issues or breaking incremental
> builds.
> 
> All cotire variables are conditionally set if option
> ENABLE_PRECOMPILED_HEADERS is set. In this commit, this is ON for
> WIN32 builds, off for all others.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
>   src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
> 
> 
> Diff: https://reviews.apache.org/r/58014/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58014: CMake: Setup cotire for 'mesos-agent' target.

[Andrew Schwartzmeyer]

> On March 29, 2017, 6:54 p.m., Joseph Wu wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 39-40 (patched)
> > 
> >
> > I'll add a comment explaining unity builds:
> > ```
> >   # By default Cotire generates both precompiled headers and a "unity" 
> > build.
> >   # A unity build is where all the source files in a target are 
> > combined into
> >   # a single source file to reduce the number of files that need to be 
> > opened
> >   # and read. We disable "unity" builds for now.
> > ```
> 
> Andrew Schwartzmeyer wrote:
> I'd also add why we don't use Unity builds (they're a terrible idea that 
> should never be used haha).
> 
> Joseph Wu wrote:
> Hmm...  I don't have a good reason for this, other than "the build will 
> probably OOM if we enable this" :)

(And it doesn't work at all! Too many collisions.)


- Andrew


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


On March 29, 2017, 12:56 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58014/
> ---
> 
> (Updated March 29, 2017, 12:56 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This sets several cotire variables in the 'src' directory to enable
> pre-compiled headers for the 'mesos-agent' target. The excluded
> headers were removed due to namespace issues or breaking incremental
> builds.
> 
> All cotire variables are conditionally set if option
> ENABLE_PRECOMPILED_HEADERS is set. In this commit, this is ON for
> WIN32 builds, off for all others.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
>   src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
> 
> 
> Diff: https://reviews.apache.org/r/58014/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58014: CMake: Setup cotire for 'mesos-agent' target.

[Joseph Wu]

> On March 29, 2017, 11:54 a.m., Joseph Wu wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 34-36 (patched)
> > 
> >
> > I think we can remove this check, as it would block people that want to 
> > fix the compilation issues on non-Windows + PCHs.
> > 
> > For now, a WARNING should be sufficient.
> 
> Andrew Schwartzmeyer wrote:
> Sure, but a dev fixing compilation issues would also need to disable the 
> warning when it's supported. What I'm saying is, they'll see it either way 
> and have to change it too. A `FATAL_ERROR` seems safer to keep people from 
> complaining.

Sounds reasonable.  I'll open and reference this issue to track non-Windows 
PCHs: https://issues.apache.org/jira/browse/MESOS-7322


> On March 29, 2017, 11:54 a.m., Joseph Wu wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 39-40 (patched)
> > 
> >
> > I'll add a comment explaining unity builds:
> > ```
> >   # By default Cotire generates both precompiled headers and a "unity" 
> > build.
> >   # A unity build is where all the source files in a target are 
> > combined into
> >   # a single source file to reduce the number of files that need to be 
> > opened
> >   # and read. We disable "unity" builds for now.
> > ```
> 
> Andrew Schwartzmeyer wrote:
> I'd also add why we don't use Unity builds (they're a terrible idea that 
> should never be used haha).

Hmm...  I don't have a good reason for this, other than "the build will 
probably OOM if we enable this" :)


- Joseph


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


On March 28, 2017, 5:56 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58014/
> ---
> 
> (Updated March 28, 2017, 5:56 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This sets several cotire variables in the 'src' directory to enable
> pre-compiled headers for the 'mesos-agent' target. The excluded
> headers were removed due to namespace issues or breaking incremental
> builds.
> 
> All cotire variables are conditionally set if option
> ENABLE_PRECOMPILED_HEADERS is set. In this commit, this is ON for
> WIN32 builds, off for all others.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
>   src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
> 
> 
> Diff: https://reviews.apache.org/r/58014/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58014: CMake: Setup cotire for 'mesos-agent' target.

[Joseph Wu]

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




src/CMakeLists.txt
Lines 545 (patched)


Looks like this file also introduces some nasty namespace conflicts, so 
I'll exclude it before committing:
```

"${CMAKE_SOURCE_DIR}/include/mesos/authentication/http/combined_authenticator.hpp"
```


- Joseph Wu


On March 28, 2017, 5:56 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58014/
> ---
> 
> (Updated March 28, 2017, 5:56 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This sets several cotire variables in the 'src' directory to enable
> pre-compiled headers for the 'mesos-agent' target. The excluded
> headers were removed due to namespace issues or breaking incremental
> builds.
> 
> All cotire variables are conditionally set if option
> ENABLE_PRECOMPILED_HEADERS is set. In this commit, this is ON for
> WIN32 builds, off for all others.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
>   src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
> 
> 
> Diff: https://reviews.apache.org/r/58014/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58011: CMake: Add Cotire module (version 1.7.9).

[Joseph Wu]

> On March 29, 2017, 12:34 p.m., Andrew Schwartzmeyer wrote:
> > Can we record what commit of upstream cotire this file got pulled from (or 
> > tag if we did indeed pull from a release)? I realize the module has a 
> > version in it, but I don't trust that it gurantees the file didn't come 
> > from tip of master.

I've already added the SHA to the commit I have staged for committing :)


- Joseph


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


On March 28, 2017, 5:54 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58011/
> ---
> 
> (Updated March 28, 2017, 5:54 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add CMake's cotire module to allow for use of PCH (precompiled
> headers) during compilation, reducing compilation speed.
> 
> 
> Diffs
> -
> 
>   3rdparty/cmake/cotire.cmake PRE-CREATION 
>   CMakeLists.txt 0d1e17b863856287b18fd026e1ae71ec35e5ad83 
>   LICENSE f11970cada909e7bbb685f96cb9ef0ff24e01924 
> 
> 
> Diff: https://reviews.apache.org/r/58011/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Review Request 58042: Remove unused include in libprocess.

[Neil Conway]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

Remove unused include in libprocess.


Diffs
-

  3rdparty/libprocess/src/tests/process_tests.cpp 
287eb5577896b9d2c6ea7b49247ab54f66e66de8 


Diff: https://reviews.apache.org/r/58042/diff/1/


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 58044: Removed unused "using" statements.

[Neil Conway]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

Removed unused "using" statements.


Diffs
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 1be8c23e62dee7fb2f9bca23e550ab5884ad8b63 
  src/slave/slave.cpp c8479d7e8eb915284f0ea8cf75f47acd679dee7e 
  src/tests/http_fault_tolerance_tests.cpp 
4040d24e4b910ebda9337f760633e64ede06acf8 
  src/tests/partition_tests.cpp 3de37d272389f4e33cc246ac2654728ddf59016d 
  src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6 


Diff: https://reviews.apache.org/r/58044/diff/1/


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 58043: Cleanup usage of namespace-qualified identifiers.

[Neil Conway]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

Cleanup usage of namespace-qualified identifiers.


Diffs
-

  src/checks/checker.cpp 314354cc374b453ec12e25e3d4730a92697468cf 
  src/tests/default_executor_tests.cpp 6dadd8937eb6809bcb0aca55fab1cb7f17c3262f 


Diff: https://reviews.apache.org/r/58043/diff/1/


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 58041: Don't mark a value parameter `const`.

[Neil Conway]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

This has no effect on the function's signature.

Spotted via clang-tidy.


Diffs
-

  src/tests/fetcher_cache_tests.cpp 49173af9fa1537b28784ae7543458e5b57e5180f 


Diff: https://reviews.apache.org/r/58041/diff/1/


Testing
---

`make check`

Note that we could make similar changes in more places, but I just fixed this 
one spot for now.


Thanks,

Neil Conway

Review Request 58040: Avoid various redundant copies.

[Neil Conway]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

Spotted via clang-tidy.


Diffs
-

  src/examples/dynamic_reservation_framework.cpp 
c1650dc978dddf14cf1e22a33752b58b06914ff4 
  src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
  src/master/master.cpp ab071f56a66ae66c97a185b2377a9cfe9a5c1ade 
  src/oci/spec.cpp 06fceb4f0441431d756eb45c8aaf9730ef9e248a 
  src/tests/default_executor_tests.cpp 6dadd8937eb6809bcb0aca55fab1cb7f17c3262f 


Diff: https://reviews.apache.org/r/58040/diff/1/


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 58039: Avoid redundant copies when using `std::get` on a tuple.

[Neil Conway]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

`std::get` returns a reference; in most cases we can avoid a copy at the
call site by assigning the result to a const reference.

Spotted via clang-tidy.


Diffs
-

  src/checks/health_checker.cpp 2211228f7aa0228af64d8fce6c5f2dd1847328f9 
  src/common/command_utils.cpp cb9de720814c7c69758d167fbe6292f12cd73224 
  src/hdfs/hdfs.cpp 93450b9c18198c5e841ba9dc7ec6e903cf119f89 
  src/linux/perf.cpp 2271564eeb6c8e5b7dca1461f3f94f1c12b35a1a 
  src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp 
8cc1b12624ce92afa9e2d535cfbdebf5cc3cae35 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
6e95315b70a5d9d3b4b21c4cf235b0a483760190 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 1be8c23e62dee7fb2f9bca23e550ab5884ad8b63 
  src/slave/containerizer/mesos/isolators/posix/disk.cpp 
805566ca9c346e9c1987f804bcc5d111886eb729 
  src/uri/fetchers/copy.cpp 9c79ac6075a8350b52676d5e219491ded3d3625e 
  src/uri/fetchers/curl.cpp a592eb501fda859764d5f14d866032ca8ca3dc97 
  src/uri/fetchers/docker.cpp d051a4d534c24ef6c4dd98181a0f2a175c69d0f9 


Diff: https://reviews.apache.org/r/58039/diff/1/


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 58038: Removed redundant call to `std::string::c_str()` in Mesos.

[Neil Conway]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

Spotted via clang-tidy.


Diffs
-

  src/tests/containerizer/docker_volume_isolator_tests.cpp 
13a1f131973d15225e100b04f8e24c1f9f4d8539 


Diff: https://reviews.apache.org/r/58038/diff/1/


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 58037: Removed redundant call to `std::string::c_str()` in stout.

[Neil Conway]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

Spotted via clang-tidy.


Diffs
-

  3rdparty/stout/include/stout/elf.hpp da2ce0c83372070a1050b8adb4c16a636f63e5b4 


Diff: https://reviews.apache.org/r/58037/diff/1/


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 58014: CMake: Setup cotire for 'mesos-agent' target.

[Andrew Schwartzmeyer]

> On March 29, 2017, 6:54 p.m., Joseph Wu wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 32 (patched)
> > 
> >
> > You need to surround `WIN32` with `${}`.  Otherwise, this will default 
> > to `OFF` on Windows.
> > 
> > You probably didn't catch this because CMake caches options.  If you 
> > call `cmake .. -DENABLE_PRECOMPILED_HEADERS=1` in the past, then all future 
> > builds will use PCHs.  The default values for options only take effect if 
> > you delete the `CMakeCache.txt` file.

I remember saying we need to be sure to test this ;). Looking around the file 
it's not clear to me if `${}` is required only in strings i.e. `"${VAR}"` or 
everywhere (since there are certainly naked variables in use without the 
braces).

Edit: I double checked [syntax 
rules](https://cmake.org/Wiki/CMake/Language_Syntax). `${x}` is required for 
substitution; otherwise definition is tested (is it set or unset, but not what 
it is). Annoying, but now I know.


> On March 29, 2017, 6:54 p.m., Joseph Wu wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 34-36 (patched)
> > 
> >
> > I think we can remove this check, as it would block people that want to 
> > fix the compilation issues on non-Windows + PCHs.
> > 
> > For now, a WARNING should be sufficient.

Sure, but a dev fixing compilation issues would also need to disable the 
warning when it's supported. What I'm saying is, they'll see it either way and 
have to change it too. A `FATAL_ERROR` seems safer to keep people from 
complaining.


> On March 29, 2017, 6:54 p.m., Joseph Wu wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 39-40 (patched)
> > 
> >
> > I'll add a comment explaining unity builds:
> > ```
> >   # By default Cotire generates both precompiled headers and a "unity" 
> > build.
> >   # A unity build is where all the source files in a target are 
> > combined into
> >   # a single source file to reduce the number of files that need to be 
> > opened
> >   # and read. We disable "unity" builds for now.
> > ```

I'd also add why we don't use Unity builds (they're a terrible idea that should 
never be used haha).


- Andrew


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


On March 29, 2017, 12:56 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58014/
> ---
> 
> (Updated March 29, 2017, 12:56 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This sets several cotire variables in the 'src' directory to enable
> pre-compiled headers for the 'mesos-agent' target. The excluded
> headers were removed due to namespace issues or breaking incremental
> builds.
> 
> All cotire variables are conditionally set if option
> ENABLE_PRECOMPILED_HEADERS is set. In this commit, this is ON for
> WIN32 builds, off for all others.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
>   src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
> 
> 
> Diff: https://reviews.apache.org/r/58014/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58015: Add name to contributors.yaml file for first contribution.

[Andrew Schwartzmeyer]

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




docs/contributors.yaml
Lines 355 (patched)


Is this the email you're authoring with?


- Andrew Schwartzmeyer


On March 29, 2017, 1 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58015/
> ---
> 
> (Updated March 29, 2017, 1 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add name to contributors.yaml file for first contribution.
> 
> 
> Diffs
> -
> 
>   docs/contributors.yaml 725f50c3d781b187dc61039e84fdd4ada6d874fa 
> 
> 
> Diff: https://reviews.apache.org/r/58015/diff/1/
> 
> 
> Testing
> ---
> 
> ## CentOS 7
> 
> Full build with all changes, made sure build was successful with no errors.
> 
> ## Windows
> 
> Full build with all changes, made sure build was successful with no errors.
> 
> 
> ## Timing improvements with this change
> 
>  Build times on Windows (no PCH):
>   1. Full build time (everything including 3rd party products): 24:49.47 
> (1489.47s total)
>   2. Time to rebuild all of Mesos itself: 21:8.63 (1268.63s total)
>   3. Time for a build with no changes: 0:30.47 (30.47s total)
>   4. Modified file 3rdparty/stout/include/stout/os/os.hpp (added a comment)
>   5. Time for an incremental build: 36:55.55 (2215.55s total)
>  (Very odd that an incremental takes longer than a full build!)
>   6. Time for an incremental build (just mesos-agent): 0:14:24.73 (864.73s 
> total)
>   7. Time for an incremental build, mesos-agent only: 0:16:42.57 (1002.57s 
> total)
>  (Includes speedup to linker flags)
> 
> 
>  Build times on Windows (with PCH):
>   1. Full build time (everything including 3rd party products): 19:54.49 
> (1194.49s total)
>   2. Time to rebuild all of Mesos itself: 0:15:49.58 (949.58s total)
>   3. Time for a build with no changes: 0:0:21.72 (21.72s total)
>   4. Modified file 3rdparty/stout/include/stout/os/os.hpp (added a comment)
>   5. Time for an incremental build: 0:34:40.64 (2080.64s total)
>   6. Time for an incremental build (just mesos-agent): 0:11:53.57 (713.57s 
> total)
>   7. Time for an incremental build, mesos-agent only: 0:10:43.10 (643.10s 
> total)
>  (Includes speedup to linker flags)
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58011: CMake: Add Cotire module (version 1.7.9).

[Andrew Schwartzmeyer]

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



Can we record what commit of upstream cotire this file got pulled from (or tag 
if we did indeed pull from a release)? I realize the module has a version in 
it, but I don't trust that it gurantees the file didn't come from tip of master.

- Andrew Schwartzmeyer


On March 29, 2017, 12:54 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58011/
> ---
> 
> (Updated March 29, 2017, 12:54 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add CMake's cotire module to allow for use of PCH (precompiled
> headers) during compilation, reducing compilation speed.
> 
> 
> Diffs
> -
> 
>   3rdparty/cmake/cotire.cmake PRE-CREATION 
>   CMakeLists.txt 0d1e17b863856287b18fd026e1ae71ec35e5ad83 
>   LICENSE f11970cada909e7bbb685f96cb9ef0ff24e01924 
> 
> 
> Diff: https://reviews.apache.org/r/58011/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58012: Fix code issues to facilitate use of precompiled headers on Windows.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On March 29, 2017, 12:54 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58012/
> ---
> 
> (Updated March 29, 2017, 12:54 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Precompiled headers often include new include files that were not
> otherwise included in a module. If two include files have the same
> class name (with different scopoing), this causes a conflict.
> 
> Fully qualify class names and fix other code issues to allow for
> precompiled headers on Windows.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.cpp 5ef23067c3f03c7972e24968d4a84b3cf61726a0 
>   src/slave/containerizer/docker.cpp dfab26224e8679b493d770657f83c711a1d442ef 
>   src/slave/windows_ctrlhandler.hpp 25b14a446175cd302d1840b1f542989ef8315018 
> 
> 
> Diff: https://reviews.apache.org/r/58012/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58013: WIN32: Add compile/link flags to improve incremental link times.

[Andrew Schwartzmeyer]

> On March 29, 2017, 6:53 p.m., Joseph Wu wrote:
> > After double-checking what these flags actually do, I'm going to
> > add some more information to the commit description:
> > ```
> > The `/Zc:inline` flag tells the compiler to generate object files
> > but exclude symbols that are either unreferenced or have internal
> > linkage only.  This leads to smaller object files and faster linking.
> > See: https://msdn.microsoft.com/en-us/library/dn642448.aspx
> > 
> > The `/debug:FASTLINK` flag changes how the linker generates
> > program database files (PDB).  Instead of making a full copy of any
> > related debug information used by the library, this option instead
> > creates indexes to other PDBs, thereby cutting down on link-time
> > code generation.  This only affects DEBUG builds.
> > See: https://msdn.microsoft.com/en-us/library/xe4t6fc1.aspx
> > ```
> 
> Andrew Schwartzmeyer wrote:
> Yes please :)

Nit pick on title: I've been following the seeming convention of `Windows: ` 
prefix instead of `WIN32: `. Joe, can you fix this when you commit?


- Andrew


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


On March 29, 2017, 12:55 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58013/
> ---
> 
> (Updated March 29, 2017, 12:55 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When working on precompiled headers, we recognized that adding
> some compile/link flags resulted in a 20% reduction in incremental
> linkage time. This is NOT directly dependent on precompiled headers
> (the improvement comes regardless of PCH).
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
> 
> 
> Diff: https://reviews.apache.org/r/58013/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58013: WIN32: Add compile/link flags to improve incremental link times.

[Andrew Schwartzmeyer]

> On March 29, 2017, 6:53 p.m., Joseph Wu wrote:
> > After double-checking what these flags actually do, I'm going to
> > add some more information to the commit description:
> > ```
> > The `/Zc:inline` flag tells the compiler to generate object files
> > but exclude symbols that are either unreferenced or have internal
> > linkage only.  This leads to smaller object files and faster linking.
> > See: https://msdn.microsoft.com/en-us/library/dn642448.aspx
> > 
> > The `/debug:FASTLINK` flag changes how the linker generates
> > program database files (PDB).  Instead of making a full copy of any
> > related debug information used by the library, this option instead
> > creates indexes to other PDBs, thereby cutting down on link-time
> > code generation.  This only affects DEBUG builds.
> > See: https://msdn.microsoft.com/en-us/library/xe4t6fc1.aspx
> > ```

Yes please :)


- Andrew


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


On March 29, 2017, 12:55 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58013/
> ---
> 
> (Updated March 29, 2017, 12:55 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When working on precompiled headers, we recognized that adding
> some compile/link flags resulted in a 20% reduction in incremental
> linkage time. This is NOT directly dependent on precompiled headers
> (the improvement comes regardless of PCH).
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
> 
> 
> Diff: https://reviews.apache.org/r/58013/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58014: CMake: Setup cotire for 'mesos-agent' target.

[Andrew Schwartzmeyer]

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




cmake/CompilationConfigure.cmake
Lines 33 (patched)


Nit-pick but we should match tense with the other options (i.e. "enable" 
versus "enabled").



cmake/CompilationConfigure.cmake
Lines 38 (patched)


Are we going to enable verbose Cotire logging? It's useful.



src/CMakeLists.txt
Lines 541 (patched)


Can we add a comment explaining why these specific files are excluded? (As 
the reasons are various.)



src/CMakeLists.txt
Lines 549 (patched)


Can there be a comment explaining why we use this particular header path 
versus alternatives?



src/CMakeLists.txt
Lines 551 (patched)


When are we going to cotire other targets too?


- Andrew Schwartzmeyer


On March 29, 2017, 12:56 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58014/
> ---
> 
> (Updated March 29, 2017, 12:56 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This sets several cotire variables in the 'src' directory to enable
> pre-compiled headers for the 'mesos-agent' target. The excluded
> headers were removed due to namespace issues or breaking incremental
> builds.
> 
> All cotire variables are conditionally set if option
> ENABLE_PRECOMPILED_HEADERS is set. In this commit, this is ON for
> WIN32 builds, off for all others.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
>   src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
> 
> 
> Diff: https://reviews.apache.org/r/58014/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58013: WIN32: Add compile/link flags to improve incremental link times.

[Joseph Wu]

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


Ship it!




After double-checking what these flags actually do, I'm going to
add some more information to the commit description:
```
The `/Zc:inline` flag tells the compiler to generate object files
but exclude symbols that are either unreferenced or have internal
linkage only.  This leads to smaller object files and faster linking.
See: https://msdn.microsoft.com/en-us/library/dn642448.aspx

The `/debug:FASTLINK` flag changes how the linker generates
program database files (PDB).  Instead of making a full copy of any
related debug information used by the library, this option instead
creates indexes to other PDBs, thereby cutting down on link-time
code generation.  This only affects DEBUG builds.
See: https://msdn.microsoft.com/en-us/library/xe4t6fc1.aspx
```

- Joseph Wu


On March 28, 2017, 5:55 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58013/
> ---
> 
> (Updated March 28, 2017, 5:55 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When working on precompiled headers, we recognized that adding
> some compile/link flags resulted in a 20% reduction in incremental
> linkage time. This is NOT directly dependent on precompiled headers
> (the improvement comes regardless of PCH).
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
> 
> 
> Diff: https://reviews.apache.org/r/58013/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58014: CMake: Setup cotire for 'mesos-agent' target.

[Joseph Wu]

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


Fix it, then Ship it!




Only minor issues, so I'll tweak them before testing and committing.


cmake/CompilationConfigure.cmake
Lines 32 (patched)


You need to surround `WIN32` with `${}`.  Otherwise, this will default to 
`OFF` on Windows.

You probably didn't catch this because CMake caches options.  If you call 
`cmake .. -DENABLE_PRECOMPILED_HEADERS=1` in the past, then all future builds 
will use PCHs.  The default values for options only take effect if you delete 
the `CMakeCache.txt` file.



cmake/CompilationConfigure.cmake
Lines 34-36 (patched)


I think we can remove this check, as it would block people that want to fix 
the compilation issues on non-Windows + PCHs.

For now, a WARNING should be sufficient.



cmake/CompilationConfigure.cmake
Lines 39-40 (patched)


I'll add a comment explaining unity builds:
```
  # By default Cotire generates both precompiled headers and a "unity" 
build.
  # A unity build is where all the source files in a target are combined 
into
  # a single source file to reduce the number of files that need to be 
opened
  # and read. We disable "unity" builds for now.
```


- Joseph Wu


On March 28, 2017, 5:56 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58014/
> ---
> 
> (Updated March 28, 2017, 5:56 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This sets several cotire variables in the 'src' directory to enable
> pre-compiled headers for the 'mesos-agent' target. The excluded
> headers were removed due to namespace issues or breaking incremental
> builds.
> 
> All cotire variables are conditionally set if option
> ENABLE_PRECOMPILED_HEADERS is set. In this commit, this is ON for
> WIN32 builds, off for all others.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
>   src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
> 
> 
> Diff: https://reviews.apache.org/r/58014/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58011: CMake: Add Cotire module (version 1.7.9).

[Joseph Wu]

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


Ship it!




I'm going to tweak the commit description because I'm sure you meant "reducing 
compilation time" rather than "reducing compilation speed" ;)

- Joseph Wu


On March 28, 2017, 5:54 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58011/
> ---
> 
> (Updated March 28, 2017, 5:54 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add CMake's cotire module to allow for use of PCH (precompiled
> headers) during compilation, reducing compilation speed.
> 
> 
> Diffs
> -
> 
>   3rdparty/cmake/cotire.cmake PRE-CREATION 
>   CMakeLists.txt 0d1e17b863856287b18fd026e1ae71ec35e5ad83 
>   LICENSE f11970cada909e7bbb685f96cb9ef0ff24e01924 
> 
> 
> Diff: https://reviews.apache.org/r/58011/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58012: Fix code issues to facilitate use of precompiled headers on Windows.

[Joseph Wu]

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


Ship it!




LGTM.

- Joseph Wu


On March 28, 2017, 5:54 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58012/
> ---
> 
> (Updated March 28, 2017, 5:54 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Precompiled headers often include new include files that were not
> otherwise included in a module. If two include files have the same
> class name (with different scopoing), this causes a conflict.
> 
> Fully qualify class names and fix other code issues to allow for
> precompiled headers on Windows.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.cpp 5ef23067c3f03c7972e24968d4a84b3cf61726a0 
>   src/slave/containerizer/docker.cpp dfab26224e8679b493d770657f83c711a1d442ef 
>   src/slave/windows_ctrlhandler.hpp 25b14a446175cd302d1840b1f542989ef8315018 
> 
> 
> Diff: https://reviews.apache.org/r/58012/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 57167: Updated quota handler logic for hierarchical roles.

[Michael Park]

> On March 28, 2017, 4:06 p.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 469-487 (original), 605-618 (patched)
> > 
> >
> > Couldn't this be just:
> > 
> > ```cpp
> > vector components = strings::split(request.url.path, "/", 3u);
> > if (components.size() < 3u) {
> >   return BadRequest(
> >   "Failed to parse request path '" + request.url.path +
> >   "': 3 tokens ('master', 'quota', 'role') required, found " +
> >   stringify(tokens.size()) + " token(s)"));
> > }
> > 
> > CHECK_EQ(3u, components.size());
> > const string& role = components.back();
> > /* ... */
> > ```
> 
> Neil Conway wrote:
> I'm not sure this is actually more readable; leaving as-is for now, will 
> discuss offline.

A summary of our discussion yesterday: the `components.begin() + 3` part looks 
wrong, but is indeed correct
because `split` returns empty tokens, and the first token will be an empty 
string.
This means that my suggestion of `split(..., 3u)` actually has a bug... subtle 
:(.

We also didn't like the splitting/joining of the "rest", because it's not 
obvious that we get back what we put in.

Also, based on the code that I see in libprocess for `route`, it looks like we 
would allow extraneous slashes such as `/master///quota`.
This would mean that the use of `split` actually introduces a 
backwards-incompatibility here.

I personally think it's simplest for us to do 
`strings::tokenize(request.url.path, "/", 3u)`.

What do you think?


- Michael


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


On March 28, 2017, 4:42 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> ---
> 
> (Updated March 28, 2017, 4:42 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children.
> 
> When creating and removing quota, we must ensure that this invariant is
> not violated.
> 
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
> role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
> cluster capacity heuristic.
> 
> 
> Diffs
> -
> 
>   src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67 
>   src/tests/hierarchical_allocator_tests.cpp 
> e343dc37bd7136f0f6dd5dbc22a25cabe715038d 
>   src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837 
> 
> 
> Diff: https://reviews.apache.org/r/57167/diff/9/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58030: Added support for COMMAND checks to the default executor.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57644, 57648, 56288, 55901, 57645, 57646, 57647, 57854, 
57912, 58030]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On March 29, 2017, 4:39 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58030/
> ---
> 
> (Updated March 29, 2017, 4:39 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Bugs: MESOS-7277
> https://issues.apache.org/jira/browse/MESOS-7277
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for COMMAND checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp e8af3160c9fd52ec20acf41b86bade50f4539fb1 
>   src/checks/checker.cpp 314354cc374b453ec12e25e3d4730a92697468cf 
>   src/launcher/default_executor.cpp 606fd9c164052fe8d168969bbb05f6ec99180d3b 
>   src/tests/check_tests.cpp 16f1c07e109e24d475ad593ef1992dfb9f482ba6 
> 
> 
> Diff: https://reviews.apache.org/r/58030/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` under Fedora 24 and macOS including the 4 new tests
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 57898: Windows: Add deprecation warning for VS 2015.

[Jeff Coffler]

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



This looks fine to me. I'll hold off upgrading until this is merged, just to 
double check that it trips properly.

- Jeff Coffler


On March 27, 2017, 5:12 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57898/
> ---
> 
> (Updated March 27, 2017, 5:12 p.m.)
> 
> 
> Review request for mesos, Jeff Coffler, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-7297
> https://issues.apache.org/jira/browse/MESOS-7297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Specifically this checks if the generator used did not match "Visual
> Studio 15 2017", which Mesos prefers. Note that the `CMAKE_GENERATOR`
> variable is expanded fully even when shorthand is used, e.g. `-G"Visual
> Studio 15"` will correctly match.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
> 
> 
> Diff: https://reviews.apache.org/r/57898/diff/1/
> 
> 
> Testing
> ---
> 
> Tested using different generators on Windows. Warning is displayed when using 
> `-G"Visual Studio 14"`, `-G"Visual Studio 14 2015"`, and `-G"Visual Studio 14 
> 2015 Win64"`, but not when using `-G"Visual Studio 15"`, `-G"Visual Studio 15 
> 2017"`, or `-G"Visual Studio 15 2017 Win64"`.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 57912: Enabled pause/resume for checks.

[Gastón Kleiman]

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

(Updated March 29, 2017, 4:40 p.m.)


Review request for mesos, Alexander Rukletsov and Vinod Kone.


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


Repository: mesos


Description
---

Enabled pause/resume for checks.


Diffs
-

  src/checks/checker.hpp e8af3160c9fd52ec20acf41b86bade50f4539fb1 
  src/checks/checker.cpp 314354cc374b453ec12e25e3d4730a92697468cf 
  src/launcher/default_executor.cpp 606fd9c164052fe8d168969bbb05f6ec99180d3b 
  src/launcher/executor.cpp a4bac9d932b6574eb2e1b4a27aa44eaaca6aaa62 


Diff: https://reviews.apache.org/r/57912/diff/2/


Testing
---

`make check` on Fedora 24


Thanks,

Gastón Kleiman

Review Request 58030: Added support for COMMAND checks to the default executor.

[Gastón Kleiman]

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

Review request for mesos, Alexander Rukletsov and Vinod Kone.


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


Repository: mesos


Description
---

Added support for COMMAND checks to the default executor.


Diffs
-

  src/checks/checker.hpp e8af3160c9fd52ec20acf41b86bade50f4539fb1 
  src/checks/checker.cpp 314354cc374b453ec12e25e3d4730a92697468cf 
  src/launcher/default_executor.cpp 606fd9c164052fe8d168969bbb05f6ec99180d3b 
  src/tests/check_tests.cpp 16f1c07e109e24d475ad593ef1992dfb9f482ba6 


Diff: https://reviews.apache.org/r/58030/diff/1/


Testing
---

`make check` under Fedora 24 and macOS including the 4 new tests


Thanks,

Gastón Kleiman

Re: Review Request 57912: Enabled pause/resume for checks.

[Gastón Kleiman]

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

(Updated March 29, 2017, 4:37 p.m.)


Review request for mesos, Alexander Rukletsov and Vinod Kone.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Enabled pause/resume for checks.


Diffs (updated)
-

  src/checks/checker.hpp e8af3160c9fd52ec20acf41b86bade50f4539fb1 
  src/checks/checker.cpp 314354cc374b453ec12e25e3d4730a92697468cf 
  src/launcher/default_executor.cpp 606fd9c164052fe8d168969bbb05f6ec99180d3b 
  src/launcher/executor.cpp a4bac9d932b6574eb2e1b4a27aa44eaaca6aaa62 


Diff: https://reviews.apache.org/r/57912/diff/2/

Changes: https://reviews.apache.org/r/57912/diff/1-2/


Testing
---

`make check` on Fedora 24


Thanks,

Gastón Kleiman

Re: Review Request 57854: Improved log/failure messages in the (health)checker libraries.

[Gastón Kleiman]

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

(Updated March 29, 2017, 4:37 p.m.)


Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, 
and Vinod Kone.


Changes
---

Fixed another message.


Repository: mesos


Description
---

- Made log/failure messages consistent across both libraries.
- Task and container IDs are user generated and can contain spaces, so
  we have to wrap them in single quotes.
- Removed the redundant task IDs from 'Failure' messages.


Diffs (updated)
-

  src/checks/checker.cpp 314354cc374b453ec12e25e3d4730a92697468cf 
  src/checks/health_checker.cpp 2211228f7aa0228af64d8fce6c5f2dd1847328f9 
  src/tests/check_tests.cpp 16f1c07e109e24d475ad593ef1992dfb9f482ba6 


Diff: https://reviews.apache.org/r/57854/diff/8/

Changes: https://reviews.apache.org/r/57854/diff/7-8/


Testing
---

Did some manual testing and looked at the new log messages, they're so much 
nicer now <3.


Thanks,

Gastón Kleiman

Re: Review Request 52379: Added agent flag '--oci_store_dir'.

[Qian Zhang]

> On Feb. 5, 2017, 12:18 p.m., Jie Yu wrote:
> > src/slave/flags.cpp
> > Lines 173 (patched)
> > 
> >
> > Let's make the default /tmp/mesos/store/oci, to be consistent with 
> > others.

This was a suggestion of Avinash when he reviewed the design doc, the reason 
that we do not choose /tmp/mesos/store/oci is, /tmp is a tmpfs (or ramfs) an 
in-memory file system, the OCI images can be quite big (as with docker images) 
and might end up filling this file system very fast. And another reason is /tmp 
will be automatically cleared upon agent host reboot which is also what we do 
not want.


- Qian


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


On Feb. 1, 2017, 9:15 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52379/
> ---
> 
> (Updated Feb. 1, 2017, 9:15 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent flag '--oci_store_dir'.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 6a30471d325f35e171e81a687c23fd69e9ec97ee 
>   docs/endpoints/slave/state.json.md 0f82c1926404e79b281b2ea5f4d0ca21323aeded 
>   docs/endpoints/slave/state.md b34459e8624f0b29e927ff79be7fc845ac88080b 
>   src/slave/flags.hpp 2c4bd6ae628a272a4c6c2f02670baef011df4505 
>   src/slave/flags.cpp b48678bae8b3f5bab62710043eaca14ab8370183 
>   src/slave/http.cpp d68c9adc1db43c2e853c8b2290705fdc7739d45c 
> 
> 
> Diff: https://reviews.apache.org/r/52379/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>