Re: Review Request 60439: Added scripts to automate website publishing.

[Vinod Kone]

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

(Updated Aug. 4, 2017, 6:36 a.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and haosdent huang.


Changes
---

addressed comments.


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


Repository: mesos


Description
---

These scripts are expected to be run by ASF CI on any commit to
the mesos repo. The directory layout is similar to tidybot CI job.


Diffs (updated)
-

  support/jenkins/websitebot.sh PRE-CREATION 
  support/mesos-website.sh PRE-CREATION 
  support/mesos-website/Dockerfile PRE-CREATION 
  support/mesos-website/build.sh PRE-CREATION 
  support/mesos-website/entrypoint.sh PRE-CREATION 


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

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


Testing
---

Tested by running a CI job pointing to a branch containing this patch.


Thanks,

Vinod Kone

Re: Review Request 60439: Added scripts to automate website publishing.

[Vinod Kone]

> On Aug. 2, 2017, 8:59 a.m., Benjamin Bannier wrote:
> > support/mesos-website/entrypoint.sh
> > Lines 23 (patched)
> > 
> >
> > Do you still recall why this was needed? It would be great to add e.g., 
> > a JIRA or a small reproducer to eval in the future whether this is still 
> > needed.

Filed MESOS-7859


- Vinod


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


On July 27, 2017, 12:28 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60439/
> ---
> 
> (Updated July 27, 2017, 12:28 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and haosdent 
> huang.
> 
> 
> Bugs: MESOS-7625
> https://issues.apache.org/jira/browse/MESOS-7625
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These scripts are expected to be run by ASF CI on any commit to
> the mesos repo. The directory layout is similar to tidybot CI job.
> 
> 
> Diffs
> -
> 
>   support/jenkins/websitebot.sh PRE-CREATION 
>   support/mesos-website.sh PRE-CREATION 
>   support/mesos-website/Dockerfile PRE-CREATION 
>   support/mesos-website/build.sh PRE-CREATION 
>   support/mesos-website/entrypoint.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60439/diff/2/
> 
> 
> Testing
> ---
> 
> Tested by running a CI job pointing to a branch containing this patch.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 60439: Added scripts to automate website publishing.

[Vinod Kone]

> On Aug. 2, 2017, 8:52 a.m., Benjamin Bannier wrote:
> > support/mesos-website/build.sh
> > Lines 22 (patched)
> > 
> >
> > This script will misbehave if run from a live dev checkout (it might 
> > e.g., reconfigure an already configued setup, remove an already generated 
> > site, rebuild the whole project if the developer builds in `build/`, or 
> > pick up changes from a dirty tree so the SHA does not correspond to the 
> > contents).
> > 
> > I'd suggest to add a safeguard against that. One option would be to 
> > refuse to run if the tree contains any untracked files or uncommitted 
> > changes, e.g., with
> > 
> > if [ "$(git status --porcelain | wc -l)" -ne 0 ]; then
> >   echo "This script is not intended to run on a live checkout, but 
> > the source tree contains untracked files or uncommitted changes."
> >   
> >   exit 1
> > fi
> > 
> > We should add this before setting up the the exit trap.

I put this check in "support/jenkins/websitebot.sh" to fail early.


- Vinod


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


On July 27, 2017, 12:28 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60439/
> ---
> 
> (Updated July 27, 2017, 12:28 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and haosdent 
> huang.
> 
> 
> Bugs: MESOS-7625
> https://issues.apache.org/jira/browse/MESOS-7625
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These scripts are expected to be run by ASF CI on any commit to
> the mesos repo. The directory layout is similar to tidybot CI job.
> 
> 
> Diffs
> -
> 
>   support/jenkins/websitebot.sh PRE-CREATION 
>   support/mesos-website.sh PRE-CREATION 
>   support/mesos-website/Dockerfile PRE-CREATION 
>   support/mesos-website/build.sh PRE-CREATION 
>   support/mesos-website/entrypoint.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60439/diff/2/
> 
> 
> Testing
> ---
> 
> Tested by running a CI job pointing to a branch containing this patch.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 61406: Introduced `--share_pid_namespace` agent flag.

[Qian Zhang]

> On Aug. 4, 2017, 12:29 a.m., Jie Yu wrote:
> > why we need this flag? I think we need a way for operator to prevent top 
> > level containers on a host to share with agent pid namespace.
> 
> Gilbert Song wrote:
> @Qian, I guess you are trying to simplify the agent flag name, but I 
> think we may want to emphasize that:
> 1. this is for top level container only
> 2. it is operator specific
> 
> Do you have anyother concern for the flag name mentioned in the JIRA?

@Jie and @Gilbert, I know the original proposed flag name is 
`--disallow_top_level_pid_ns_sharing`, however, I went through all the agent 
flags, it seems there is no one named with `--disallow` or `--allow` as the 
prefix, that's why I named it `--share_pid_namespace`. However, I agree the 
original name is more explicit and clear, so I will change it back later.


- Qian


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


On Aug. 3, 2017, 11:20 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61406/
> ---
> 
> (Updated Aug. 3, 2017, 11:20 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-7853
> https://issues.apache.org/jira/browse/MESOS-7853
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced `--share_pid_namespace` agent flag.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 041c3dfb9c0c1718770f74dfb33a9f5d6fbe9b61 
>   src/slave/flags.hpp 032880dfa68cd29420e559d34e592e57827cfc07 
>   src/slave/flags.cpp 4171604090ffebe79fa35579458cabff7270c0de 
> 
> 
> Diff: https://reviews.apache.org/r/61406/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 61410: Remove support for omitting 202 responses to old libprocess clients.

[Benjamin Mahler]

> On Aug. 3, 2017, 7:13 p.m., Benjamin Hindman wrote:
> > Random question, do you know if there is a test that sends responses back 
> > to libprocess to exercise the `process::internal::ignore_recv_data` 
> > function?

Hm.. I couldn't find one.


> On Aug. 3, 2017, 7:13 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/tests/process_tests.cpp
> > Lines 1318 (patched)
> > 
> >
> > Were these actually necessary? I thought this was only necessary after 
> > we started to use `http::Server` since without `keepAlive` the 
> > `http::Server` will shutdown the socket right away (and 
> > `connection.disconnect()` would fail?).

They're necessary because before this change no response is received, so the 
connection remains open even with no keep alive. After this change, the 
response is received and http::Connection will disconnect after receiving the 
response. This test expects to be able to disconnect sucessfully, whereas 
disconnect() fails if already disconnected (although perhaps it should just 
return a ready future in that case!)


- Benjamin


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


On Aug. 3, 2017, 6:30 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61410/
> ---
> 
> (Updated Aug. 3, 2017, 6:30 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to commit d5fe51c on April 11 2014, we needed to omit the 202
> responses for libprocess messages because libprocess did not read
> the data from its message passing sockets.
> 
> This change removes support for omitting the responses to old clients.
> This also means that any 3rdparty libprocess clients (e.g. someone's
> go library) need to be correctly reading from their sockets to
> communicate with libprocess after this change.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> af5a75950bf24059d291acfd48399ab2d55eb8c2 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 30d0fb845468ff993e42a5568f57be131f0cd24b 
> 
> 
> Diff: https://reviews.apache.org/r/61410/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 61118: Building gRPC support in libprocess with CMake.

[Chun-Hung Hsiao]

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

(Updated Aug. 4, 2017, 1:20 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Jie Yu, and Joseph Wu.


Changes
---

Automatically generated `grpc_tests.pb.*` and `grpc_tests.grpc.pb.*` for grpc 
tests.


Summary (updated)
-

Building gRPC support in libprocess with CMake.


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


Repository: mesos


Description (updated)
---

This patch enables CMake to build code for gRPC support in libprocess
along with tests under Linux.

TODO(chhsiao): gRPC support for Windows.


Diffs (updated)
-

  3rdparty/libprocess/src/CMakeLists.txt 
f97291bd7cadcb440231619a8a2d1029a447ec27 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
27451c275b1f5a2eb7ada78f8cbe4b7c2c63ca92 


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

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


Testing (updated)
---

make
sudo make check
cd 3rdparty/libprocess/src/tests && make libprocess-tests && ./libprocess-tests


Thanks,

Chun-Hung Hsiao

Re: Review Request 61096: Building gRPC with CMake.

[Chun-Hung Hsiao]

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

(Updated Aug. 4, 2017, 1:17 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Jie Yu, and Joseph Wu.


Changes
---

Rebased on https://github.com/andschwa/mesos/commits/cmake-refactor-andy.


Summary (updated)
-

Building gRPC with CMake.


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


Repository: mesos


Description (updated)
---

This patch is based on @andschwa's branch:
https://github.com/andschwa/mesos/commits/cmake-refactor-andy
Related patches: r/61365.

This patch enables building the bundled gRPC library for libprocess
under Linux.

A patch for gRPC's CMake config is introduced for gRPC's bundled c-ares
library linkage issue: https://github.com/grpc/grpc/pull/11565.

TODO(chhsiao): Windows support.


Diffs (updated)
-

  3rdparty/CMakeLists.txt f4feaf90b4750338cfa2127524a5e6556a86d063 
  3rdparty/cmake/Versions.cmake 9586296420d76bd4493034fd710209008ee03595 
  3rdparty/grpc-1.4.2.patch PRE-CREATION 


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

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


Testing
---

make


Thanks,

Chun-Hung Hsiao

Re: Review Request 60957: Added a time window for Timer metrics.

[Benjamin Mahler]

> On July 19, 2017, 8:30 p.m., Benjamin Mahler wrote:
> > src/master/registrar.cpp
> > Line 156 (original), 156 (patched)
> > 
> >
> > There is only a single fetch, so this intentionally omits the time 
> > window since getting a distribution for a single data point isn't useful.
> 
> James Peach wrote:
> Hmm, maybe this is better as a `Gauge` then? If we add the time window, 
> then the values would eventually expire and the stats would go to zero while 
> the initial value is still non-zero, which would be weird.
> 
> James Peach wrote:
> I've left this change in (for now). Although the fetch only happens once, 
> it will generate statistics consistent with other `Timer` metrics. The sample 
> won't get expired because the history buffer won't fill and the calculated 
> statistics will show operators that this only happens once.

Hm.. using an explicit gauge for single measurement sounds reasonable to me. 
Can you make that change so that we don't dump the statistics for the state 
fetch?


- Benjamin


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


On July 19, 2017, 9:04 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60957/
> ---
> 
> (Updated July 19, 2017, 9:04 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6918
> https://issues.apache.org/jira/browse/MESOS-6918
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a time window argument for the Timer metrics that were missing it.
> 
> 
> Diffs
> -
> 
>   src/master/registrar.cpp 242cd0a4f1fad327dcc1a59260f46987cfeba59c 
>   src/state/log.cpp b71383906cf28fe0769cbb620387a0e0134f01f9 
> 
> 
> Diff: https://reviews.apache.org/r/60957/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 61387: Made the rlimits isolator work with nested containers.

[Gastón Kleiman]

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

(Updated Aug. 3, 2017, 10:10 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Ported the test to the v0 API.


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


Repository: mesos


Description (updated)
---

Make the rlimits isolator support nesting. Without this patch rlimits
sets for tasks launched by the DefaultExecutor are silently ignored.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/posix/rlimits.hpp 
ee36a24ae36179d3ff3141f8c3cdc768b1e399af 
  src/slave/containerizer/mesos/isolators/posix/rlimits.cpp 
3fc2b3dbd5b382e422c948adae0dc50f4fbcfc49 
  src/tests/containerizer/posix_rlimits_isolator_tests.cpp 
b7ccc7eeeb7e4d0d8f4fd463d506cfe7157076a4 


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

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


Testing
---

`bin/mesos-tests.sh --gtest_filter="PosixRLimitsIsolatorTest.NestedContainers" 
--gtest_repeat=1000 --gtest_break_on_failure` passed on a machine running 
GNU/Linux.


Thanks,

Gastón Kleiman

Re: Review Request 61125: Track the total number of time series samples.

[Benjamin Mahler]

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



I would imagine if people want to know this information they would use a 
counter, e.g.:

```
request_latency_ms/p50
request_latency_ms/p90
...
request_latency_ms/total = 1000

vs.

request_count = 1000
```

Here, getting the total number of requests by looking at the total number of 
latency samples seems rather indirect and a bit redundant with just keeping a 
separate counter. Do you have any examples that can motivate this?

- Benjamin Mahler


On July 25, 2017, 11:57 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61125/
> ---
> 
> (Updated July 25, 2017, 11:57 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6918
> https://issues.apache.org/jira/browse/MESOS-6918
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The time series only keeps a windows of data, so we can know
> the current number of samples, but we can't get a count of the
> total number of samples. Add a counter to the TimeSeries so
> that we can now the total number of samples seen in the series.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/statistics.hpp 
> e9f1fc23bf83f92a2e7de94dba0df48272cc3394 
>   3rdparty/libprocess/include/process/timeseries.hpp 
> 64b10a8d551ba33e252aa33987e3d5da8d56a1d6 
>   3rdparty/libprocess/src/tests/statistics_tests.cpp 
> 144b5109cfb7640b29bec8de8f5b2ad00665212f 
> 
> 
> Diff: https://reviews.apache.org/r/61125/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 61394: Made the capabilities isolator work with nested containers.

[Gastón Kleiman]

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

(Updated Aug. 3, 2017, 10:08 p.m.)


Review request for mesos, Benjamin Bannier, Jie Yu, and James Peach.


Changes
---

Ported the test to the v0 API.


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


Repository: mesos


Description
---

Make the capabilities isolator support nesting. Without this patch
capabilities sets for tasks launched by the DefaultExecutor are silently
ignored.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/linux/capabilities.hpp 
c3afe2054bb206e9b2823c008435f78e70ce7d63 
  src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 
f85a84d3871307bd1e9ebd45388ba680534acd89 
  src/tests/containerizer/linux_capabilities_isolator_tests.cpp 
80508760f8d635b414651e521848315399918fbc 


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

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


Testing
---

`sudo GLOG_v=1 bin/mesos-tests.sh 
--gtest_filter="*LinuxCapabilitiesIsolatorTest*"`


Thanks,

Gastón Kleiman

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

[Eric Chung]

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

(Updated Aug. 3, 2017, 9:41 p.m.)


Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.


Repository: mesos


Description (updated)
---

Added mesos.http and mesos.exceptions for CLI.

Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions
modules, which provides a Resource class and its descendants for
abstracting away common operations over http connectioins with JSON
serialization.


Diffs
-

  src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
  src/python/cli_new/pylint.config 63fbb94fbf47cccd1053b4c1eafac18b88dbc818 
  src/python/lib/mesos/exceptions.py PRE-CREATION 
  src/python/lib/mesos/http.py PRE-CREATION 
  src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
  src/python/lib/tests/conftest.py PRE-CREATION 
  src/python/lib/tests/test_exceptions.py PRE-CREATION 
  src/python/lib/tests/test_http.py PRE-CREATION 
  src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 


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


Testing
---

under src/python/lib, call `tox` for running unit tests. The test should pass 
and test coverage should be at 100%.


Thanks,

Eric Chung

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

[Eric Chung]

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

(Updated Aug. 3, 2017, 9:41 p.m.)


Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.


Repository: mesos


Description (updated)
---

Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions
modules, which provides a Resource class and its descendants for
abstracting away common operations over http connectioins with JSON
serialization.


Diffs (updated)
-

  src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
  src/python/cli_new/pylint.config 63fbb94fbf47cccd1053b4c1eafac18b88dbc818 
  src/python/lib/mesos/exceptions.py PRE-CREATION 
  src/python/lib/mesos/http.py PRE-CREATION 
  src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
  src/python/lib/tests/conftest.py PRE-CREATION 
  src/python/lib/tests/test_exceptions.py PRE-CREATION 
  src/python/lib/tests/test_http.py PRE-CREATION 
  src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 


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

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


Testing
---

under src/python/lib, call `tox` for running unit tests. The test should pass 
and test coverage should be at 100%.


Thanks,

Eric Chung

Re: Review Request 61270: Added container PID namespace control protobuf field in LinuxInfo.

[Gilbert Song]

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

(Updated Aug. 3, 2017, 2:40 p.m.)


Review request for mesos, Avinash sridharan, Gastón Kleiman, Jie Yu, Kevin 
Klues, Qian Zhang, and Vinod Kone.


Repository: mesos


Description
---

User facing interface for container configurable PID namespace.


Diffs (updated)
-

  include/mesos/mesos.proto f31f5bdc2ace2b261885b252e7c01ceb9b76c461 
  include/mesos/v1/mesos.proto 66386a84bc21989d1c1237e629d5d04662a368fa 


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

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


Testing
---

make


Thanks,

Gilbert Song

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

[Eric Chung]

> On July 29, 2017, 3:12 p.m., Armand Grillet wrote:
> > src/python/lib/mesos/http.py
> > Lines 119 (patched)
> > 
> >
> > Missing period, s/`verb`/`token` according to 
> > https://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html.

changed to "...with given method..."


> On July 29, 2017, 3:12 p.m., Armand Grillet wrote:
> > src/python/lib/mesos/http.py
> > Lines 162 (patched)
> > 
> >
> > Missing period.

dropped the JSONResource class as it is actually a bad design. Moved all json 
methods to the Resource class.


> On July 29, 2017, 3:12 p.m., Armand Grillet wrote:
> > src/python/lib/requirements.in
> > Lines 1 (patched)
> > 
> >
> > Not sure about using `<=` here and in the lines below, would use `<` so 
> > that we keep the same main dependency versions.

good catch. not sure why i added = (facepalm)


- Eric


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


On Aug. 3, 2017, 9:15 p.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61172/
> ---
> 
> (Updated Aug. 3, 2017, 9:15 p.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mesos.http and mesos.exceptions for CLI.
> 
> Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions 
> modules, which provides a Resource class and its descendants for abstracting 
> away common operations over http connectioins with JSON serialization.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
>   src/python/cli_new/pylint.config 63fbb94fbf47cccd1053b4c1eafac18b88dbc818 
>   src/python/lib/mesos/exceptions.py PRE-CREATION 
>   src/python/lib/mesos/http.py PRE-CREATION 
>   src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
>   src/python/lib/tests/conftest.py PRE-CREATION 
>   src/python/lib/tests/test_exceptions.py PRE-CREATION 
>   src/python/lib/tests/test_http.py PRE-CREATION 
>   src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
> 
> 
> Diff: https://reviews.apache.org/r/61172/diff/2/
> 
> 
> Testing
> ---
> 
> under src/python/lib, call `tox` for running unit tests. The test should pass 
> and test coverage should be at 100%.
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 61406: Introduced `--share_pid_namespace` agent flag.

[Gilbert Song]

> On Aug. 3, 2017, 9:29 a.m., Jie Yu wrote:
> > why we need this flag? I think we need a way for operator to prevent top 
> > level containers on a host to share with agent pid namespace.

@Qian, I guess you are trying to simplify the agent flag name, but I think we 
may want to emphasize that:
1. this is for top level container only
2. it is operator specific

Do you have anyother concern for the flag name mentioned in the JIRA?


- Gilbert


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


On Aug. 3, 2017, 8:20 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61406/
> ---
> 
> (Updated Aug. 3, 2017, 8:20 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-7853
> https://issues.apache.org/jira/browse/MESOS-7853
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced `--share_pid_namespace` agent flag.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 041c3dfb9c0c1718770f74dfb33a9f5d6fbe9b61 
>   src/slave/flags.hpp 032880dfa68cd29420e559d34e592e57827cfc07 
>   src/slave/flags.cpp 4171604090ffebe79fa35579458cabff7270c0de 
> 
> 
> Diff: https://reviews.apache.org/r/61406/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

[Eric Chung]

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

(Updated Aug. 3, 2017, 9:15 p.m.)


Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.


Repository: mesos


Description (updated)
---

Added mesos.http and mesos.exceptions for CLI.


Diffs (updated)
-

  src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
  src/python/cli_new/pylint.config 63fbb94fbf47cccd1053b4c1eafac18b88dbc818 
  src/python/lib/mesos/exceptions.py PRE-CREATION 
  src/python/lib/mesos/http.py PRE-CREATION 
  src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
  src/python/lib/tests/conftest.py PRE-CREATION 
  src/python/lib/tests/test_exceptions.py PRE-CREATION 
  src/python/lib/tests/test_http.py PRE-CREATION 
  src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 


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

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


Testing
---

under src/python/lib, call `tox` for running unit tests. The test should pass 
and test coverage should be at 100%.


Thanks,

Eric Chung

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

[Eric Chung]

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

(Updated Aug. 3, 2017, 9:15 p.m.)


Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.


Changes
---

fix summary


Repository: mesos


Description (updated)
---

Added mesos.http and mesos.exceptions for CLI.

Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions 
modules, which provides a Resource class and its descendants for abstracting 
away common operations over http connectioins with JSON serialization.


Diffs
-

  src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
  src/python/cli_new/pylint.config 63fbb94fbf47cccd1053b4c1eafac18b88dbc818 
  src/python/lib/mesos/exceptions.py PRE-CREATION 
  src/python/lib/mesos/http.py PRE-CREATION 
  src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
  src/python/lib/tests/conftest.py PRE-CREATION 
  src/python/lib/tests/test_exceptions.py PRE-CREATION 
  src/python/lib/tests/test_http.py PRE-CREATION 
  src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 


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


Testing
---

under src/python/lib, call `tox` for running unit tests. The test should pass 
and test coverage should be at 100%.


Thanks,

Eric Chung

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

[Eric Chung]

> On July 29, 2017, 3:12 p.m., Armand Grillet wrote:
> > src/python/lib/mesos/http.py
> > Lines 33 (patched)
> > 
> >
> > We generally prefer to do one import per line but it would be quite 
> > heavy in that situation.

this was actually formated with isort. we used it in several projects and it 
helps make imports very consistet. would you guys be open to using it?


- Eric


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


On July 27, 2017, 7:09 a.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61172/
> ---
> 
> (Updated July 27, 2017, 7:09 a.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mesos.http and mesos.exceptions for CLI.
> 
> Part of MESOS-7310, This patch adds the mesos.http and mesos.exceptions 
> modules, which provides a `Resource` class and its descendants for 
> abstracting away common operations over http connectioins with JSON 
> serialization.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
>   src/python/cli_new/pylint.config 63fbb94fbf47cccd1053b4c1eafac18b88dbc818 
>   src/python/lib/mesos/exceptions.py PRE-CREATION 
>   src/python/lib/mesos/http.py PRE-CREATION 
>   src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
>   src/python/lib/tests/conftest.py PRE-CREATION 
>   src/python/lib/tests/test_exceptions.py PRE-CREATION 
>   src/python/lib/tests/test_http.py PRE-CREATION 
>   src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
> 
> 
> Diff: https://reviews.apache.org/r/61172/diff/1/
> 
> 
> Testing
> ---
> 
> under src/python/lib, call `tox` for running unit tests. The test should pass 
> and test coverage should be at 100%.
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

[Eric Chung]

> On July 29, 2017, 3:12 p.m., Armand Grillet wrote:
> > In _testing done_, there seems to be a missing first part asking to run the 
> > updated bootstrap script and `source activate` under `src/python/cli_new` 
> > before running `tox`. At least these were necessary steps for me to have 
> > `tox` running correctly.

hmm that's weird. can you try again with a clean checkout? what were the errors 
that you saw?


- Eric


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


On July 27, 2017, 7:09 a.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61172/
> ---
> 
> (Updated July 27, 2017, 7:09 a.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mesos.http and mesos.exceptions for CLI.
> 
> Part of MESOS-7310, This patch adds the mesos.http and mesos.exceptions 
> modules, which provides a `Resource` class and its descendants for 
> abstracting away common operations over http connectioins with JSON 
> serialization.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
>   src/python/cli_new/pylint.config 63fbb94fbf47cccd1053b4c1eafac18b88dbc818 
>   src/python/lib/mesos/exceptions.py PRE-CREATION 
>   src/python/lib/mesos/http.py PRE-CREATION 
>   src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
>   src/python/lib/tests/conftest.py PRE-CREATION 
>   src/python/lib/tests/test_exceptions.py PRE-CREATION 
>   src/python/lib/tests/test_http.py PRE-CREATION 
>   src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
> 
> 
> Diff: https://reviews.apache.org/r/61172/diff/1/
> 
> 
> Testing
> ---
> 
> under src/python/lib, call `tox` for running unit tests. The test should pass 
> and test coverage should be at 100%.
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 61410: Remove support for omitting 202 responses to old libprocess clients.

[Benjamin Hindman]

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


Fix it, then Ship it!




Random question, do you know if there is a test that sends responses back to 
libprocess to exercise the `process::internal::ignore_recv_data` function?


3rdparty/libprocess/src/tests/process_tests.cpp
Lines 1318 (patched)


Were these actually necessary? I thought this was only necessary after we 
started to use `http::Server` since without `keepAlive` the `http::Server` will 
shutdown the socket right away (and `connection.disconnect()` would fail?).


- Benjamin Hindman


On Aug. 3, 2017, 6:30 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61410/
> ---
> 
> (Updated Aug. 3, 2017, 6:30 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to commit d5fe51c on April 11 2014, we needed to omit the 202
> responses for libprocess messages because libprocess did not read
> the data from its message passing sockets.
> 
> This change removes support for omitting the responses to old clients.
> This also means that any 3rdparty libprocess clients (e.g. someone's
> go library) need to be correctly reading from their sockets to
> communicate with libprocess after this change.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> af5a75950bf24059d291acfd48399ab2d55eb8c2 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 30d0fb845468ff993e42a5568f57be131f0cd24b 
> 
> 
> Diff: https://reviews.apache.org/r/61410/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 61410: Remove support for omitting 202 responses to old libprocess clients.

[Benjamin Mahler]

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

Review request for mesos and Benjamin Hindman.


Repository: mesos


Description
---

Prior to commit d5fe51c on April 11 2014, we needed to omit the 202
responses for libprocess messages because libprocess did not read
the data from its message passing sockets.

This change removes support for omitting the responses to old clients.
This also means that any 3rdparty libprocess clients (e.g. someone's
go library) need to be correctly reading from their sockets to
communicate with libprocess after this change.


Diffs
-

  3rdparty/libprocess/src/process.cpp af5a75950bf24059d291acfd48399ab2d55eb8c2 
  3rdparty/libprocess/src/tests/process_tests.cpp 
30d0fb845468ff993e42a5568f57be131f0cd24b 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Re: Review Request 61157: Refactored ProcessManager::handle for future use with http::Server.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On July 27, 2017, 1:55 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61157/
> ---
> 
> (Updated July 27, 2017, 1:55 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored ProcessManager::handle for future use with http::Server.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> af5a75950bf24059d291acfd48399ab2d55eb8c2 
> 
> 
> Diff: https://reviews.apache.org/r/61157/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 61222: Added V1 teardown Call.

[Anand Mazumdar]

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




src/master/http.cpp
Lines 3776-3779 (patched)


uh oh! There is a race condition here that can crash the master. It's 
possible that the master removed the framework by the time the continuation 
function is executed :-)

Can you rename the existing teardown that exists to `__teardown` and make 
this function invoke that? Also, goes for L3751


- Anand Mazumdar


On Aug. 3, 2017, 5:56 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61222/
> ---
> 
> (Updated Aug. 3, 2017, 5:56 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Greg Mann.
> 
> 
> Bugs: MESOS-6846
> https://issues.apache.org/jira/browse/MESOS-6846
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added V1 call support for teardown operation.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 0e4c30e3df704a929a3bd2e1787a76421d14a983 
>   include/mesos/v1/master/master.proto 
> c04fd1602396a58086331f5fa56518c5dee9af89 
>   src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70 
>   src/master/master.hpp 84465af782d4024f22463d981ef9d0ef7827d043 
>   src/master/validation.cpp 4885339b2112b6dbdc930875c20e1f5872b1edbf 
> 
> 
> Diff: https://reviews.apache.org/r/61222/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 61151: Used Future::recover in Mesos.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On July 27, 2017, 1:55 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61151/
> ---
> 
> (Updated July 27, 2017, 1:55 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used Future::recover in Mesos.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 64180944f8680828781168faa67417489abf1bc8 
> 
> 
> Diff: https://reviews.apache.org/r/61151/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 55318: Used loop in PollSocketImpl.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On Jan. 8, 2017, 7:44 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55318/
> ---
> 
> (Updated Jan. 8, 2017, 7:44 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used loop in PollSocketImpl.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/poll_socket.cpp 
> 5410b07e7b2ba5de4c1b61f358b6bcb224012eda 
> 
> 
> Diff: https://reviews.apache.org/r/55318/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Review Request 61408: Added test cases for V1 teardown Call.

[Quinn Leng]

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

Review request for mesos, Anand Mazumdar and Greg Mann.


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


Repository: mesos


Description
---

Added test cases for teardown operation in V1 operator API.


Diffs
-

  src/tests/api_tests.cpp 1d5b080c809248bdf4c76ddad382d714692c804b 


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


Testing
---

make check


Thanks,

Quinn Leng

Re: Review Request 61222: Added V1 teardown Call.

[Quinn Leng]

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

(Updated Aug. 3, 2017, 5:56 p.m.)


Review request for mesos, Anand Mazumdar and Greg Mann.


Summary (updated)
-

Added V1 teardown Call.


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


Repository: mesos


Description (updated)
---

Added V1 call support for teardown operation.


Diffs (updated)
-

  include/mesos/master/master.proto 0e4c30e3df704a929a3bd2e1787a76421d14a983 
  include/mesos/v1/master/master.proto c04fd1602396a58086331f5fa56518c5dee9af89 
  src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70 
  src/master/master.hpp 84465af782d4024f22463d981ef9d0ef7827d043 
  src/master/validation.cpp 4885339b2112b6dbdc930875c20e1f5872b1edbf 


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

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


Testing
---

make check


Thanks,

Quinn Leng

Re: Review Request 61406: Introduced `--share_pid_namespace` agent flag.

[Jie Yu]

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



why we need this flag? I think we need a way for operator to prevent top level 
containers on a host to share with agent pid namespace.

- Jie Yu


On Aug. 3, 2017, 3:20 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61406/
> ---
> 
> (Updated Aug. 3, 2017, 3:20 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-7853
> https://issues.apache.org/jira/browse/MESOS-7853
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced `--share_pid_namespace` agent flag.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 041c3dfb9c0c1718770f74dfb33a9f5d6fbe9b61 
>   src/slave/flags.hpp 032880dfa68cd29420e559d34e592e57827cfc07 
>   src/slave/flags.cpp 4171604090ffebe79fa35579458cabff7270c0de 
> 
> 
> Diff: https://reviews.apache.org/r/61406/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 61406: Introduced `--share_pid_namespace` agent flag.

[Qian Zhang]

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

Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, 
and Vinod Kone.


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


Repository: mesos


Description
---

Introduced `--share_pid_namespace` agent flag.


Diffs
-

  docs/configuration.md 041c3dfb9c0c1718770f74dfb33a9f5d6fbe9b61 
  src/slave/flags.hpp 032880dfa68cd29420e559d34e592e57827cfc07 
  src/slave/flags.cpp 4171604090ffebe79fa35579458cabff7270c0de 


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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 61270: Added container PID namespace control protobuf field in LinuxInfo.

[Qian Zhang]

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


Fix it, then Ship it!





include/mesos/mesos.proto
Lines 2741 (patched)


s/namepace/namespace/



include/mesos/mesos.proto
Lines 2744 (patched)


s/namespce/namespace/



include/mesos/v1/mesos.proto
Lines 2724 (patched)


Ditto.



include/mesos/v1/mesos.proto
Lines 2727 (patched)


Ditto.


- Qian Zhang


On Aug. 3, 2017, 1:51 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61270/
> ---
> 
> (Updated Aug. 3, 2017, 1:51 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gastón Kleiman, Jie Yu, Kevin 
> Klues, Qian Zhang, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> User facing interface for container configurable PID namespace.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto f31f5bdc2ace2b261885b252e7c01ceb9b76c461 
>   include/mesos/v1/mesos.proto 66386a84bc21989d1c1237e629d5d04662a368fa 
> 
> 
> Diff: https://reviews.apache.org/r/61270/diff/2/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 61272: Added a MockResourceProvider.

[Jan Schlicht]

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

(Updated Aug. 3, 2017, 4:28 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

Added a MockResourceProvider.


Diffs (updated)
-

  src/tests/mesos.hpp 6f06261d81870b923b7053daf8205327c4ac6a45 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 61272: Added a MockResourceProvider.

[Jan Schlicht]

> On Aug. 3, 2017, noon, Benjamin Bannier wrote:
> > src/tests/mesos.hpp
> > Lines 2301 (patched)
> > 
> >
> > How about passing this as an argument instead? It semantically belongs 
> > to the remote description we already pass (`url`, `contentType`).

Makes sense. By making the crendential an additional template parameter (and 
setting it in the v1 namespace), this would also allow us to use a 
`v2::DEFAULT_CREDENTIAL` or something else in the future.


- Jan


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


On Aug. 2, 2017, 1:38 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61272/
> ---
> 
> (Updated Aug. 2, 2017, 1:38 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-7469
> https://issues.apache.org/jira/browse/MESOS-7469
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a MockResourceProvider.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 6f06261d81870b923b7053daf8205327c4ac6a45 
> 
> 
> Diff: https://reviews.apache.org/r/61272/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 61271: Implemented HTTP connection handling for the resource provider driver.

[Jan Schlicht]

> On Aug. 3, 2017, noon, Benjamin Bannier wrote:
> > include/mesos/v1/resource_provider.hpp
> > Lines 81 (patched)
> > 
> >
> > Passing an `Option` here makes sense to me, but this is not done in 
> > e.g., the scheduler, 
> > https://github.com/apache/mesos/blob/382f526ee2c13df063e17d8346915f3716fe6d21/include/mesos/scheduler.hpp#L364-L366
> >  and I am not sure I follow their reasoning (we also already assume that 
> > e.g., libprocess can be used in this interface).

`mesos/scheduler.hpp` isn't the right place to look, it's actually 
`mesos/v1/scheduler.hpp`. There, an `Option` is used as a parameter in the 
constructor. Maybe stout isn't visible in `mesos/scheduler.hpp` because it's a 
public interface that is used by other language bindings.


> On Aug. 3, 2017, noon, Benjamin Bannier wrote:
> > src/Makefile.am
> > Lines 1095 (patched)
> > 
> >
> > We need this in the cmake setup as well.

I couldn't find a similar section in any of the `CMakeLists.txt`, that's why I 
didn't add anything there.


> On Aug. 3, 2017, noon, Benjamin Bannier wrote:
> > src/resource_provider/driver.cpp
> > Lines 43 (patched)
> > 
> >
> > As we only have a single user below, I feel that it would be fine to 
> > use a lambda at the use site instead.

The use site is a constructor's initializer list. While I could use a lambda 
there, IMHO it doesn't look very nice and quite distracting.


- Jan


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


On Aug. 2, 2017, 2:38 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61271/
> ---
> 
> (Updated Aug. 2, 2017, 2:38 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-7469
> https://issues.apache.org/jira/browse/MESOS-7469
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Similar to the existing HTTP connection handling of schedulers and
> executors, the resource provider driver will create two connections
> with the resource provider manager, one for streaming events and another
> one for sending calls. This connection handling has been generalized as
> a 'HttpConnectionProcess' and can be reused in other cases.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/resource_provider.hpp 
> 88b606212ea57fee1c1ea522d2dc7f8124a9adef 
>   src/Makefile.am 5712bad2acc4cf0f8ec9b7febffcdb0fa77578c9 
>   src/resource_provider/driver.cpp 6778ec9c863022446f141ee88f70eb563178ea05 
>   src/resource_provider/http_connection.hpp PRE-CREATION 
>   src/resource_provider/storage/provider.cpp 
> 4c39312be5e4a6d783df3d385a66be6b3dcf8603 
> 
> 
> Diff: https://reviews.apache.org/r/61271/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 60508: Fixed Attributes comparison.

[Ilya Pronin]

> On Aug. 2, 2017, 6:56 p.m., Anand Mazumdar wrote:
> > include/mesos/v1/attributes.hpp
> > Lines 95 (patched)
> > 
> >
> > Can we add a comment for posterity on the usage of `contains` and how 
> > does it differ from `get()`?

Done. Added comments for `contains()` and `get()` methods describing what they 
do.


- Ilya


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


On Aug. 3, 2017, 1:11 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60508/
> ---
> 
> (Updated Aug. 3, 2017, 1:11 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-1216
> https://issues.apache.org/jira/browse/MESOS-1216
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently we don't enforce attribute name uniqueness. But the comparison
> operator for `Attributes` expects that attribute name and type are
> unique. This causes causes agent recovery failure.
> 
> This fixes the issue by allowing multiple attributes of same name and
> type.
> 
> 
> Diffs
> -
> 
>   include/mesos/attributes.hpp 641febbd3bc3700b2c088f4388d8e6f18e6b078a 
>   include/mesos/v1/attributes.hpp 022bfde430c618708843a4d790f3f8a118ae082b 
>   src/common/attributes.cpp ac683c7fe5b67fb8a73599230a3eb47911293a7e 
>   src/tests/attributes_tests.cpp cb71be5ecead322d90943146f54f8a0c915eba1c 
>   src/v1/attributes.cpp 409d66453c492a329adc4e16312924e21a6d4709 
> 
> 
> Diff: https://reviews.apache.org/r/60508/diff/2/
> 
> 
> Testing
> ---
> 
> Added a test to `AttributesTest.Equality`. Ran `make check`. Verified 
> manually by failing over the agent with multiple attributes with the same 
> name.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 60508: Fixed Attributes comparison.

[Ilya Pronin]

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

(Updated Aug. 3, 2017, 1:11 p.m.)


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


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Currently we don't enforce attribute name uniqueness. But the comparison
operator for `Attributes` expects that attribute name and type are
unique. This causes causes agent recovery failure.

This fixes the issue by allowing multiple attributes of same name and
type.


Diffs (updated)
-

  include/mesos/attributes.hpp 641febbd3bc3700b2c088f4388d8e6f18e6b078a 
  include/mesos/v1/attributes.hpp 022bfde430c618708843a4d790f3f8a118ae082b 
  src/common/attributes.cpp ac683c7fe5b67fb8a73599230a3eb47911293a7e 
  src/tests/attributes_tests.cpp cb71be5ecead322d90943146f54f8a0c915eba1c 
  src/v1/attributes.cpp 409d66453c492a329adc4e16312924e21a6d4709 


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

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


Testing
---

Added a test to `AttributesTest.Equality`. Ran `make check`. Verified manually 
by failing over the agent with multiple attributes with the same name.


Thanks,

Ilya Pronin

Re: Review Request 61272: Added a MockResourceProvider.

[Benjamin Bannier]

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




src/tests/mesos.hpp
Lines 2258-2259 (patched)


This is not needed as these functions will be declared automatically.



src/tests/mesos.hpp
Lines 2291 (patched)


We want to construct our actual base class object of type `Driver` here, 
e.g.,

: Driver( ...



src/tests/mesos.hpp
Lines 2301 (patched)


How about passing this as an argument instead? It semantically belongs to 
the remote description we already pass (`url`, `contentType`).



src/tests/mesos.hpp
Lines 2308 (patched)


It seems weird to `move` here. `std::queue::front` returns a reference, and 
we need to make a copy anyway.


- Benjamin Bannier


On Aug. 2, 2017, 1:38 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61272/
> ---
> 
> (Updated Aug. 2, 2017, 1:38 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-7469
> https://issues.apache.org/jira/browse/MESOS-7469
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a MockResourceProvider.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 6f06261d81870b923b7053daf8205327c4ac6a45 
> 
> 
> Diff: https://reviews.apache.org/r/61272/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 61271: Implemented HTTP connection handling for the resource provider driver.

[Benjamin Bannier]

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




include/mesos/v1/resource_provider.hpp
Lines 37-39 (patched)


Let's move this into the namespace `mesos` declared below.

nit: `} // namespace internal {



include/mesos/v1/resource_provider.hpp
Lines 81 (patched)


Passing an `Option` here makes sense to me, but this is not done in e.g., 
the scheduler, 
https://github.com/apache/mesos/blob/382f526ee2c13df063e17d8346915f3716fe6d21/include/mesos/scheduler.hpp#L364-L366
 and I am not sure I follow their reasoning (we also already assume that e.g., 
libprocess can be used in this interface).



src/Makefile.am
Lines 1095 (patched)


We need this in the cmake setup as well.



src/resource_provider/driver.cpp
Lines 43 (patched)


As we only have a single user below, I feel that it would be fine to use a 
lambda at the use site instead.



src/resource_provider/driver.cpp
Lines 52-63 (original), 50-59 (patched)


Let's not introduce this as part of this patch, but instead as a separate 
package implementing https://issues.apache.org/jira/browse/MESOS-7854 (we 
should also add tests for this functionality then). Currently this assume a 
certain authentication scheme which might not what we want.

We can pass a none credential when instantiating the driver below.



src/resource_provider/driver.cpp
Lines 82 (patched)


Just pass `None()`, see above.



src/resource_provider/http_connection.hpp
Lines 59 (patched)


Let's document somewhere our expectations on `Call`. We e.g., assume that 
it has an enum-valued member function `type()`. The enum value 
`Call::SUBSCRIBE` has a special meaning. Pretty minor, we also expect the enum 
value to be stringifyable.



src/resource_provider/http_connection.hpp
Lines 82 (patched)


Let's add this as part of MESOS-7854 (we do not have any tests for it ATM 
anyway).



src/resource_provider/http_connection.hpp
Lines 90 (patched)


I wonder if it makes more sense to return e.g., a `Try` here in order 
to surface the failure scenarios below, many of which might be caused by the 
caller. This could give them a chance to retry if we map `true` to success and 
`false` to transient errors.

Otherwise let's add a `TODO`.



src/resource_provider/http_connection.hpp
Lines 99-113 (patched)


This block ensures both that callers use this method correctly, and that 
our internal invariants are good.

Explicitly calling out the internal invariants could make this more 
readable, e.g., add below

CHECK(state == State::CONNECTED || state == State::SUBSCRIBED) << state;
CHECK_SOME(connections);



src/resource_provider/http_connection.hpp
Lines 129 (patched)


Suggest to move this just below the block ensuring this does not fire.



src/resource_provider/http_connection.hpp
Lines 133 (patched)


Let's call out what state transition we are performing, e.g.,

CHECK_EQ(State::CONNECTED, state);



src/resource_provider/http_connection.hpp
Lines 237-241 (patched)


We check an aweful lot of states here making it hard to see what cases 
could be missed. How about

void disconnected(const std::string& failure)
{
  switch (state) {
case State::DISCONNECTED: {
  UNREACHABLE();
}
 
case State::CONNECTED:
case State::CONNECTING:
case State::SUBSCRIBED: {
  mutex.lock()
.then(defer(
self(),
[this]() { return process::async(callbacks.disconnected); 
}))
.onAny(lambda::bind(&process::Mutex::unlock, mutex));
  break;
}
 
case State::SUBSCRIBING: {
  break;
}
  }

  disconnect();
}



src/resource_provider/http_connection.hpp
Lines 400 (patched)


Let's delete the copy constructor here, e.g.,

// The decoder cannot be copied meaningfully, see MESOS-5122.
SubscribedResponse(const SubscribedResponse&) = delete;
  

Re: Review Request 61275: Added a URL parameter to the resource provider driver.

[Benjamin Bannier]

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




src/resource_provider/daemon.hpp
Lines 44 (patched)


I wonder if it would make more sense to keep passing a pid here like you 
did previously. With that we'd form the `URL` inside the daemon when creating 
the drivers.

Since the use of http is a detail of how drivers currently work, passing a 
pid would prevent us from leaking that fact beyond the daemon. Also, a driver 
will take a `ContentType` which is specific to HTTP which makes sense together 
with an URL; an URL in isolation seems leaky for that reason as well to me. I 
believe a pid and some `slave::Flags` passed here would form a similarly 
intuitive package on a different level of abstraction.


- Benjamin Bannier


On Aug. 2, 2017, 1:37 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61275/
> ---
> 
> (Updated Aug. 2, 2017, 1:37 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-7469
> https://issues.apache.org/jira/browse/MESOS-7469
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The URL will be used by the resource provider driver to determine
> the endpoint of the resource provider manager API.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/resource_provider.hpp 
> 88b606212ea57fee1c1ea522d2dc7f8124a9adef 
>   src/resource_provider/daemon.hpp 467a5d30510f0c8f7f042d989c615c97bbae52af 
>   src/resource_provider/daemon.cpp adcb60af8244840c56fbd2d846bdaeecb4f62282 
>   src/resource_provider/driver.cpp 6778ec9c863022446f141ee88f70eb563178ea05 
>   src/resource_provider/local.hpp 604c5d06b393349c352d0b698609ad6bfb16454a 
>   src/resource_provider/local.cpp a57c7c99c674f18036c36ec4b6df71947f700db8 
>   src/resource_provider/storage/provider.hpp 
> c6ea440feeb4cd79c59aa1f25851513e5d8dcc86 
>   src/resource_provider/storage/provider.cpp 
> 4c39312be5e4a6d783df3d385a66be6b3dcf8603 
>   src/slave/slave.cpp 7381530515f86faf4c3e8f82bcd9483f6cf0498b 
> 
> 
> Diff: https://reviews.apache.org/r/61275/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 61272: Added a MockResourceProvider.

[Jan Schlicht]

> On Aug. 2, 2017, 12:37 a.m., Jie Yu wrote:
> > src/tests/mesos.hpp
> > Lines 2287-2291 (patched)
> > 
> >
> > This is a bit counter intuitive. I was expecting that 
> > MockResourceProvider will take a real Driver:
> > ```
> > class MockResourceProvider
> > {
> > public:
> >   MockResourceProvider(
> >   const URL& endpoint,
> >   ContentType contentType)
> > : driver(
> >   Owned(new 
> > ConstantEndpointDetector(endpoint)),
> >   contentType,
> >   [this]() { connected(); },
> >   [this]() { disconnected(); },
> >   [this](std::queue events) {
> > while (!events.empty()) {
> >   Event event = std::move(events.front());
> >   events.pop();
> >   received(event);
> > }
> >   }) {}
> >   
> >   MOCK_METHOD0_T(connected, void());
> >   MOCK_METHOD0_T(disconnected, void());
> >   ...
> > 
> > private:
> >   Driver driver;
> > };
> > ```
> 
> Jan Schlicht wrote:
> The driver needs to be accessible in the test case to allow things like
> ```
>   v1::MockResourceProvider resourceProvider;
>   v1::resource_provider::TestDriver driver;
>   ...
> 
>   Future subscribed;
>   EXPECT_CALL(*resourceProvider, subscribed(_))
> .WillOnce(FutureArg<0>(&subscribed));
> 
>   mesos::v1::ResourceProviderInfo resourceProviderInfo;
>   resourceProviderInfo.set_type("org.apache.mesos.rp.test");
>   resourceProviderInfo.set_name("test");
> 
>   // Creates a 'SUBSCRIBE' message and sends it using the driver.
>   subscribe(&driver, resourceProviderInfo);
> 
>   AWAIT_READY(subscribed);
> ```
> 
> Jie Yu wrote:
> hum, why can't we use a real RP manager to send the message?

Sorry, I don't understand. The driver sends `Call`s to the RP manager and 
receives `Event`s that it relays to `MockResourceProvider`. We need to separate 
driver and `MockResourceProvider` to be able to set expectations on our mock, 
before the driver starts. E.g. we'd expect `MockResourceProvider::connected` to 
be called when a connection to the RP manager has been established. We need to 
set this expectation (by creating a `Future` and `EXPECT_CALL` on the 
`MockResourceProvider` instance) before a driver instanciated, hence have to 
keep them separated.


- Jan


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


On Aug. 2, 2017, 1:38 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61272/
> ---
> 
> (Updated Aug. 2, 2017, 1:38 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-7469
> https://issues.apache.org/jira/browse/MESOS-7469
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a MockResourceProvider.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 6f06261d81870b923b7053daf8205327c4ac6a45 
> 
> 
> Diff: https://reviews.apache.org/r/61272/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>