Re: Review Request 44854: Added validation for executor's shutdown grace period.

[Ben Mahler]

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


Ship it!




The change looks good but please add a test for this.

Ideally there could be a stateless validateExecutorInfo, in addition to the one 
that needs the master state. In the absence of this, you can write a test that 
spins up a framework and launches a task with a negative shutdown duration.

- Ben Mahler


On March 17, 2016, 11:34 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44854/
> ---
> 
> (Updated March 17, 2016, 11:34 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4949
> https://issues.apache.org/jira/browse/MESOS-4949
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
> 
> Diff: https://reviews.apache.org/r/44854/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44553: Added authentication to agent HTTP endpoints.

[Adam B]

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


Fix it, then Ship it!




Minor suggestions


src/tests/slave_tests.cpp (lines 1480 - 1482)


In fact, you don't even need to call `CreateSlaveFlags` explicitly, since 
`StartSlave(detector.get())` will call it under the hood for you.



src/tests/slave_tests.cpp (line 1487)


`s/__recover/recover/` (for the variable name)


- Adam B


On March 17, 2016, 3:56 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44553/
> ---
> 
> (Updated March 17, 2016, 3:56 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4850
> https://issues.apache.org/jira/browse/MESOS-4850
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authentication to agent HTTP endpoints.
> 
> This patch adds HTTP authentication to the `/state`, `/state.json`, and 
> `/flags` endpoints. Tests are also updated to use authentication when hitting 
> these endpoints, and a new test was added to probe these endpoints' behavior 
> when bad credentials are supplied: `SlaveTest.HTTPEndpointsBadAuthentication`.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 
>   src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
>   src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> f6fce7df82417e029fadf805d6e0b793f396aa69 
>   src/tests/fault_tolerance_tests.cpp 
> f99413f56e96a796d3d45decad1f049e6a238789 
>   src/tests/health_check_tests.cpp d32164aeb1eb439bd062afa28614dd919e24f06b 
>   src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169 
> 
> Diff: https://reviews.apache.org/r/44553/diff/
> 
> 
> Testing
> ---
> 
> Tests were updated to use authentication when hitting the affected agent 
> endpoints, and new tests were added to probe the endpoint behavior when 
> invalid credentials are supplied.
> 
> `sudo make check` was used to test on both OSX and Ubuntu 14.04. The new test 
> was run 1000 times to look for flakiness.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 44907: Remove `Fetcher` in `DockerContainerizer::create`.

[haosdent huang]

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

(Updated March 16, 2016, 5:57 p.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Rebase.


Repository: mesos


Description
---

Remove `Fetcher` in `DockerContainerizer::create`.


Diffs (updated)
-

  src/slave/containerizer/containerizer.cpp 
f6fc7863d0c215611f170dc0c89aa229407b5137 
  src/slave/containerizer/docker.hpp 79cd955e9c241becff52cc4bbef81dcc16802ee7 
  src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 

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


Testing
---


Thanks,

haosdent huang

Review Request 44954: Regenerated master endpoint documentation.

[Joerg Schad]

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

Review request for mesos, Adam B and Greg Mann.


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


Repository: mesos


Description
---

Reran the generate-endpoint-help.py scipt after adding the AUTHENTICATION 
information to HELP.


Diffs
-

  docs/endpoints/master/api/v1/scheduler.md 
7a75fd75eef458f26f9e5067b792329f310d5d24 
  docs/endpoints/master/create-volumes.md 
2ab116a16581fc5cfc0fc5cf70c6e001a28d8493 
  docs/endpoints/master/destroy-volumes.md 
88679fe12baf7642b63e71688b90031aaedab1ef 
  docs/endpoints/master/flags.md 739d6bfae5ae35223d6bc7c4782ff0f16b868018 
  docs/endpoints/master/frameworks.md 95c1f0e63f6a61d0e6622a8ec9d821a646656cfc 
  docs/endpoints/master/health.md 7f9352025a6364b812b91e9ccbe380a3a69ba4e1 
  docs/endpoints/master/machine/down.md 
6eec9ecc6e92fd1299b27499f78055c6f4f68e81 
  docs/endpoints/master/machine/up.md 7cbbf04859a218f90672453e5037480676f79fe5 
  docs/endpoints/master/maintenance/schedule.md 
5f5267d24e392d6302d126e2367be08e6cc5e174 
  docs/endpoints/master/maintenance/status.md 
4d0c7551acb89fb375834fd703c406d68f8bdcfc 
  docs/endpoints/master/observe.md fae1ee062350d7b25775bef98a0bebda1892ab62 
  docs/endpoints/master/quota.md 812874d2dd6c3548887e3044ba1f3c3c8c9d1dd6 
  docs/endpoints/master/redirect.md ac9d0fa3eae485726b10a3ac756228d0cb5aeb27 
  docs/endpoints/master/reserve.md 1a4f67961baf761f79a693780f42a1a8ce2244fc 
  docs/endpoints/master/roles.json.md 863715386791ef9192f38bf390fcd2b31d988547 
  docs/endpoints/master/roles.md 171e0163c4c2db34bf34c8303846793f3c29bdf5 
  docs/endpoints/master/slaves.md ec169c15507d3c731b8833f2656949caf0efcc33 
  docs/endpoints/master/state-summary.md 
fb10ac7db5b94ea7982c245f1d884312d818c6b5 
  docs/endpoints/master/state.json.md 0415cfdd63cee0b350b3e33c496bacfc1ba53f8a 
  docs/endpoints/master/state.md ae1ec718b8d718718727dbd2e7ce4739cca41680 
  docs/endpoints/master/tasks.json.md 46a1253d33cdfdca0a7158808d1e29da1a634933 
  docs/endpoints/master/tasks.md 2ada97b5dfdaf5f1f7578fa42eb4e61233e6a753 
  docs/endpoints/master/teardown.md f68d083a4271ed7dd0ddcc87da104a43eb40c7ec 
  docs/endpoints/master/unreserve.md e059d8d888343c5036346f7a615f04375f44f517 
  docs/endpoints/master/weights.md 632be242d85fbd61ded62d846e385009a321533c 

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


Testing
---

viewed docs with docker-website generator.


Thanks,

Joerg Schad

Review Request 44932: Removed unused overload of `model` for `TaskInfo`.

[Neil Conway]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

Removed unused overload of `model` for `TaskInfo`.


Diffs
-

  src/common/http.hpp 61c63a09c428331f7561a9c4c0254bf4e1c8915f 
  src/common/http.cpp ff3b7fc2fb8d2895354c12c1773be6f655352223 
  src/tests/common/http_tests.cpp d1bafeb2c5298f9eda8f3927d4ff2ad62b1bb0be 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 44421: Added support for "overlay" keyword.

[Jian Qiu]

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




src/linux/fs.cpp (line 74)


This line seems redundant.


- Jian Qiu


On 三月 11, 2016, 11:58 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44421/
> ---
> 
> (Updated 三月 11, 2016, 11:58 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Jie Yu, and Shuai Lin.
> 
> 
> Bugs: MESOS-4874
> https://issues.apache.org/jira/browse/MESOS-4874
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The "overlayfs" was renamed to "overlay" in kernel 4.2, for overlay
> support check function, it should check both "overlay" and "overlayfs"
> in "/proc/filesystems".
> 
> 
> Diffs
> -
> 
>   src/linux/fs.hpp 4525a5d5566e2bc913894e993ac7350f1bbd9cc0 
>   src/linux/fs.cpp dbf94759b6382bfafd3f3b8b4c2047af36a53ad5 
>   src/slave/containerizer/mesos/provisioner/backend.cpp 
> 55652540e35f9c451ad85cfead575a788aa3eba1 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
> 5cc0f8b5a8cd4c945023f874056a8184113186c5 
>   src/tests/environment.cpp ee1bbe6b4e3dda1e27b63d71a08ef0d1d254741a 
> 
> Diff: https://reviews.apache.org/r/44421/diff/
> 
> 
> Testing
> ---
> 
> root@mesos002:~/src/mesos/m1/mesos/build# uname -r
> 4.2.3-040203-generic
> root@mesos002:~/src/mesos/m1/mesos/build# lsmod  | grep over
> overlay45056  1 
> root@mesos002:~/src/mesos/m1/mesos/build# cat /proc/filesystems | grep over
> nodev overlay
> root@mesos002:~/src/
> 
> make
> make check
> ./bin/mesos-tests.sh 
> --gtest_filter="OverlayBackendTest.ROOT_OVERLAYFS_OverlayFSBackend" --verbose
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 44081: Stout: Moved `os::libraries::` namespace back to `stout/os.hpp`.

[Michael Park]

> On March 18, 2016, 7:27 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, line 131
> > 
> >
> > `setenv` in `stout/windows/os.hpp` is commented out and unimplemeneted. 
> > How does this work?

This seems to be tackled in https://reviews.apache.org/r/44082.


- Michael


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


On March 14, 2016, 9:10 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44081/
> ---
> 
> (Updated March 14, 2016, 9:10 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Moved `os::libraries::` namespace back to `stout/os.hpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> 9ee233b988c08d953e70345c55bcdd5c2f7c101b 
> 
> Diff: https://reviews.apache.org/r/44081/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 42684: Multiple Disk: Added persistent volumes tests for `MOUNT` type.

[Neil Conway]

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


Fix it, then Ship it!





src/tests/persistent_volume_tests.cpp (line 152)


Personally I would find this clearer if the order of operands was reversed, 
e.g., `CHECK_GE(id, 1)`



src/tests/persistent_volume_tests.cpp (line 204)


`constexpr`? And name this `NUM_DISKS`?



src/tests/persistent_volume_tests.cpp (line 229)


Typo ("privileges")



src/tests/persistent_volume_tests.cpp (line 290)


Can you add a comment (e.g., to commit message) why you're changing all 
these disk resources sizes to be larger?


- Neil Conway


On Jan. 26, 2016, 8:20 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42684/
> ---
> 
> (Updated Jan. 26, 2016, 8:20 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Bugs: MESOS-191
> https://issues.apache.org/jira/browse/MESOS-191
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Multiple Disk: Added persistent volumes tests for `MOUNT` type.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> cbf2bcedd5b4c14107d3b50a74f161aa9395d7d0 
>   src/tests/resources_tests.cpp 54a4fa88bfdcff3c0a7e89cbf3a1674c954b7f23 
> 
> Diff: https://reviews.apache.org/r/42684/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 44678: Modified basic HTTP authenticator creator to accept realm.

[Greg Mann]

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

(Updated March 18, 2016, 12:20 a.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Modified basic HTTP authenticator creator to accept realm.

To accommodate different authentication realms for the master and agent, the 
default basic HTTP authenticator needs to accept its authentication realm as a 
parameter. This patch adds this parameter and modifies the HTTP authentication 
tests to validate it appropriately. A new test was also added: 
`HttpAuthenticationTest.BasicWithoutRealm`.


Diffs (updated)
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 
  src/authentication/http/basic_authenticator_factory.cpp 
62f851685db3b42c52bbcb7cff3e4f4703004ed7 
  src/examples/test_http_authenticator_module.cpp 
459b7046bd76d3043d2484a2dd30c10d7deaedd4 
  src/master/master.cpp e6290ea686ccf17813d6faeaf2f2012f79cf3b7f 
  src/tests/http_authentication_tests.cpp 
cf2bb762272fa38e04e5c26aef2858300bbd0459 

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


Testing
---

HTTP authentication tests were updated to pass the authentication realm to the 
basic HTTP authenticator, and to adhere to the new credentials format in the 
module parameters. A new test was also added: 
`HttpAuthenticationTest.BasicWithoutRealm`

`make check` was used to test on both OSX and CentOS 7.


Thanks,

Greg Mann

Re: Review Request 44288: Changed MasterDetector/Contender namespace.

[Anurag Singh]

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

(Updated March 18, 2016, 12:29 a.m.)


Review request for mesos and Benjamin Hindman.


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


Repository: mesos


Description
---

Also modified users of MasterContender and MasterDetector to use this
namespace.


Diffs (updated)
-

  include/mesos/scheduler.hpp 14c7ff964aa7b94f439d16e605380661d2279d54 
  include/mesos/v1/scheduler.hpp 765935e97b6c1686ab464a5cf1cf2dfd816f51f1 
  src/cli/resolve.cpp 257e29034abf32491511f9a4e476b6859714829d 
  src/local/local.cpp f8599e7378e9a0065bbd01ad8f23f11debb30c91 
  src/master/contender.cpp 9ad49ce10439fb41d78d52eaa4c1e6b9c5c7f735 
  src/master/detector.cpp 9274435802d6292b183be48f42b43999476e016e 
  src/master/main.cpp 61210d9f275d4073967c3468179307cf09e88551 
  src/master/master.hpp 124d43931a5c8a00ee0aaa604feb1761795209f2 
  src/master/master.cpp e6290ea686ccf17813d6faeaf2f2012f79cf3b7f 
  src/sched/sched.cpp 525255eec808c3fe5c0e38b3d1a2086bbd4eb171 
  src/scheduler/scheduler.cpp 6a834473ef35540eedac7e211b5204ab5f4eb7b2 
  src/slave/main.cpp 33a1af84aeb079224b15e92caf97bcf081ea4646 
  src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
  src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
  src/tests/authentication_tests.cpp 8143cd7a22bbdbcd0fc613cb44eae8b55fd458e7 
  src/tests/cluster.hpp 06424dd741aed2261a926429bb0fc7dea141c11b 
  src/tests/cluster.cpp 22167da70a855a39fd9c3ca980304372c70bd8d3 
  src/tests/command_executor_tests.cpp 970cdc39f4f2b0377d36acf2465d377d2a6e1d05 
  src/tests/container_logger_tests.cpp 71101c31cee6a400b89cf285cf0a105d2d1534a8 
  src/tests/containerizer/docker_containerizer_tests.cpp 
f6fce7df82417e029fadf805d6e0b793f396aa69 
  src/tests/containerizer/external_containerizer_test.cpp 
5e2116355418f5a0716cfd1573bab48ba75df596 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
b3bd5a03266150a0cd83c966d646a32c419bf512 
  src/tests/containerizer/isolator_tests.cpp 
6a2e25b967742c034364d19372f06aa9f9cdf828 
  src/tests/containerizer/memory_pressure_tests.cpp 
be6c3a118b528c39c534da423c15e9dcbb970dbc 
  src/tests/containerizer/port_mapping_tests.cpp 
de4b6f99f3a994bcedafa801eed9c4a7b79bac23 
  src/tests/containerizer/provisioner_docker_tests.cpp 
72d4c3e8756e1bea2332db20654af0a5fbb124f1 
  src/tests/containerizer/runtime_isolator_tests.cpp 
9f3b0b08da7cebba722062a9932fae1b5f825efb 
  src/tests/credentials_tests.cpp b61ba2ea5df8957f12659de219f6a57cf30d987a 
  src/tests/disk_quota_tests.cpp 7f5e32f3239db3adf6e4cec2df15ccf89b4f13f4 
  src/tests/exception_tests.cpp a50ccf1255dee59fdbc6fb1539bd1f6429458fb4 
  src/tests/executor_http_api_tests.cpp 
ff7b672e03185fca8b408b8805223a314fa3e483 
  src/tests/fault_tolerance_tests.cpp f99413f56e96a796d3d45decad1f049e6a238789 
  src/tests/fetcher_cache_tests.cpp 645dae208cb2b0aa2d2181d96eb1fd8893975430 
  src/tests/gc_tests.cpp 42059b2d6544f360cdc9230fe6ed33a11a15bc50 
  src/tests/health_check_tests.cpp d32164aeb1eb439bd062afa28614dd919e24f06b 
  src/tests/hook_tests.cpp 595991deab38c34e918601e85d250dc995d0f34c 
  src/tests/http_fault_tolerance_tests.cpp 
c06e07daf6d6519c10489310cd4275ae94f302c6 
  src/tests/master_allocator_tests.cpp b41ba2bda4d680f6fc42f525719973d56c11fe31 
  src/tests/master_authorization_tests.cpp 
8b9b8991fbb8c5a5beb69416a9c4a4ef3525942d 
  src/tests/master_contender_detector_tests.cpp 
bbce379e5a0a0ca608579d0ab2b10970e9cd5ef1 
  src/tests/master_maintenance_tests.cpp 
b42a81fc2e0982e8fca669bffb798c0acda684fc 
  src/tests/master_quota_tests.cpp c2b46d23002481e63ff162e8628f9b974e3e8ef9 
  src/tests/master_slave_reconciliation_tests.cpp 
988f1d46580ab5a707fe801824e24f94d4f50da7 
  src/tests/master_tests.cpp d34ba0bdd71efd261850d8c205c16cecb701ac7c 
  src/tests/master_validation_tests.cpp 
8d0070a1b8e8dcc7fe6360f8c6c6182ba9edef7d 
  src/tests/mesos.hpp 93b9340d94d91663283fe5df5ad9febe69ffd2a3 
  src/tests/mesos.cpp 90aef6bfe619dc0acdb4ccba6a7180482dd13ce5 
  src/tests/metrics_tests.cpp 51b4515369b5a72cd9613bb87e7b9df9e1118e83 
  src/tests/monitor_tests.cpp 5dcb2481ff2f1a7caf54036bc3e60c78feb982b1 
  src/tests/oversubscription_tests.cpp ba036810758d99a6fb0034c5e2bc7829e2343a44 
  src/tests/partition_tests.cpp 349adbf67686e6044a2e6a4b673043ad74fce44e 
  src/tests/persistent_volume_endpoints_tests.cpp 
d04063090e7f45b4c047a4e037eed1de79cd6958 
  src/tests/persistent_volume_tests.cpp 
26fff19daa8b175fdcc06fd9467224d5920a1967 
  src/tests/reconciliation_tests.cpp 5f541f5fe004ede943a1b022daab92f01d1f4853 
  src/tests/registrar_zookeeper_tests.cpp 
45b687cf96438e2ea8cc94ab2147e2538763e702 
  src/tests/reservation_endpoints_tests.cpp 
028e28c68e8a438d310df04fea0a7e54a6d642a3 
  src/tests/reservation_tests.cpp a9261bdf48c0af933e7fc303b7af356a60b49506 
  

Review Request 45034: Removed superfluous blank line in slave/http.cpp.

[Joerg Schad]

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

Review request for mesos, Adam B and Greg Mann.


Repository: mesos


Description
---

Removed superfluous blank line in slave/http.cpp.


Diffs
-

  src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 

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


Testing
---


Thanks,

Joerg Schad

Re: Review Request 44909: Update containerizer construction in docker_containerizer_tests.cpp.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44798, 44822, 44823, 44824, 44825, 44826, 44828, 44892, 
44893, 44894, 44895, 44896, 44897, 44898, 44899, 44900, 44901, 44902, 44903, 
44904, 44905, 44906, 44907, 44908, 44909]

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

- Mesos ReviewBot


On March 16, 2016, 11:36 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44909/
> ---
> 
> (Updated March 16, 2016, 11:36 a.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update containerizer construction in docker_containerizer_tests.cpp.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 8afaa4dab3984e9866b7b223e8e2e70ef83a39dc 
> 
> Diff: https://reviews.apache.org/r/44909/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Review Request 44907: Remove `Fetcher` in `DockerContainerizer::create`.

[haosdent huang]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Remove `Fetcher` in `DockerContainerizer::create`.


Diffs
-

  src/slave/containerizer/containerizer.cpp 
f6fc7863d0c215611f170dc0c89aa229407b5137 
  src/slave/containerizer/docker.hpp 79cd955e9c241becff52cc4bbef81dcc16802ee7 
  src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 44656: Introduced `KillPolicy` protobuf.

[Alexander Rukletsov]

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

(Updated March 18, 2016, 5:14 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

Describes a kill policy for a task. Currently does not express
different policies (e.g. hitting HTTP endpoints). For now, this
controls how long to wait between SIGTERM and SIGKILL.


Diffs (updated)
-

  include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b 
  include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b 

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


Testing
---

The complete chain was tested. See https://reviews.apache.org/r/44662/.


Thanks,

Alexander Rukletsov

Re: Review Request 44758: Upgrade to clang-format-3.8 (MESOS-4906).

[Yong Tang]

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

(Updated March 17, 2016, 9:08 p.m.)


Review request for mesos and Michael Park.


Changes
---

Uncomment "BasedOnStyle: Google" based on the feedback from Michael.


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


Repository: mesos


Description
---

Upgrade to clang-format-3.8 (MESOS-4906).


Diffs (updated)
-

  docs/clang-format.md 7f1c1dfd70e1fe9bfa186df1bdda7bdcf867db04 
  support/clang-format 499d0e749e14e50256ae649afa0ced2b04589a0e 

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


Testing
---

make check


Thanks,

Yong Tang

Review Request 44899: Update containerizer construction in slave_recovery_tests.cpp.

[haosdent huang]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Update containerizer construction in slave_recovery_tests.cpp.


Diffs
-

  src/tests/slave_recovery_tests.cpp bd7b94f3f1fac6705e5bf14c6f6103b540cde56c 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 44989: Fixed a race in the resource offers tests.

[Greg Mann]

> On March 18, 2016, 9:47 a.m., Adam B wrote:
> > "The test is slowed considerably" - Can you provide some stats on how fast 
> > it is before/after the patch?

Good call - timing information has been added, and it actually wasn't as bad as 
I thought.


- Greg


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


On March 18, 2016, 6:55 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44989/
> ---
> 
> (Updated March 18, 2016, 6:55 p.m.)
> 
> 
> Review request for mesos, Adam B and Joerg Schad.
> 
> 
> Bugs: MESOS-4849
> https://issues.apache.org/jira/browse/MESOS-4849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a race in the resource offers tests.
> 
> Adding HTTP credentials to `StartSlave` in 'src/tests/mesos.cpp' has exposed 
> a race condition in ResourceOffersTest.ResourceOfferWithMultipleSlaves. The 
> test quickly runs `StartSlave` 10 times to create 10 agents. Under the 
> covers, `StartSlave` writes data to disk, and it seems that with the 
> additional data being written to disk for HTTP credentials, the filesystem 
> operations for one `StartSlave` call were not completing before the next call.
> 
> By settling the clock in between each invocation of `StartSlave`, this patch 
> fixes the race. The test is slower, but it is now reliable. Running the test 
> 3 times, the old implementation gives an average runtime of 265ms, while the 
> new one runs in an average of 359ms.
> 
> 
> Diffs
> -
> 
>   src/tests/resource_offers_tests.cpp 
> 1cf292ee7931207596f8f06677386bef5965ef15 
> 
> Diff: https://reviews.apache.org/r/44989/diff/
> 
> 
> Testing
> ---
> 
> `GTEST_FILTER="ResourceOffersTest.ResourceOfferWithMultipleSlaves" 
> bin/mesos-tests.sh --gtest_repeat=1000 --gtest_break_on_failure=1` was used 
> to test on both OSX and Ubuntu 14.04.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 44903: Update containerizer construction in port_mapping_tests.cpp.

[haosdent huang]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Update containerizer construction in port_mapping_tests.cpp.


Diffs
-

  src/tests/containerizer/port_mapping_tests.cpp 
a1427fd0157dee343b643f3272dba8ffea61f7b0 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 44997: Fixed how we detect C++11 compiler support.

[Joris Van Remoortere]

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


Ship it!




Ship It!

- Joris Van Remoortere


On March 17, 2016, 11:40 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44997/
> ---
> 
> (Updated March 17, 2016, 11:40 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4963
> https://issues.apache.org/jira/browse/MESOS-4963
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adopt a new version of the M4 macro from the upstream macro archive.
> We no longer care about whether the compiler supports particular
> C++11 features, so we can discard some previous local modifications
> we had to this macro. This should also make it easier to mandate
> C++14 support in the future.
> 
> 
> Diffs
> -
> 
>   configure.ac 8e4f03593df4a8ba13f00292963e351acc3f71c1 
>   m4/ax_cxx_compile_stdcxx.m4 PRE-CREATION 
>   m4/ax_cxx_compile_stdcxx_11.m4 6a859b89953df2132a75f61833d5d3316968ec99 
> 
> Diff: https://reviews.apache.org/r/44997/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 44766: Enabled Authentication information in endpoint HELP.

[Joerg Schad]

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

(Updated March 17, 2016, 5:01 p.m.)


Review request for mesos, Adam B and Greg Mann.


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


Repository: mesos


Description
---

Enabled endpoint HELP string to display information about authentication 
requirements.


Diffs (updated)
-

  3rdparty/libprocess/include/process/help.hpp 
783304e2fc78db70f1a5fccbf5e96fcc76a88fd8 
  3rdparty/libprocess/src/help.cpp 5f368801affecacb0d1daaeb6ccf5895ccb231d2 
  3rdparty/libprocess/src/logging.cpp 015a43db77fd7015aeee0c45fa10c292f3e9cf58 

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


Testing
---

Viewed master endpoint help in browser.


Thanks,

Joerg Schad

Re: Review Request 44974: Added device support in cgroups abstraction.

[Kevin Klues]

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



I ran the review by Ben again and sat with him to rework it to his 
satisfaction. The commit can be found on this branch:
https://github.com/klueska-mesosphere/mesos/commits/r/a10gupta/cgroups-devices

If you just apply the patch as is, Ben says he will get it committed early next 
week.

- Kevin Klues


On March 18, 2016, 9:41 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44974/
> ---
> 
> (Updated March 18, 2016, 9:41 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Felix Abecassis, Kevin Klues, and 
> Niklas Nielsen.
> 
> 
> Bugs: MESOS-3368
> https://issues.apache.org/jira/browse/MESOS-3368
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> There are some helper methods added to support device cgroups in cgroups 
> abstraction to aid isolators controlling access to devices.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 51ccefd1fd2d0989b9bd31342d3d1e3701f88ed2 
>   src/linux/cgroups.cpp df18ed46a2a96871f67c7eb4233c3b4c27b7aa1c 
> 
> Diff: https://reviews.apache.org/r/44974/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 44139: Windows:[2/2] Lifted socket API into Stout.

[Daniel Pravat]

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

(Updated March 17, 2016, 6:52 p.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Alex 
Clemmer, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Repository: mesos


Description
---

Windows:[2/2] Lifted socket API into Stout.


Diffs (updated)
-

  3rdparty/libprocess/include/process/network.hpp 
7d203f0ff1cdb3145bc2b914f8bd606203878f09 

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


Testing (updated)
---

OSX: make check
Windows: build/execute


Thanks,

Daniel Pravat

Review Request 45015: Windows: Fixed bug causing `os::exists` to report invalid paths exist.

[Alex Clemmer]

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

Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Repository: mesos


Description
---

Currently on Windows, `os::exists` will return true if a component of a
path does not exist. For example if you have `a/fancy/path`, and you ask
`os::exists("a/fake/path")`, the result currently reports `true`. In
other words, the Windows code path only checks for the error that a file
does not exist, and ignores the error that says the path is not valid.

This commit will fix this, and also add a test that will verify we don't
regress.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/exists.hpp 
9211851e4562e04045276421b359c3c78cdae7f1 
  3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp 
4c30189bb8261ccfc699da0f31b8b1fd3e9b3c83 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 44994: Added a test for executor shutdown grace period.

[Alexander Rukletsov]

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

(Updated March 18, 2016, 5:13 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description (updated)
---

See summary.


Diffs (updated)
-

  src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169 

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


Testing
---

On Mac OS 10.10.4:
`make check`
`GLOG_v=2 GTEST_FILTER="*SlaveTest*" ./bin/mesos-tests.sh --gtest_repeat=100 
--gtest_break_on_failure`


Thanks,

Alexander Rukletsov

Re: Review Request 44553: Added authentication to agent HTTP endpoints.

[Greg Mann]

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

(Updated March 17, 2016, 10:56 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Changed dependency.


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


Repository: mesos


Description
---

Added authentication to agent HTTP endpoints.

This patch adds HTTP authentication to the `/state`, `/state.json`, and 
`/flags` endpoints. Tests are also updated to use authentication when hitting 
these endpoints, and a new test was added to probe these endpoints' behavior 
when bad credentials are supplied: `SlaveTest.HTTPEndpointsBadAuthentication`.


Diffs
-

  src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 
  src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
  src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
  src/tests/containerizer/docker_containerizer_tests.cpp 
f6fce7df82417e029fadf805d6e0b793f396aa69 
  src/tests/fault_tolerance_tests.cpp f99413f56e96a796d3d45decad1f049e6a238789 
  src/tests/health_check_tests.cpp d32164aeb1eb439bd062afa28614dd919e24f06b 
  src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169 

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


Testing
---

Tests were updated to use authentication when hitting the affected agent 
endpoints, and new tests were added to probe the endpoint behavior when invalid 
credentials are supplied.

`sudo make check` was used to test on both OSX and Ubuntu 14.04. The new test 
was run 1000 times to look for flakiness.


Thanks,

Greg Mann

Re: Review Request 44675: Updated `/metrics/snapshot` endpoint to use `jsonify`.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On March 10, 2016, 9:59 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44675/
> ---
> 
> (Updated March 10, 2016, 9:59 p.m.)
> 
> 
> Review request for mesos, Michael Park, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4740
> https://issues.apache.org/jira/browse/MESOS-4740
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated `/metrics/snapshot` endpoint to use `jsonify`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/metrics/metrics.cpp 
> f1e6774ebf8670b006ba6ea181439d0ef1529b40 
> 
> Diff: https://reviews.apache.org/r/44675/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Re: Review Request 44954: Regenerated master endpoint documentation.

[Joerg Schad]

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

(Updated March 18, 2016, 4:48 p.m.)


Review request for mesos, Adam B and Greg Mann.


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


Repository: mesos


Description
---

Reran the generate-endpoint-help.py scipt after adding the AUTHENTICATION 
information to HELP.


Diffs (updated)
-

  docs/endpoints/master/api/v1/scheduler.md 
7a75fd75eef458f26f9e5067b792329f310d5d24 
  docs/endpoints/master/create-volumes.md 
2ab116a16581fc5cfc0fc5cf70c6e001a28d8493 
  docs/endpoints/master/destroy-volumes.md 
88679fe12baf7642b63e71688b90031aaedab1ef 
  docs/endpoints/master/flags.md 739d6bfae5ae35223d6bc7c4782ff0f16b868018 
  docs/endpoints/master/frameworks.md 95c1f0e63f6a61d0e6622a8ec9d821a646656cfc 
  docs/endpoints/master/health.md 7f9352025a6364b812b91e9ccbe380a3a69ba4e1 
  docs/endpoints/master/machine/down.md 
6eec9ecc6e92fd1299b27499f78055c6f4f68e81 
  docs/endpoints/master/machine/up.md 7cbbf04859a218f90672453e5037480676f79fe5 
  docs/endpoints/master/maintenance/schedule.md 
5f5267d24e392d6302d126e2367be08e6cc5e174 
  docs/endpoints/master/maintenance/status.md 
4d0c7551acb89fb375834fd703c406d68f8bdcfc 
  docs/endpoints/master/observe.md fae1ee062350d7b25775bef98a0bebda1892ab62 
  docs/endpoints/master/quota.md 812874d2dd6c3548887e3044ba1f3c3c8c9d1dd6 
  docs/endpoints/master/redirect.md ac9d0fa3eae485726b10a3ac756228d0cb5aeb27 
  docs/endpoints/master/reserve.md 1a4f67961baf761f79a693780f42a1a8ce2244fc 
  docs/endpoints/master/roles.json.md 863715386791ef9192f38bf390fcd2b31d988547 
  docs/endpoints/master/roles.md 171e0163c4c2db34bf34c8303846793f3c29bdf5 
  docs/endpoints/master/slaves.md ec169c15507d3c731b8833f2656949caf0efcc33 
  docs/endpoints/master/state-summary.md 
fb10ac7db5b94ea7982c245f1d884312d818c6b5 
  docs/endpoints/master/state.json.md 0415cfdd63cee0b350b3e33c496bacfc1ba53f8a 
  docs/endpoints/master/state.md ae1ec718b8d718718727dbd2e7ce4739cca41680 
  docs/endpoints/master/tasks.json.md 46a1253d33cdfdca0a7158808d1e29da1a634933 
  docs/endpoints/master/tasks.md 2ada97b5dfdaf5f1f7578fa42eb4e61233e6a753 
  docs/endpoints/master/teardown.md f68d083a4271ed7dd0ddcc87da104a43eb40c7ec 
  docs/endpoints/master/unreserve.md e059d8d888343c5036346f7a615f04375f44f517 
  docs/endpoints/master/weights.md 632be242d85fbd61ded62d846e385009a321533c 

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


Testing
---

viewed docs with docker-website generator.


Thanks,

Joerg Schad

Re: Review Request 44656: Introduced `KillPolicy` protobuf.

[Alexander Rukletsov]

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

(Updated March 17, 2016, 11:36 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

Describes a kill policy for a task. Currently does not express
different policies (e.g. hitting HTTP endpoints). For now, this
controls how long to wait between SIGTERM and SIGKILL.


Diffs (updated)
-

  include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b 
  include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b 

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


Testing
---

The complete chain was tested. See https://reviews.apache.org/r/44662/.


Thanks,

Alexander Rukletsov

Review Request 44896: Update containerizer construction in disk_quota_tests.cpp.

[haosdent huang]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Update containerizer construction in disk_quota_tests.cpp.


Diffs
-

  src/tests/disk_quota_tests.cpp 413e562026a4fc9779f616e921ae2fa2ca51e012 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 44985: Fix the broken ProvisionerDockerPullerTest on Centos7.

[Jie Yu]

> On March 17, 2016, 10:12 p.m., Gilbert Song wrote:
> > src/tests/containerizer/provisioner_docker_tests.cpp, line 401
> > 
> >
> > `_NonShellCommand`?

I'll rename it to SimpleCommand.


- Jie


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


On March 17, 2016, 10 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44985/
> ---
> 
> (Updated March 17, 2016, 10 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Joseph Wu.
> 
> 
> Bugs: MESOS-4810
> https://issues.apache.org/jira/browse/MESOS-4810
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix the broken ProvisionerDockerPullerTest on Centos7.
> 
> See the ticket for root cause.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 72d4c3e8756e1bea2332db20654af0a5fbb124f1 
>   src/tests/mesos.hpp 93b9340d94d91663283fe5df5ad9febe69ffd2a3 
> 
> Diff: https://reviews.apache.org/r/44985/diff/
> 
> 
> Testing
> ---
> 
> sudo make check.
> 
> Verified in our CI.
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 44655: Made `shutdown_grace_period` configurable in `ExecutorInfo`.

[Alexander Rukletsov]

> On March 15, 2016, 8:11 p.m., Ben Mahler wrote:
> > include/mesos/v1/executor/executor.proto, lines 53-55
> > 
> >
> > Now that the period within the Shutdown message was removed, we should 
> > probably have a TODO following this note for adding the period into the 
> > Shutdown message so that the agent can communicate when a shorter period 
> > has been alloted.

I will add this TODO as food for thought, but I'm not sure we should introduce 
one, because even if we communicate the "actual" period, a failure may occur 
which will render this "actual" period incorrect.


> On March 15, 2016, 8:11 p.m., Ben Mahler wrote:
> > src/exec/exec.cpp, lines 715-716
> > 
> >
> > Can you confirm the environment variable setting didn't make it in 
> > 0.28.0?

Good catch, Ben, that change made it to 0.28 indeed.


- Alexander


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


On March 15, 2016, 3:49 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44655/
> ---
> 
> (Updated March 15, 2016, 3:49 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Gilbert Song.
> 
> 
> Bugs: MESOS-4949
> https://issues.apache.org/jira/browse/MESOS-4949
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If `ExecutorInfo.shutdown_grace_period` is set, the executor
> driver uses it, otherwise it falls back to the environment
> variable `MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD`.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 739d4ff9aeeb1ba70ce11033168d63d37b6ef56b 
>   include/mesos/executor/executor.proto 
> ae211194a44e0bf2fadc79e833881e45ea3eb2c2 
>   include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b 
>   include/mesos/v1/executor/executor.proto 
> 36a2b3f9bc3aaa524f655b9e686a6d33512e6aaa 
>   include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b 
>   src/exec/exec.cpp 741786132f3a8cc43f5b9ced262429038832a946 
>   src/executor/executor.cpp 87db4e02cbaa778aab0173741bfe066fdee9a48d 
>   src/slave/flags.cpp 4d10818105627738e258116647ccada374e3d7b9 
>   src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
> 
> Diff: https://reviews.apache.org/r/44655/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44985: Fix the broken ProvisionerDockerPullerTest on Centos7.

[Jie Yu]

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

(Updated March 17, 2016, 10 p.m.)


Review request for mesos, Bernd Mathiske and Joseph Wu.


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


Repository: mesos


Description (updated)
---

Fix the broken ProvisionerDockerPullerTest on Centos7.

See the ticket for root cause.


Diffs
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
72d4c3e8756e1bea2332db20654af0a5fbb124f1 
  src/tests/mesos.hpp 93b9340d94d91663283fe5df5ad9febe69ffd2a3 

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


Testing
---

sudo make check.

Verified in our CI.


Thanks,

Jie Yu

Re: Review Request 37168: Add an example framework using dynamic reservation.

[Greg Mann]

> On March 16, 2016, 6:27 p.m., Greg Mann wrote:
> > src/examples/dynamic_reservation_framework.cpp, line 119
> > 
> >
> > Could you clarify for me how the slave gets into the RESERVED state the 
> > first time? It seems to me we should have something here similar to what's 
> > in the switch for `State::UNRESERVING`, i.e., check the offer and see if 
> > the reserved resources are contained in it? The framework test passes for 
> > me though, so it seems to be working nonetheless?
> 
> Klaus Ma wrote:
> Good catch! I think I missed `updateState(dispatched, offer.slave_id(), 
> State::RESERVED);` just before launching task; it works because both 
> `State::RESERVED` and `State::RESERVING` are using the same logic.
> 
> When slave transfer from `State::RESERVING` to `State::RESERVED`, it's 
> better to launch tasks directly; otherwise, we have to wait for another 
> allocation interval to launch task. I used to try following code to seperate 
> action in those two state, but `case State::RESERVING:` without `break;` is 
> not a good style :).
> 
> ```
> case State::RESERVING:
>   {
> Resources resources = offer.resources();
> Resources reserved = resources.reserved(role);
> 
> if (reserved.contains(taskResources)) {
>   updateState(dispatched, offer.slave_id(), State::RESERVED);
> }
>   }
>   // NOTE: No `break;`. Launch task after transfering to
>   // `State::RESERVED`.
> case State::RESERVED:
>   {
> if (tasksLaunched == totalTasks) {
>   // If all tasks were launched, unreserve those resources.
>   Try reserved = unreserveResources(driver, offer);
>   updateState(reserved, offer.slave_id(), State::UNRESERVING);
> } else if (tasksLaunched < totalTasks) {
>   // Framework dispatches task on the reserved resources.
>   Try dispatched = dispatchTasks(driver, offer);
>   updateState(dispatched, offer.slave_id(), 
> State::TASK_RUNNING);
> }
>   }
>   break;
> ```

Ah yea, this makes sense :-) Thanks for the explanation! Do you think you could 
add a comment to the code here to explain this reasoning?


- Greg


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


On March 17, 2016, 6:15 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37168/
> ---
> 
> (Updated March 17, 2016, 6:15 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Joerg Schad, and Michael Park.
> 
> 
> Bugs: MESOS-3063
> https://issues.apache.org/jira/browse/MESOS-3063
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Provide example for dynamic reservation features.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
>   src/examples/dynamic_reservation_framework.cpp PRE-CREATION 
>   src/tests/dynamic_reservation_framework_test.sh PRE-CREATION 
>   src/tests/examples_tests.cpp 6ddac17bb2ac0330bcc09eaab975ae70e84a7695 
> 
> Diff: https://reviews.apache.org/r/37168/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 44851: Renamed an allocator metric.

[Ben Mahler]

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




src/master/allocator/mesos/metrics.hpp (lines 40 - 41)


Can you clarify here what the deprecation cycle is? It would be helpful to 
say that the new one was added in 0.29.0.

Could you also update the CHANGELOG with this deprecation and point users 
towards using the new metric name?


- Ben Mahler


On March 15, 2016, 2:51 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44851/
> ---
> 
> (Updated March 15, 2016, 2:51 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since allocators can be replaced with a custom module instead use a
> name clearly communicating that the metrics reported here are related
> to the default (hierarchical) allocator.
> 
> For backwards compatibility we continue to support the old metrics
> name.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/metrics.hpp 
> d04e9a11c77b6fb2d522608e3085f81f8a6657ca 
>   src/master/allocator/mesos/metrics.cpp 
> 46dd7f7b3d8d49c59b42e789fe5dcd49f1693ace 
> 
> Diff: https://reviews.apache.org/r/44851/diff/
> 
> 
> Testing
> ---
> 
> check make check (OS X, clang-trunk) here and with later patches using other 
> allocator metrics.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

[Qian Zhang]

> On March 12, 2016, 2:45 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 358
> > 
> >
> > If we block here, it will lock the isolator, and prevent the 
> > `MesosContainerizer` from launching any more containers?
> 
> Qian Zhang wrote:
> I do not think it will prevent `MesosContainerizer` from launching any 
> more containers. Please take a look at: 
> https://github.com/apache/mesos/blob/0.27.2/src/slave/containerizer/mesos/containerizer.cpp#L945:L1034,
>  the whole code to call launcher to fork executor process and call isolator's 
> `isolate()` are already in a `defer()` as a lambda, and it is also a very big 
> lambda.
> 
> Avinash sridharan wrote:
> Hi Qian,
>  Didn't understand your comment. Are you implying calling `await` on the 
> `Future` won't block?
> 
> Qian Zhang wrote:
> I think it will not block `MesosContainerizerProcess`, but it will indeed 
> block `NetworkCniIsolatorProcess`. So yes, I agree we should use `defer`. And 
> I have another question, for the container which want to join multiple 
> networks, do we want to call plugin to join multiple networks in parallel or 
> sequentially? I think joining one network should be independent on joining 
> another, but if we do them in parallel, I am afraid that user may see "`eth2, 
> eth0, eth1`" rather than "`eth0, eth1, eth2`" in the container. Please let me 
> know your thought :-)

Second thought: since one libprocess can only handle one event at a time, I 
think container has to join multiple networks sequentially, so we do not need 
to worry about the interface names may be disordered.


- Qian


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


On March 16, 2016, 3:50 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 16, 2016, 3:50 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44553: Added authentication to agent HTTP endpoints.

[Greg Mann]

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

(Updated March 18, 2016, 7:12 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Added authentication to agent HTTP endpoints.

This patch adds HTTP authentication to the `/state`, `/state.json`, and 
`/flags` endpoints. Tests are also updated to use authentication when hitting 
these endpoints, and a new test was added to probe these endpoints' behavior 
when bad credentials are supplied: `SlaveTest.HTTPEndpointsBadAuthentication`.


Diffs (updated)
-

  src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 
  src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
  src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
  src/tests/containerizer/docker_containerizer_tests.cpp 
f6fce7df82417e029fadf805d6e0b793f396aa69 
  src/tests/fault_tolerance_tests.cpp f99413f56e96a796d3d45decad1f049e6a238789 
  src/tests/health_check_tests.cpp d32164aeb1eb439bd062afa28614dd919e24f06b 
  src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169 

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


Testing
---

Tests were updated to use authentication when hitting the affected agent 
endpoints, and new tests were added to probe the endpoint behavior when invalid 
credentials are supplied.

`sudo make check` was used to test on both OSX and Ubuntu 14.04. The new test 
was run 1000 times to look for flakiness.


Thanks,

Greg Mann

Re: Review Request 44657: Used `KillPolicy` and shutdown grace period in command executor.

[Ben Mahler]

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




src/launcher/executor.cpp (lines 496 - 499)


I added this TODO for killTask, can you help by clarifying that the 
shutdown arrives after a kill task?


- Ben Mahler


On March 18, 2016, 5:19 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44657/
> ---
> 
> (Updated March 18, 2016, 5:19 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Gilbert Song.
> 
> 
> Bugs: MESOS-4909
> https://issues.apache.org/jira/browse/MESOS-4909
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The command executor determines how much time it allots the
> underlying task to clean up (effectively how long to wait for
> the task to comply to SIGTERM before sending SIGKILL) based
> on both optional task's `KillPolicy` and optional
> `shutdown_grace_period` field in `ExecutorInfo`.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 4149f084b0b234fb995df04e9d1ca704feec082e 
>   src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
> 
> Diff: https://reviews.apache.org/r/44657/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 45040: Added a test for task's kill policy.

[Ben Mahler]

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



Thanks for the test! Although I wouldn't want to ship this unless you show some 
manual testing in the 'testing done' section, since this test doesn't exercise 
the functionality :)


src/tests/slave_tests.cpp (line 3372)


No need for the NOTE (per the previous test review feedback).



src/tests/slave_tests.cpp (lines 3393 - 3431)


Per the feedback from the other test review, let's pull this up into a test 
within master_validation_tests.cpp. It can be a trivial unit test since the 
validation function is stateless :)



src/tests/slave_tests.cpp (lines 3419 - 3424)


You won't need to bother with this once we pull out the validation test, 
right?



src/tests/slave_tests.cpp (lines 3433 - 3477)


Once you pull out the validation test you won't need this scope anymore.

This is just testing that we don't mutate the grace period? Can you add a 
TODO to the top of this test to mention that we should also actually test that 
it does what it is supposed to do?

Did you manually test this? I wouldn't feel comfortable shipping this 
unless you did, since this test doesn't cover the signal escalation behavior :)



src/tests/slave_tests.cpp (lines 3442 - 3444)


You shouldn't need to do this once the validation test is pulled out, right?



src/tests/slave_tests.cpp (lines 3466 - 3476)


It doesn't look like you need to bother sending a status update here, just 
use FutureArg instead of SaveArg and wait for the argument to arrive. In 
general we try to avoid SaveArg since it doesn't indicate when the value 
arrives.



src/tests/slave_tests.cpp (line 3474)


Please sweep for s/.get()./->/



src/tests/slave_tests.cpp (line 3482)


Why the resume here?


- Ben Mahler


On March 18, 2016, 5:17 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45040/
> ---
> 
> (Updated March 18, 2016, 5:17 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4909
> https://issues.apache.org/jira/browse/MESOS-4909
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169 
> 
> Diff: https://reviews.apache.org/r/45040/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.104:
> `make check`
> `GTEST_FILTER="*TaskKillPolicy*" ./bin/mesos-tests.sh --gtest_repeat=100 
> --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44755: Added getAppcImage for Appc provisioning tests.

[Guangya Liu]

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




src/tests/containerizer/provisioner_appc_tests.cpp (lines 67 - 73)


It is better change comments to the following format:

/* description
 *
 * @param 
 * 
 * return
 */


- Guangya Liu


On 三月 15, 2016, 6:26 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44755/
> ---
> 
> (Updated 三月 15, 2016, 6:26 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper function to get an Appc image protobuf object with given
> parameters.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 069af8a0b5e031d9032fd13154cee250f279b404 
> 
> Diff: https://reviews.apache.org/r/44755/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 43615: Update test suite to use the reworked MesosTest helpers.

[Jie Yu]

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




src/tests/containerizer/port_mapping_tests.cpp (line 2086)


This needs be fixed as well.


- Jie Yu


On March 16, 2016, 12:21 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43615/
> ---
> 
> (Updated March 16, 2016, 12:21 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Includes the following changes:
> 
> * Added the `` header where appropriate.
> * Added the namespace `using process::Owned;` where appropriate.
> * Generally replaced `Try` with `Try`.  
> And `Try` with `Try`.
> * Added the (now required) `MasterDetector` argument to all slaves.  Before, 
> this was fetched from the first master in `Cluster`.
> * Removed `Shutdown();` from all tests.
> * Replaced `Stop(...)` with the appropriate master/slave destruction calls.
> * Wrap various slave objects in `Owned` (i.e. containerizers, isolators, 
> launchers, etc).
> * Replace `CHECK` in tests with `ASSERT`.
> 
> 
> Diffs
> -
> 
>   src/tests/authentication_tests.cpp 85f14c3d453ca5aeffa1c915f38fe3031c2cf712 
>   src/tests/command_executor_tests.cpp 
> 0d2fcf6d4b8d9a925eb6748e6bd33cf279b8f7f8 
>   src/tests/container_logger_tests.cpp 
> 00f4129e46aa9268fbb66da25b34e61004fa87b2 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 8afaa4dab3984e9866b7b223e8e2e70ef83a39dc 
>   src/tests/containerizer/external_containerizer_test.cpp 
> 8e1dbe306a088eb16cd3b9c6174b95fad5685da4 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> e72239a55724f1aeeec5362cc370c93dbeca7164 
>   src/tests/containerizer/isolator_tests.cpp 
> 342037ce0a5f8caa4e3cf1550b8f9a7cc328acf9 
>   src/tests/containerizer/memory_pressure_tests.cpp 
> 03879d99c371f296f8d9904666911b34209c114d 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 
>   src/tests/containerizer/port_mapping_tests.cpp 
> a1427fd0157dee343b643f3272dba8ffea61f7b0 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> b2663a83afccbb156f10b4703f29ee0f638384bf 
>   src/tests/containerizer/runtime_isolator_tests.cpp 
> 1625614f560a4c9a644b1b9e9568199f30373ab5 
>   src/tests/credentials_tests.cpp 7edcc857e0f6f8e80e265deeec59d6349d392224 
>   src/tests/disk_quota_tests.cpp 413e562026a4fc9779f616e921ae2fa2ca51e012 
>   src/tests/exception_tests.cpp 6b71316d545e97f14a45daa14d0fd95204befd3b 
>   src/tests/executor_http_api_tests.cpp 
> 2fc0893f5f5e80a783296fb31b30abe86d92df1b 
>   src/tests/fault_tolerance_tests.cpp 
> 67f362747ac4301a74693aeaa5631b7dd866e782 
>   src/tests/gc_tests.cpp 61a8abb9581dc4602b197a88a677b19386969cbf 
>   src/tests/health_check_tests.cpp 9605859b665645654bbdb2688455cae2d692a057 
>   src/tests/hook_tests.cpp bb287c77493209e663a37e7f88d09a0459855f7d 
>   src/tests/http_fault_tolerance_tests.cpp 
> 7c7f3d90210148176e83553346100a506f263591 
>   src/tests/master_allocator_tests.cpp 
> cba7c36471f93b678d94e1da0251a28a893696b1 
>   src/tests/master_authorization_tests.cpp 
> 6a4de4709960d7ca505e99396e14a1bb51d6902d 
>   src/tests/master_contender_detector_tests.cpp 
> 255ab8119a04b55bb4f1b61dee19c4be64499376 
>   src/tests/master_quota_tests.cpp f1277f03f5b5cf735fea7ba324a27614a829cd50 
>   src/tests/master_slave_reconciliation_tests.cpp 
> d41178eb41df519073fc0890c5716bbc9fed6ad2 
>   src/tests/master_tests.cpp e8c39e775d6ca218ce74cfc6bb50c7576d73e90e 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
>   src/tests/metrics_tests.cpp 419d275e0b32817388120222bd433ee6f4835efd 
>   src/tests/monitor_tests.cpp 869c9e032817e8859a968232d4a61556a3d53d45 
>   src/tests/oversubscription_tests.cpp 
> e528476cd83b0e3f7ae8cea7d86dfabc1f66484e 
>   src/tests/partition_tests.cpp 3776a0a104582f60b9f19ea58b011485194399b9 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> b5f425ab7f2b4a540418d761bea145565467bd2b 
>   src/tests/persistent_volume_tests.cpp 
> e9215de2e073025f67cdc73e8a8de38cf030671f 
>   src/tests/rate_limiting_tests.cpp 352a5e7a58b500c25c7c8a421047c4e79434e38e 
>   src/tests/reconciliation_tests.cpp e8f3f29836652d20a6ee1bb5231a15e71eb76990 
>   src/tests/registrar_zookeeper_tests.cpp 
> 3df9779ee5d076e16f6a538326693a36f986b6d0 
>   src/tests/repair_tests.cpp 0dfb557a62f64baf2334dbc9f75ecdfb10473c2d 
>   src/tests/reservation_endpoints_tests.cpp 
> 

Re: Review Request 44989: Fixed a race in the resource offers tests.

[Benjamin Bannier]

> On March 18, 2016, 12:16 a.m., Neil Conway wrote:
> > src/tests/resource_offers_tests.cpp, line 63
> > 
> >
> > Style-wise, do we want all tests to resume if they initially pause it? 
> > I think we do a mix of both.
> 
> Greg Mann wrote:
> Hmmm good question. I know that I'm responsible for some instances where 
> `resume` is *not* called at the end of the test, but now that you mention it, 
> it does seem like a good idea. I've added it here. We should do a sweep and 
> make this consistent across the tests; I could have a look next week.

Note that if an `ASSERT*` fails a `Clock::resume` at the end of the test would 
never be called, so one would expect something like this to be handled 
elsewhere, something we already do, see 
https://github.com/apache/mesos/blob/master/src/tests/mesos.hpp#L984 or 
https://github.com/apache/mesos/blob/master/3rdparty/libprocess/src/tests/main.cpp#L59.


- Benjamin


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


On March 18, 2016, 10:48 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44989/
> ---
> 
> (Updated March 18, 2016, 10:48 a.m.)
> 
> 
> Review request for mesos, Adam B and Joerg Schad.
> 
> 
> Bugs: MESOS-4849
> https://issues.apache.org/jira/browse/MESOS-4849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a race in the resource offers tests.
> 
> Adding HTTP credentials to `StartSlave` in 'src/tests/mesos.cpp' has exposed 
> a race condition in ResourceOffersTest.ResourceOfferWithMultipleSlaves. The 
> test quickly runs `StartSlave` 10 times to create 10 agents. Under the 
> covers, `StartSlave` writes data to disk, and it seems that with the 
> additional data being written to disk for HTTP credentials, the filesystem 
> operations for one `StartSlave` call were not completing before the next call.
> 
> By settling the clock in between each invocation of `StartSlave`, this patch 
> fixes the race. The test is slowed considerably, but it is now reliable.
> 
> 
> Diffs
> -
> 
>   src/tests/resource_offers_tests.cpp 
> 1cf292ee7931207596f8f06677386bef5965ef15 
> 
> Diff: https://reviews.apache.org/r/44989/diff/
> 
> 
> Testing
> ---
> 
> `GTEST_FILTER="ResourceOffersTest.ResourceOfferWithMultipleSlaves" 
> bin/mesos-tests.sh --gtest_repeat=1000 --gtest_break_on_failure=1` was used 
> to test on both OSX and Ubuntu 14.04.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 44810: Documentation change for the update quota feature.

[Zhitao Li]

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




docs/quota.md (lines 157 - 163)


Note that this part is unsettled. Will update once consensus is reached in 
design doc.


- Zhitao Li


On March 15, 2016, 3:12 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44810/
> ---
> 
> (Updated March 15, 2016, 3:12 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van 
> Remoortere, and Michael Browning.
> 
> 
> Bugs: MESOS-4941
> https://issues.apache.org/jira/browse/MESOS-4941
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documentation change for the update quota feature.
> 
> This serves as the first patch in the review chain for quota update feature 
> and documents the current plan. Actual implementations to follow once we 
> reach consensus on the this plan.
> 
> 
> Diffs
> -
> 
>   docs/quota.md 12696bf805d43f997d80149e56281c5e7dc0557e 
> 
> Diff: https://reviews.apache.org/r/44810/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 44934: Updated mesos-execute to add support for Appc.

[Jojy Varghese]

> On March 17, 2016, 2:16 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, lines 251-281
> > 
> >
> > What is the behavior of this if with mesos containerizer but without a 
> > docker/appc image?
> > 
> > The current logic is a bit different from previous logic, the mesos 
> > execute will not construct the containerInfo if no docker image.

Thanks for the review ! Updated the patch.


- Jojy


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


On March 16, 2016, 9:21 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> ---
> 
> (Updated March 16, 2016, 9:21 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated mesos-execute to add support for Appc.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e 
> 
> Diff: https://reviews.apache.org/r/44934/diff/
> 
> 
> Testing
> ---
> 
> Tested with various Appc images.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 44758: Upgrade to clang-format-3.8 (MESOS-4906).

[Michael Park]

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


Ship it!




The patch looks good to me. Would you like to send an announcement email to the 
dev list about this upgrade? I can do it if you'd rather not.

- Michael Park


On March 16, 2016, 1:37 a.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44758/
> ---
> 
> (Updated March 16, 2016, 1:37 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-4906
> https://issues.apache.org/jira/browse/MESOS-4906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Upgrade to clang-format-3.8 (MESOS-4906).
> 
> 
> Diffs
> -
> 
>   docs/clang-format.md 7f1c1dfd70e1fe9bfa186df1bdda7bdcf867db04 
>   support/clang-format 499d0e749e14e50256ae649afa0ced2b04589a0e 
> 
> Diff: https://reviews.apache.org/r/44758/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Steve Niemitz]

> On March 16, 2016, 4:27 p.m., Jie Yu wrote:
> > Ship It!
> 
> Jie Yu wrote:
> Can you confirm that this patch works in your environment?

Actually, it looks like the change to use update() instead of _update 
introduced a bug (the cgroups no longer get updated because the resources don't 
change).  I'll submit a fix in a minute.


- Steve


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


On Feb. 20, 2016, 6:47 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated Feb. 20, 2016, 6:47 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 44909: Update containerizer construction in docker_containerizer_tests.cpp.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44798, 44822, 44823, 44824, 44825, 44826, 44828, 44892, 
44893, 44894, 44895, 44896, 44897, 44898, 44899, 44900, 44901, 44902, 44903, 
44904, 44905, 44906, 44907, 44908, 44909]

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

- Mesos ReviewBot


On March 16, 2016, 5:57 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44909/
> ---
> 
> (Updated March 16, 2016, 5:57 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update containerizer construction in docker_containerizer_tests.cpp.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> f6fce7df82417e029fadf805d6e0b793f396aa69 
> 
> Diff: https://reviews.apache.org/r/44909/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 43629: Especially updated tests to use the updated MesosTest helpers.

[Joseph Wu]

> On March 16, 2016, 5:48 a.m., Michael Park wrote:
> > src/tests/fetcher_cache_tests.cpp, lines 167-169
> > 
> >
> > Could you explain this NOTE? It looks like we do `fetcherProcess = new 
> > MockFetcherProcess();` but we wrap it in an `Owned` and pass it to 
> > `fetcher`?

By "violate" I mean that the tests will use and modify the `fetcherProcess` 
even though it is owned elsewhere.  I've gotten around this pattern in the past 
by instantiating the mock (and the expectations) before passing ownership 
along.  But that doesn't quite work here.

i.e. You'll see things like this in the tests:
```
  EXPECT_CALL(*fetcherProcess, _fetch(_, _, _, _, _, _))
.WillRepeatedly(
DoAll(SatisfyOne(),
  Invoke(fetcherProcess, ::unmocked__fetch)));
```

This could be considered a TODO for future cleanup.


- Joseph


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


On March 15, 2016, 1:48 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43629/
> ---
> 
> (Updated March 15, 2016, 1:48 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Continuation of https://reviews.apache.org/r/43615/ with a slightly different 
> pattern.
> 
> 
> Diffs
> -
> 
>   src/tests/dynamic_weights_tests.cpp 
> a95663f633f376954d4201b6cfbe693429be9c39 
>   src/tests/fetcher_cache_tests.cpp 776c95267caff7b27cd70c2fa6149d8158c86750 
>   src/tests/resource_offers_tests.cpp 
> 0bad45dd1dabecc88fef1ab46e8ea26718070b33 
>   src/tests/slave_recovery_tests.cpp bd7b94f3f1fac6705e5bf14c6f6103b540cde56c 
> 
> Diff: https://reviews.apache.org/r/43629/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43911: Updated `/state` agent endpoint to use jsonify.

[Neil Conway]

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

(Updated March 16, 2016, 9:14 p.m.)


Review request for mesos and Michael Park.


Changes
---

Rebase.


Repository: mesos


Description
---

Updated `/state` agent endpoint to use jsonify.


Diffs (updated)
-

  src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 44902: Update containerizer construction in memory_pressure_tests.cpp.

[haosdent huang]

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

(Updated March 16, 2016, 5:57 p.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Rebase.


Repository: mesos


Description
---

Update containerizer construction in memory_pressure_tests.cpp.


Diffs (updated)
-

  src/tests/containerizer/memory_pressure_tests.cpp 
be6c3a118b528c39c534da423c15e9dcbb970dbc 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 44435: Fixed a bug that causes the task stuck in staging state.

[Shuai Lin]

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

(Updated March 19, 2016, 3:35 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Fixed a bug that causes the task stuck in staging state.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
4638d08328127a8c4ae37555f2be74fe9695da31 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
e849932ab558ccd792080433169ae50dc208a12e 

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


Testing
---

- Added a new test `"MesosContainerizerProvisionerTest.ProvisionFailed"`
- make check


Thanks,

Shuai Lin

Re: Review Request 44435: Fixed a bug that causes the task stuck in staging state.

[Jie Yu]

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




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


Thanks for catching the bug. I think this is a pretty bad bug which might 
worth cutting 0.28.1.

Your fix for this issue does reveal another issue in the code: It's 
possible that we will call provisioner->destroy while provisioner->provision is 
in progress. This could cause leaking of the provisioned directory. I'll create 
another ticket for that. Can you add a TODO here?

For this patch, can you add a comment here stating why you need to set it 
to be a ready future here? E.g.:
```
We need to set the 'launchInfos' to be a ready future initially before we 
starting calling isolator->prepare() because otherwise, the destroy will wait 
forever trying to wait for this future to be ready, which it never will.
```



src/tests/containerizer/mesos_containerizer_tests.cpp (line 776)


Can you use a UUID::random() here?



src/tests/containerizer/mesos_containerizer_tests.cpp (line 819)


This is not accurate. We need to improve that. Can you add a TODO here. I 
think we should introduce a new state PROVISIONING.


- Jie Yu


On March 6, 2016, 9:23 p.m., Shuai Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44435/
> ---
> 
> (Updated March 6, 2016, 9:23 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4878
> https://issues.apache.org/jira/browse/MESOS-4878
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a bug that causes the task stuck in staging state.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> af3ff5750649497d8852b4761c78d4cae5455a02 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 
> 
> Diff: https://reviews.apache.org/r/44435/diff/
> 
> 
> Testing
> ---
> 
> - Added a new test `"MesosContainerizerProvisionerTest.ProvisionFailed"`
> - make check
> 
> 
> Thanks,
> 
> Shuai Lin
> 
>

Re: Review Request 45033: Add a commit hook for checking non-ascii characters (MESOS-4033).

[Yong Tang]

> On March 18, 2016, 7:03 p.m., haosdent huang wrote:
> > docs/versioning.md, line 85
> > 
> >
> > Any reason we need change here?
> 
> Yong Tang wrote:
> Hi haosdent, the left is one unicode (U+2026) of "Horizontal Ellipsis" 
> (three dots). Inside the md file you will see it is 0xE2,0x80,0xA6 (UTF-8) if 
> you use binary mode in vi.  The right are three ascii characters of dot 
> (0xA0).

By the way, thanks for the review and the help. I will address the rest of the 
issues later.


- Yong


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


On March 18, 2016, 2:48 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45033/
> ---
> 
> (Updated March 18, 2016, 2:48 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-4033
> https://issues.apache.org/jira/browse/MESOS-4033
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This review request tries to add a commit hook for checking non-ascii
> characters. It scans .cpp, .hpp, .cc, .h, and .md files and report
> error if non-ascii characters exists.
> 
> As part of this review request, two non-ascii characters are identified
> in versioning.md (one in Ln 85 and another in Ln 96) and are corrected
> accordingly.
> 
> 
> Diffs
> -
> 
>   docs/versioning.md ecacd8433f0fa1643827b36d03154042538c1c6b 
>   support/hooks/pre-commit 10838a4c99db2a8318d64f95d90d2c2c90150453 
>   support/non-ascii.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45033/diff/
> 
> 
> Testing
> ---
> 
> Tested manually and found two non ascii characters in docs/versioning.md 
> (fixed as part of this review request).
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 44657: Used `KillPolicy` and shutdown grace period in command executor.

[Ben Mahler]

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


Fix it, then Ship it!





src/launcher/executor.cpp (line 924)


It doesn't crash, it just exits :)



src/launcher/executor.cpp (lines 925 - 926)


Or remove flags



src/slave/slave.cpp (line 3691)


Can you pull Seconds(1) down to the next line so that each component of the 
sum is on a separate line?


- Ben Mahler


On March 18, 2016, 5:19 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44657/
> ---
> 
> (Updated March 18, 2016, 5:19 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Gilbert Song.
> 
> 
> Bugs: MESOS-4909
> https://issues.apache.org/jira/browse/MESOS-4909
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The command executor determines how much time it allots the
> underlying task to clean up (effectively how long to wait for
> the task to comply to SIGTERM before sending SIGKILL) based
> on both optional task's `KillPolicy` and optional
> `shutdown_grace_period` field in `ExecutorInfo`.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 4149f084b0b234fb995df04e9d1ca704feec082e 
>   src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
> 
> Diff: https://reviews.apache.org/r/44657/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44709: Allowed unknown flags in command and docker executors.

[Jie Yu]

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




src/docker/executor.cpp (lines 588 - 590)


This is more like a question: are you talking about the case where the 
agent binary is updated, while the executor binary is not?


- Jie Yu


On March 18, 2016, 5:24 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44709/
> ---
> 
> (Updated March 18, 2016, 5:24 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We allow unknown flags to ensure that when a new flag is added
> in Mesos version N, agent of version N works with the executor
> of version N-1, i.e. the latter does not segfault when observing
> a newly introduced flag.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp afc769d0887e3842106e4c350e94c95c8ffc085e 
>   src/launcher/executor.cpp 4149f084b0b234fb995df04e9d1ca704feec082e 
> 
> Diff: https://reviews.apache.org/r/44709/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 45057: Made `unzip` overwrite existing files without prompting.

[Tomasz Janiszewski]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Made `unzip` overwrite existing files without prompting.


Diffs
-

  src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
  src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 

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


Testing
---


Thanks,

Tomasz Janiszewski

Re: Review Request 44993: Updated `TestContainerizer` to support default actions.

[Ben Mahler]

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



I don't have enough context on why you had to make this change, can you please 
provide context?

- Ben Mahler


On March 17, 2016, 11:47 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44993/
> ---
> 
> (Updated March 17, 2016, 11:47 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer.hpp 67fbe7fedbe170c3f22a2dcbb5aebf4195a5aabc 
>   src/tests/containerizer.cpp c6772ce5908edaab6c3189a65e8446217d1c7c27 
> 
> Diff: https://reviews.apache.org/r/44993/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44656: Introduced `KillPolicy` protobuf.

[Ben Mahler]

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


Fix it, then Ship it!





include/mesos/mesos.proto (lines 354 - 356)


How about removing the first setence in this NOTE and converting it to just 
a non-NOTE paragraph? You already say this on top of the kill_policy field?



include/mesos/mesos.proto (lines 366 - 370)


How about following similar language from the executor info change?

```
  // The grace period specifies how long to wait before forcibly
  // killing the task. It is recommended to attempt to gracefully
  // kill the task (and send TASK_KILLING) to indicate that the
  // graceful kill is in progress. Once the grace period elapses,
  // if the task has not terminated, a forcible kill should occur.
  // The task should not assume that it will *always* be alloted
  // the full grace period. For example, the executor may be
  // shutdown more quickly by the agent, or failures / forcible
  // terminations may occur.
```

However, you already have a NOTE above, so I'd rather not call it out again 
here.


- Ben Mahler


On March 18, 2016, 5:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44656/
> ---
> 
> (Updated March 18, 2016, 5:14 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4909
> https://issues.apache.org/jira/browse/MESOS-4909
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Describes a kill policy for a task. Currently does not express
> different policies (e.g. hitting HTTP endpoints). For now, this
> controls how long to wait between SIGTERM and SIGKILL.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b 
>   include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b 
> 
> Diff: https://reviews.apache.org/r/44656/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40988: stout: Fixed comments for numify(), cleaned up some test code.

[Neil Conway]

> On March 18, 2016, 11:13 p.m., Cong Wang wrote:
> > Can you separate the non-numify() change from the numify() one? It is best 
> > to do one thing per commit.
> 
> Neil Conway wrote:
> This change was committed a few months ago.
> 
> Cong Wang wrote:
> I know it, I noticed this one because of 45011. And my comment is for 
> Jie. I know that Mesos is still too young to care about bisect, but it is a 
> best practice to check in one thing within one commit, this patch/commit does 
> at least two things: one for numify(), one for the rest; it also hurts the 
> readablity of this patch a little.

Sure, I agree in general. I just mean that it is too late to separate the 
numify() from non-numify() changes at this point.


- Neil


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


On Dec. 4, 2015, 9:50 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40988/
> ---
> 
> (Updated Dec. 4, 2015, 9:50 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> stout: Fixed comments for numify(), cleaned up some test code.
> 
> Note that numify() handles negative numbers inconsistently, depending on 
> whether they are specified in hex or decimal (see test case). Can you guys 
> take a look at fixing?
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/numify.hpp 
> 322a89946befb0d7006124a08b415e2ef0a03e97 
>   3rdparty/libprocess/3rdparty/stout/tests/numify_tests.cpp 
> 522fc3470eb3496f7dfe1eeb814b171bcff21f38 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
> fc5a821bf3bb1416b67103d0ffd7ab5b88f45cca 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
> 7fa06a980b58040f68bf92d217c866f9e48a57d3 
>   3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 
> 7c0309c41ee5ad18bed30aa31a8361f11bca23a1 
> 
> Diff: https://reviews.apache.org/r/40988/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 44762: [WIP][PROPOSAL_1]Add CgroupsIsolator. This is only used for discussion.

[haosdent huang]

> On March 19, 2016, 12:39 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, line 363
> > 
> >
> > I think what you can do is to save a future in each Subsystem.
> > 
> > I am not sure if we have a primitive in libprocess to watch for if any 
> > future is ready or not. We can potentially add that. This function just 
> > watch for all futures in Subsystems and return that if any of them is 
> > ready. If any of them failed/discarded, return the failure.

>I think what you can do is to save a future in each Subsystem.
> This function just watch for all futures in Subsystems and return that if any 
> of them is ready. If any of them failed/discarded, return the failure.

Yes, we do this in [PROPOSAL_2](https://reviews.apache.org/r/44761/)

>I am not sure if we have a primitive in libprocess to watch for if any future 
>is ready or not.

I also think it would be better to add this in libprocess, so that we no need 
use `forloop` to register handlers on every future.


- haosdent


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


On March 13, 2016, 5:50 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44762/
> ---
> 
> (Updated March 13, 2016, 5:50 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4697
> https://issues.apache.org/jira/browse/MESOS-4697
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add CgroupsIsolator. This is only used for discussion.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0517dd1ae7125ac1ae85cc83b6daaca66a10b8b4 
>   src/Makefile.am f2a592dba3e963c99c48c4b9045b4a04d173cb22 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/info.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/info.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44762/diff/
> 
> 
> Testing
> ---
> 
> The relations between classes in this proposal is:
> 
> ```
> ++
> ||
> | CgroupsIsolatorProcess <-+
> || |
> +^---+ +
>  |   Belongs to
>  + +
> Belongs to |
>  + |
>  | |
>+-+-+   +---+-+
>|   |   | |
>| Subsystem <---+Used by+---+ CgroupsIsolatorInfo |
>|   |   | |
>+---+   +-+
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 44836: Update TaskId in long_lived_framework to use fixed length taskid.

[Klaus Ma]

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




src/examples/long_lived_framework.cpp (line 88)


I think we need check `format()`'s result; if `tasksLaunched` is bigger 
than `99`, what's the behaviour?


- Klaus Ma


On March 16, 2016, 10:27 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44836/
> ---
> 
> (Updated March 16, 2016, 10:27 a.m.)
> 
> 
> Review request for mesos and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Current WebUI sorts taskId lexically, which results in non-intuitive ordering.
> This patch updates long_lived_framework to use `01`, `02` instead.
> 
> 
> Diffs
> -
> 
>   src/examples/long_lived_framework.cpp 
> 289a0b9dd3d1ce30f20dd9bb381126bff30c 
> 
> Diff: https://reviews.apache.org/r/44836/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> In addition, `long_lived_framework` is ran and we confirm correct sorting of 
> `TaskID` in WebUI
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 40988: stout: Fixed comments for numify(), cleaned up some test code.

[Cong Wang]

> On March 18, 2016, 11:13 p.m., Cong Wang wrote:
> > Can you separate the non-numify() change from the numify() one? It is best 
> > to do one thing per commit.
> 
> Neil Conway wrote:
> This change was committed a few months ago.

I know it, I noticed this one because of 45011. And my comment is for Jie. I 
know that Mesos is still too young to care about bisect, but it is a best 
practice to check in one thing within one commit, this patch/commit does at 
least two things: one for numify(), one for the rest; it also hurts the 
readablity of this patch a little.


- Cong


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


On Dec. 4, 2015, 9:50 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40988/
> ---
> 
> (Updated Dec. 4, 2015, 9:50 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> stout: Fixed comments for numify(), cleaned up some test code.
> 
> Note that numify() handles negative numbers inconsistently, depending on 
> whether they are specified in hex or decimal (see test case). Can you guys 
> take a look at fixing?
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/numify.hpp 
> 322a89946befb0d7006124a08b415e2ef0a03e97 
>   3rdparty/libprocess/3rdparty/stout/tests/numify_tests.cpp 
> 522fc3470eb3496f7dfe1eeb814b171bcff21f38 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
> fc5a821bf3bb1416b67103d0ffd7ab5b88f45cca 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
> 7fa06a980b58040f68bf92d217c866f9e48a57d3 
>   3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 
> 7c0309c41ee5ad18bed30aa31a8361f11bca23a1 
> 
> Diff: https://reviews.apache.org/r/40988/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 45000: MESOS-3902: Fix in location header during redirect from non-leader.

[Vinod Kone]

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




src/master/http.cpp (line 958)


Looks like we specifically removed the scheme in 
https://reviews.apache.org/r/36018. @joris do you have context for why this was 
done?

From the RFC linked in the comment (which is really useful, lets not kil 
that comment :)) scheme doesn't seem to be mandatory. We can add path from 
request but AFAICT the spec doesn't mandate it. cc @benwhitehead.


- Vinod Kone


On March 18, 2016, 12:04 a.m., Ashwin Murthy wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45000/
> ---
> 
> (Updated March 18, 2016, 12:04 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3902: Fix in location header during redirect from non-leader. The fix 
> is to add the scheme and path to the URL in the location header. Previously, 
> we did not have the scheme and path. We need to update the mesos http API 
> documentation section on master redirect to reflect this change.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp b47ab7cc86c0a56a81815a98bd63f37a1175ba7f 
> 
> Diff: https://reviews.apache.org/r/45000/diff/
> 
> 
> Testing
> ---
> 
> Was trying to write unit test in scheduler http api test but it turns out 
> multi master tests cannot be written at this point. Need to manually verify 
> this by setting up multiple masters with ZK.
> 
> 
> Thanks,
> 
> Ashwin Murthy
> 
>

Re: Review Request 44707: Added validation for task's kill policy.

[Ben Mahler]

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


Ship it!




Please pull out the validation test into a simple unit test within 
master_validation_tests.cpp.

- Ben Mahler


On March 15, 2016, 3:47 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44707/
> ---
> 
> (Updated March 15, 2016, 3:47 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4909
> https://issues.apache.org/jira/browse/MESOS-4909
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
> 
> Diff: https://reviews.apache.org/r/44707/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44762: [WIP][PROPOSAL_1]Add CgroupsIsolator. This is only used for discussion.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 363)


I think what you can do is to save a future in each Subsystem.

I am not sure if we have a primitive in libprocess to watch for if any 
future is ready or not. We can potentially add that. This function just watch 
for all futures in Subsystems and return that if any of them is ready. If any 
of them failed/discarded, return the failure.


- Jie Yu


On March 13, 2016, 5:50 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44762/
> ---
> 
> (Updated March 13, 2016, 5:50 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4697
> https://issues.apache.org/jira/browse/MESOS-4697
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add CgroupsIsolator. This is only used for discussion.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0517dd1ae7125ac1ae85cc83b6daaca66a10b8b4 
>   src/Makefile.am f2a592dba3e963c99c48c4b9045b4a04d173cb22 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/info.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/info.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44762/diff/
> 
> 
> Testing
> ---
> 
> The relations between classes in this proposal is:
> 
> ```
> ++
> ||
> | CgroupsIsolatorProcess <-+
> || |
> +^---+ +
>  |   Belongs to
>  + +
> Belongs to |
>  + |
>  | |
>+-+-+   +---+-+
>|   |   | |
>| Subsystem <---+Used by+---+ CgroupsIsolatorInfo |
>|   |   | |
>+---+   +-+
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 44655: Made `shutdown_grace_period` configurable in `ExecutorInfo`.

[Ben Mahler]

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




include/mesos/mesos.proto (line 445)


Could we say `"must not assume that it will *always* be alloted the full 
grace period"` in all of these?


- Ben Mahler


On March 17, 2016, 11:33 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44655/
> ---
> 
> (Updated March 17, 2016, 11:33 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Gilbert Song.
> 
> 
> Bugs: MESOS-4949
> https://issues.apache.org/jira/browse/MESOS-4949
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If `ExecutorInfo.shutdown_grace_period` is set, the executor
> driver uses it, otherwise it falls back to the environment
> variable `MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD`.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md d10fa2e7fc7c477de2f0e30e10da6d817ecbf404 
>   include/mesos/executor/executor.proto 
> ae211194a44e0bf2fadc79e833881e45ea3eb2c2 
>   include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b 
>   include/mesos/v1/executor/executor.proto 
> 36a2b3f9bc3aaa524f655b9e686a6d33512e6aaa 
>   include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b 
>   src/slave/containerizer/containerizer.cpp 
> f6fc7863d0c215611f170dc0c89aa229407b5137 
>   src/slave/flags.cpp b77afa956834bb5b1f85301d7a5f386ab9da41e3 
>   src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
> 
> Diff: https://reviews.apache.org/r/44655/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44435: Fixed a bug that causes the task stuck in staging state.

[Jie Yu]

> On March 18, 2016, 7 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 674
> > 
> >
> > Thanks for catching the bug. I think this is a pretty bad bug which 
> > might worth cutting 0.28.1.
> > 
> > Your fix for this issue does reveal another issue in the code: It's 
> > possible that we will call provisioner->destroy while 
> > provisioner->provision is in progress. This could cause leaking of the 
> > provisioned directory. I'll create another ticket for that. Can you add a 
> > TODO here?
> > 
> > For this patch, can you add a comment here stating why you need to set 
> > it to be a ready future here? E.g.:
> > ```
> > We need to set the 'launchInfos' to be a ready future initially before 
> > we starting calling isolator->prepare() because otherwise, the destroy will 
> > wait forever trying to wait for this future to be ready, which it never 
> > will.
> > ```

Please include the ticket number in the TODO.


- Jie


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


On March 6, 2016, 9:23 p.m., Shuai Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44435/
> ---
> 
> (Updated March 6, 2016, 9:23 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4878
> https://issues.apache.org/jira/browse/MESOS-4878
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a bug that causes the task stuck in staging state.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> af3ff5750649497d8852b4761c78d4cae5455a02 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 
> 
> Diff: https://reviews.apache.org/r/44435/diff/
> 
> 
> Testing
> ---
> 
> - Added a new test `"MesosContainerizerProvisionerTest.ProvisionFailed"`
> - make check
> 
> 
> Thanks,
> 
> Shuai Lin
> 
>

Re: Review Request 44669: Added createFromModule methods to MasterContender and MasterDetector.

[Joseph Wu]

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




src/master/detectors/detector.cpp (line 126)


Extraneous space here.



src/master/detectors/detector.cpp (line 128)


Hmmm... on closer inspection, this might not work.  This constructor for 
the `StandaloneMasterDetector` needs to be manually `->appoint()`d later.  It 
will be better to return an `Error` here instead.


- Joseph Wu


On March 17, 2016, 5:29 p.m., Anurag Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44669/
> ---
> 
> (Updated March 17, 2016, 5:29 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-4610
> https://issues.apache.org/jira/browse/MESOS-4610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The createFromModule will be used to create a MasterContender/Detector
> from a module (specified using the --modules flag on the command
> line).
> 
> 
> Diffs
> -
> 
>   src/master/contenders/contender.cpp PRE-CREATION 
>   src/master/detectors/detector.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44669/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/44670/.
> 
> 
> Thanks,
> 
> Anurag Singh
> 
>

Re: Review Request 44994: Added a test for executor shutdown grace period.

[Ben Mahler]

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



Thanks for the test! Main high level comments are to pull out the validation 
test into the previous review that adds validation and put it alongside other 
validation tests (master_validation_tests.cpp). Second thing was to put an 
expectation on the dispatch to Slave::shutdownExecutorTimeout rather than on 
the test containerizer, which should eliminate the need for your previous 
change.


src/tests/slave_tests.cpp (line 3186)


No need for the NOTE IMO, but if you really think this needs calling out, 
the reader doesn't care about why the flag-less version of StartMaster isn't 
used; the reader cares perhaps about what the flags are used for. But as I read 
this I can quickly check what the flags are used for, and there are no suprises.

A NOTE is typically used to raise awareness of a potential suprise, or to a 
subtlety that may be missed, both of which I can't see myself experiencing 
here, which is good! :)

If we were to add a NOTE here you could imagine NOTEs all over the test 
(why do we use a mock executor? why do we use a test containerizer? why do we 
use agent flags? etc).



src/tests/slave_tests.cpp (lines 3198 - 3201)


Is there precedent for calling these kinds of variables "agent"? Let's keep 
it consistent:

```
$ grep -R 'Try slave' src | wc -l
 395
$ grep -R 'Try agent' src | wc -l
   0
```



src/tests/slave_tests.cpp (lines 3207 - 3210)


Use FutureArg and assert that when you use it later it is ready:

```
Future frameworkId;
EXPECT_CALL(sched, registered(, _, _))
  .WillOnce(FutureArg<1>());
```



src/tests/slave_tests.cpp (lines 3212 - 3251)


Can you pull this up into a small test inside 
`master_validation_tests.cpp`? We've been trying to contain all of the tests 
for master/validation.hpp in there.



src/tests/slave_tests.cpp (lines 3249 - 3250)


use the -> operator instead of .get().



src/tests/slave_tests.cpp (lines 3253 - 3298)


It seems unintuitive that you made this a scope separate from the one 
below, the scope here says that it should override the default from the agent 
flag, but that is not tested in this scope, it is tested below. Can you merge 
the scopes? Actually, once we pull out the invalid duration test, we won't need 
any nested scoping here?



src/tests/slave_tests.cpp (line 3264)


You don't need a settle here, AWAIT_READY will settle if the clock is 
paused.



src/tests/slave_tests.cpp (line 3267)


Can you do a sweep to replace .get(). with the -> operator in these patches?



src/tests/slave_tests.cpp (lines 3304 - 3307)


Thanks for this NOTE :)



src/tests/slave_tests.cpp (lines 3309 - 3312)


This was a bit confusing, I don't need to know about `_wait` here (and it's 
likely to become a stale comment) could you consolidate this into the NOTE 
above? I think we could be a bit more succinct here, for example:

```
// Note that the test containerizer only accepts "local" executors
// and it considers them "terminated" only once destroy is called.
```



src/tests/slave_tests.cpp (lines 3317 - 3323)


It seems fine to expect this but arguably if what we care about in this 
test is **how** the agent chooses the shutdown timeout, a FUTURE_DISPATCH on 
Slave::shutdownExecutorTimeout seems sufficient and closer reflects what is 
being tested? This avoids the need for the previous changes to the 
TestContainerizer AFAICT?



src/tests/slave_tests.cpp (line 3329)


Might as well expect this alongside the expectation of 'status', can you 
add a FutureSatisfy here?



src/tests/slave_tests.cpp (line 3331)


Would be nice to say that we have to spoof this because there isn't support 
in the driver for shutting down executors.



src/tests/slave_tests.cpp (lines 3337 - 3338)


Would be helpful to say that we ensure the message is received by the agent 
before we start the timer. Have you run this in repetition?



src/tests/slave_tests.cpp (lines 3341 - 3342)

Re: Review Request 44287: Added MasterContender and MasterDetector abstract classes.

[Anurag Singh]

> On March 18, 2016, 9:01 p.m., Joseph Wu wrote:
> > Sorry for the delay...
> > 
> > As Vinod suggested, you might want to consider reaching out to another 
> > shepherd (as far as I can tell, BenH is not likely to have time in the 
> > foreseeable future).  (And in case you don't know, I'm not a shepherd :)

Will do.


- Anurag


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


On March 18, 2016, 12:29 a.m., Anurag Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44287/
> ---
> 
> (Updated March 18, 2016, 12:29 a.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-4610
> https://issues.apache.org/jira/browse/MESOS-4610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MasterContender and MasterDetector abstract classes can be subclassed by
> user-provided classes to allow for arbitrary contender/detector 
> implementations.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/contender.hpp PRE-CREATION 
>   include/mesos/master/detector.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44287/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/44670/.
> 
> 
> Thanks,
> 
> Anurag Singh
> 
>

Re: Review Request 44854: Added validation for executor's shutdown grace period.

[Ben Mahler]

> On March 18, 2016, 9:33 p.m., Ben Mahler wrote:
> > The change looks good but please add a test for this.
> > 
> > Ideally there could be a stateless validateExecutorInfo, in addition to the 
> > one that needs the master state. In the absence of this, you can write a 
> > test that spins up a framework and launches a task with a negative shutdown 
> > duration.

I see now that you tested this in https://reviews.apache.org/r/44994/ inside a 
larger test, would have been helpful to mention that here since the testing 
done section points to https://reviews.apache.org/r/44662 which seems to be 
only a CHANGELOG change..?


- Ben


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


On March 17, 2016, 11:34 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44854/
> ---
> 
> (Updated March 17, 2016, 11:34 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4949
> https://issues.apache.org/jira/browse/MESOS-4949
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
> 
> Diff: https://reviews.apache.org/r/44854/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40988: stout: Fixed comments for numify(), cleaned up some test code.

[Neil Conway]

> On March 18, 2016, 11:13 p.m., Cong Wang wrote:
> > Can you separate the non-numify() change from the numify() one? It is best 
> > to do one thing per commit.

This change was committed a few months ago.


- Neil


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


On Dec. 4, 2015, 9:50 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40988/
> ---
> 
> (Updated Dec. 4, 2015, 9:50 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> stout: Fixed comments for numify(), cleaned up some test code.
> 
> Note that numify() handles negative numbers inconsistently, depending on 
> whether they are specified in hex or decimal (see test case). Can you guys 
> take a look at fixing?
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/numify.hpp 
> 322a89946befb0d7006124a08b415e2ef0a03e97 
>   3rdparty/libprocess/3rdparty/stout/tests/numify_tests.cpp 
> 522fc3470eb3496f7dfe1eeb814b171bcff21f38 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
> fc5a821bf3bb1416b67103d0ffd7ab5b88f45cca 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
> 7fa06a980b58040f68bf92d217c866f9e48a57d3 
>   3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 
> 7c0309c41ee5ad18bed30aa31a8361f11bca23a1 
> 
> Diff: https://reviews.apache.org/r/40988/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 45011: Fix numify() to handle negative numbers consistently (MESOS-4070).

[Neil Conway]

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


Ship it!




Ship It!

- Neil Conway


On March 18, 2016, 10:49 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45011/
> ---
> 
> (Updated March 18, 2016, 10:49 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Neil Conway, and Cong Wang.
> 
> 
> Bugs: MESOS-4070
> https://issues.apache.org/jira/browse/MESOS-4070
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix updated the implementation of numify() so that negative numbers
> could be handled conssitently for hex and non-hex numbers.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/numify.hpp 
> 26a637bec1193dd51437bd689c34fbe6d1935d89 
>   3rdparty/libprocess/3rdparty/stout/tests/numify_tests.cpp 
> 444377df00922df12d4b3ed25b4cfe9071cff5c3 
> 
> Diff: https://reviews.apache.org/r/45011/diff/
> 
> 
> Testing
> ---
> 
> make check (Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 45067: Updated the long-lived-framework example.

[Neil Conway]

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




src/examples/long_lived_framework.cpp (line 102)


Don't we need `TaskState_Name` here?


- Neil Conway


On March 19, 2016, 1:06 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45067/
> ---
> 
> (Updated March 19, 2016, 1:06 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Artem Harutyunyan, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This gives the example `long-lived-framework` enough options to run outside 
> of the build environment.
> 
> This also updates the style of the framework code and allows the 
> `ExecutorInfo` to be run with some cgroups isolators.
> 
> 
> Diffs
> -
> 
>   src/examples/long_lived_framework.cpp 
> 289a0b9dd3d1ce30f20dd9bb381126bff30c 
> 
> Diff: https://reviews.apache.org/r/45067/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Ran this on the master node on a Mesos cluster:
> ```
> ./long-lived-framework --master=zk://localhost:2181/mesos 
> --executor_uri="https://s3.amazonaws.com/url/to/long-lived-executor; 
> --executor_command="LD_LIBRARY_PATH=/path/to/libmesos && 
> ./long-lived-executor"
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 45067: Updated the long-lived-framework example.

[Joseph Wu]

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

Review request for mesos, Anand Mazumdar, Artem Harutyunyan, and Kevin Klues.


Repository: mesos


Description
---

This gives the example `long-lived-framework` enough options to run outside of 
the build environment.

This also updates the style of the framework code and allows the `ExecutorInfo` 
to be run with some cgroups isolators.


Diffs
-

  src/examples/long_lived_framework.cpp 
289a0b9dd3d1ce30f20dd9bb381126bff30c 

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


Testing
---

make check

Ran this on the master node on a Mesos cluster:
```
./long-lived-framework --master=zk://localhost:2181/mesos 
--executor_uri="https://s3.amazonaws.com/url/to/long-lived-executor; 
--executor_command="LD_LIBRARY_PATH=/path/to/libmesos && ./long-lived-executor"
```


Thanks,

Joseph Wu

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

[Jiang Yan Xu]

> On March 18, 2016, 11:36 a.m., Jiang Yan Xu wrote:
> > configure.ac, line 923
> > 
> >
> > Sorry this should have been a continued discussion on 
> > https://reviews.apache.org/r/44342/#comment184051 but anyways:
> > 
> > Your response to my initial comments:
> > > There's no AC_ARG_WITH. If the dependencies area available we will 
> > build the isolator and the operator can configure it at runtime. I don't 
> > think there's any need to disable this at build time since it is not 
> > enabled by default anyway.
> > 
> > I am still favoring building XFS isolator based on 
> > `--with-xfs-isolator` (AC_ARG_ENABLE is probably better than AC_ARG_WITH) 
> > for the following reasons:
> > 
> > 
> > 1. Explicitness: In general, if we build things solely based on the 
> > availability of headers, users can inadvertently build more things into the 
> > deployed binary which could result in larger binaries and perhaps other 
> > side effects (through bugs, etc.).
> > 2. Preventing user mistake: I am thinking about how we would advise 
> > users on enabling this feature:
> >   1. "The XFS isolator feature is disabled by default, to enable it in 
> > build, install these packages whose name could be different for different 
> > distros and if you haven't made mistakes in finding them, the feature will 
> > be built." vs. 
> >   2. "The XFS isolator feature is disabled by default, to enable it in 
> > build, install these packages (which could be named differently depending 
> > on your distro), if the dependencies are not satisfied, the build aborts 
> > with an message stating such error." The latter feels better to me.
> > 3. Consistency: Other optional features of Mesos follow this pattern.

Sorry I meant `--enable-xfs-isolator`.


- Jiang Yan


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


On March 17, 2016, 8:45 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> ---
> 
> (Updated March 17, 2016, 8:45 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add autoconf tests for XFS project quotas.
> 
> 
> Diffs
> -
> 
>   configure.ac 8e4f03593df4a8ba13f00292963e351acc3f71c1 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44421: Added support for "overlay" keyword.

[Guangya Liu]

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

(Updated 三月 18, 2016, 4:22 p.m.)


Review request for mesos, haosdent huang, Jie Yu, and Shuai Lin.


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


Repository: mesos


Description
---

The "overlayfs" was renamed to "overlay" in kernel 4.2, for overlay
support check function, it should check both "overlay" and "overlayfs"
in "/proc/filesystems".


Diffs (updated)
-

  src/linux/fs.hpp 4525a5d5566e2bc913894e993ac7350f1bbd9cc0 
  src/linux/fs.cpp dbf94759b6382bfafd3f3b8b4c2047af36a53ad5 
  src/slave/containerizer/mesos/provisioner/backend.cpp 
55652540e35f9c451ad85cfead575a788aa3eba1 
  src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
5cc0f8b5a8cd4c945023f874056a8184113186c5 
  src/tests/environment.cpp ee1bbe6b4e3dda1e27b63d71a08ef0d1d254741a 

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


Testing
---

root@mesos002:~/src/mesos/m1/mesos/build# uname -r
4.2.3-040203-generic
root@mesos002:~/src/mesos/m1/mesos/build# lsmod  | grep over
overlay45056  1 
root@mesos002:~/src/mesos/m1/mesos/build# cat /proc/filesystems | grep over
nodev   overlay
root@mesos002:~/src/

make
make check
./bin/mesos-tests.sh 
--gtest_filter="OverlayBackendTest.ROOT_OVERLAYFS_OverlayFSBackend" --verbose


Thanks,

Guangya Liu

Re: Review Request 44660: Used `KillPolicy` and shutdown grace period in docker executor.

[Ben Mahler]

> On March 15, 2016, 11:21 p.m., Ben Mahler wrote:
> > src/docker/executor.cpp, lines 657-663
> > 
> >
> > Hm.. ideally we could ignore the docker stop flag if a kill policy was 
> > set, because the user is being explicit about how much time they need. This 
> > is the same as how we ignore the executor shutdown grace period flag if the 
> > executor explicitly sets one.
> 
> Alexander Rukletsov wrote:
> Yes, that would be ideal. However, we'll learn about kill policy only 
> later, when the task is launched. The case when the user sets both the docker 
> stop timeout and the killpolicy, and the kill policy is smaller than the 
> docker flag seems to be rare to me. Moreover, in such case we give *not less* 
> time than requested. I'd suggest to leave this as is as opposed to saving the 
> docker flag value and deciding later whether we need it or not.

Sounds good.


- Ben


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


On March 18, 2016, 5:21 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44660/
> ---
> 
> (Updated March 18, 2016, 5:21 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.
> 
> 
> Bugs: MESOS-4909
> https://issues.apache.org/jira/browse/MESOS-4909
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The docker executor determines how much time it allots the
> underlying container to clean up (via passing the timeout to
> the docker daemon) based on both optional task's `KillPolicy`
> and optional `shutdown_grace_period` field in `ExecutorInfo`.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b 
>   include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b 
>   src/docker/executor.cpp afc769d0887e3842106e4c350e94c95c8ffc085e 
> 
> Diff: https://reviews.apache.org/r/44660/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40988: stout: Fixed comments for numify(), cleaned up some test code.

[Cong Wang]

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



Can you separate the non-numify() change from the numify() one? It is best to 
do one thing per commit.

- Cong Wang


On Dec. 4, 2015, 9:50 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40988/
> ---
> 
> (Updated Dec. 4, 2015, 9:50 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> stout: Fixed comments for numify(), cleaned up some test code.
> 
> Note that numify() handles negative numbers inconsistently, depending on 
> whether they are specified in hex or decimal (see test case). Can you guys 
> take a look at fixing?
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/numify.hpp 
> 322a89946befb0d7006124a08b415e2ef0a03e97 
>   3rdparty/libprocess/3rdparty/stout/tests/numify_tests.cpp 
> 522fc3470eb3496f7dfe1eeb814b171bcff21f38 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
> fc5a821bf3bb1416b67103d0ffd7ab5b88f45cca 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
> 7fa06a980b58040f68bf92d217c866f9e48a57d3 
>   3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 
> 7c0309c41ee5ad18bed30aa31a8361f11bca23a1 
> 
> Diff: https://reviews.apache.org/r/40988/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 45004: Fixed a memory leak in the scheduler driver.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

We were not freeing up the heap allocated `Credentials` object.


Diffs
-

  src/sched/sched.cpp 525255eec808c3fe5c0e38b3d1a2086bbd4eb171 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 37168: Add an example framework using dynamic reservation.

[Klaus Ma]

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

(Updated March 17, 2016, 2:15 p.m.)


Review request for mesos, Greg Mann, Joerg Schad, and Michael Park.


Changes
---

Remove "numify.hpp"


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


Repository: mesos


Description
---

Provide example for dynamic reservation features.


Diffs (updated)
-

  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/examples/dynamic_reservation_framework.cpp PRE-CREATION 
  src/tests/dynamic_reservation_framework_test.sh PRE-CREATION 
  src/tests/examples_tests.cpp 6ddac17bb2ac0330bcc09eaab975ae70e84a7695 

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


Testing
---

make
make check


Thanks,

Klaus Ma

Re: Review Request 43615: Update test suite to use the reworked MesosTest helpers.

[Michael Park]

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


Ship it!




On my machine, full test-suite remains to take about 1m 30s roughly.

- Michael Park


On March 16, 2016, 12:21 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43615/
> ---
> 
> (Updated March 16, 2016, 12:21 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Includes the following changes:
> 
> * Added the `` header where appropriate.
> * Added the namespace `using process::Owned;` where appropriate.
> * Generally replaced `Try` with `Try`.  
> And `Try` with `Try`.
> * Added the (now required) `MasterDetector` argument to all slaves.  Before, 
> this was fetched from the first master in `Cluster`.
> * Removed `Shutdown();` from all tests.
> * Replaced `Stop(...)` with the appropriate master/slave destruction calls.
> * Wrap various slave objects in `Owned` (i.e. containerizers, isolators, 
> launchers, etc).
> * Replace `CHECK` in tests with `ASSERT`.
> 
> 
> Diffs
> -
> 
>   src/tests/authentication_tests.cpp 85f14c3d453ca5aeffa1c915f38fe3031c2cf712 
>   src/tests/command_executor_tests.cpp 
> 0d2fcf6d4b8d9a925eb6748e6bd33cf279b8f7f8 
>   src/tests/container_logger_tests.cpp 
> 00f4129e46aa9268fbb66da25b34e61004fa87b2 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 8afaa4dab3984e9866b7b223e8e2e70ef83a39dc 
>   src/tests/containerizer/external_containerizer_test.cpp 
> 8e1dbe306a088eb16cd3b9c6174b95fad5685da4 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> e72239a55724f1aeeec5362cc370c93dbeca7164 
>   src/tests/containerizer/isolator_tests.cpp 
> 342037ce0a5f8caa4e3cf1550b8f9a7cc328acf9 
>   src/tests/containerizer/memory_pressure_tests.cpp 
> 03879d99c371f296f8d9904666911b34209c114d 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 
>   src/tests/containerizer/port_mapping_tests.cpp 
> a1427fd0157dee343b643f3272dba8ffea61f7b0 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> b2663a83afccbb156f10b4703f29ee0f638384bf 
>   src/tests/containerizer/runtime_isolator_tests.cpp 
> 1625614f560a4c9a644b1b9e9568199f30373ab5 
>   src/tests/credentials_tests.cpp 7edcc857e0f6f8e80e265deeec59d6349d392224 
>   src/tests/disk_quota_tests.cpp 413e562026a4fc9779f616e921ae2fa2ca51e012 
>   src/tests/exception_tests.cpp 6b71316d545e97f14a45daa14d0fd95204befd3b 
>   src/tests/executor_http_api_tests.cpp 
> 2fc0893f5f5e80a783296fb31b30abe86d92df1b 
>   src/tests/fault_tolerance_tests.cpp 
> 67f362747ac4301a74693aeaa5631b7dd866e782 
>   src/tests/gc_tests.cpp 61a8abb9581dc4602b197a88a677b19386969cbf 
>   src/tests/health_check_tests.cpp 9605859b665645654bbdb2688455cae2d692a057 
>   src/tests/hook_tests.cpp bb287c77493209e663a37e7f88d09a0459855f7d 
>   src/tests/http_fault_tolerance_tests.cpp 
> 7c7f3d90210148176e83553346100a506f263591 
>   src/tests/master_allocator_tests.cpp 
> cba7c36471f93b678d94e1da0251a28a893696b1 
>   src/tests/master_authorization_tests.cpp 
> 6a4de4709960d7ca505e99396e14a1bb51d6902d 
>   src/tests/master_contender_detector_tests.cpp 
> 255ab8119a04b55bb4f1b61dee19c4be64499376 
>   src/tests/master_quota_tests.cpp f1277f03f5b5cf735fea7ba324a27614a829cd50 
>   src/tests/master_slave_reconciliation_tests.cpp 
> d41178eb41df519073fc0890c5716bbc9fed6ad2 
>   src/tests/master_tests.cpp e8c39e775d6ca218ce74cfc6bb50c7576d73e90e 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
>   src/tests/metrics_tests.cpp 419d275e0b32817388120222bd433ee6f4835efd 
>   src/tests/monitor_tests.cpp 869c9e032817e8859a968232d4a61556a3d53d45 
>   src/tests/oversubscription_tests.cpp 
> e528476cd83b0e3f7ae8cea7d86dfabc1f66484e 
>   src/tests/partition_tests.cpp 3776a0a104582f60b9f19ea58b011485194399b9 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> b5f425ab7f2b4a540418d761bea145565467bd2b 
>   src/tests/persistent_volume_tests.cpp 
> e9215de2e073025f67cdc73e8a8de38cf030671f 
>   src/tests/rate_limiting_tests.cpp 352a5e7a58b500c25c7c8a421047c4e79434e38e 
>   src/tests/reconciliation_tests.cpp e8f3f29836652d20a6ee1bb5231a15e71eb76990 
>   src/tests/registrar_zookeeper_tests.cpp 
> 3df9779ee5d076e16f6a538326693a36f986b6d0 
>   src/tests/repair_tests.cpp 0dfb557a62f64baf2334dbc9f75ecdfb10473c2d 
>   src/tests/reservation_endpoints_tests.cpp 
> f95ae7a32c3809d150adf1e9e515a3b527e61699 
>   

Re: Review Request 44553: Added authentication to agent HTTP endpoints.

[Greg Mann]

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

(Updated March 18, 2016, 7:16 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Added a comment.


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


Repository: mesos


Description
---

Added authentication to agent HTTP endpoints.

This patch adds HTTP authentication to the `/state`, `/state.json`, and 
`/flags` endpoints. Tests are also updated to use authentication when hitting 
these endpoints, and a new test was added to probe these endpoints' behavior 
when bad credentials are supplied: `SlaveTest.HTTPEndpointsBadAuthentication`.


Diffs (updated)
-

  src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 
  src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
  src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
  src/tests/containerizer/docker_containerizer_tests.cpp 
f6fce7df82417e029fadf805d6e0b793f396aa69 
  src/tests/fault_tolerance_tests.cpp f99413f56e96a796d3d45decad1f049e6a238789 
  src/tests/health_check_tests.cpp d32164aeb1eb439bd062afa28614dd919e24f06b 
  src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169 

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


Testing
---

Tests were updated to use authentication when hitting the affected agent 
endpoints, and new tests were added to probe the endpoint behavior when invalid 
credentials are supplied.

`sudo make check` was used to test on both OSX and Ubuntu 14.04. The new test 
was run 1000 times to look for flakiness.


Thanks,

Greg Mann

Re: Review Request 45022: Windows: Add Windows-friendly implementation of `rm.hpp`.

[Alex Clemmer]

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

(Updated March 18, 2016, 7:39 a.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


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


Repository: mesos


Description
---

We currently depend on arcane "POSIX-like" Windows APIs like `::remove`.
This API is incompatible with NT paths -- which we will eventually have
to transition to for the Windows integration -- and has no documented
behavior interacting with subsystems like the driver subsystem, which is
required for symlinks.

This commit will move us to proper core Windows API equivalents for
`::remove`.

Review: https://reviews.apache.org/r/45022


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
7bd4bfbc2ec5922879dcefddc12137336b11be52 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rm.hpp PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/rm.hpp 
52568b303c03fd57b81f6cc67782444ce734dd41 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rm.hpp 
PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp 
4c30189bb8261ccfc699da0f31b8b1fd3e9b3c83 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 44553: Added authentication to agent HTTP endpoints.

[Joerg Schad]

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




src/slave/http.cpp (line 54)


FYI: Noticed this incorrect blank line while reviewing, removed it with 
https://reviews.apache.org/r/45034/.


- Joerg Schad


On March 17, 2016, 10:56 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44553/
> ---
> 
> (Updated March 17, 2016, 10:56 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4850
> https://issues.apache.org/jira/browse/MESOS-4850
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authentication to agent HTTP endpoints.
> 
> This patch adds HTTP authentication to the `/state`, `/state.json`, and 
> `/flags` endpoints. Tests are also updated to use authentication when hitting 
> these endpoints, and a new test was added to probe these endpoints' behavior 
> when bad credentials are supplied: `SlaveTest.HTTPEndpointsBadAuthentication`.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 
>   src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
>   src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> f6fce7df82417e029fadf805d6e0b793f396aa69 
>   src/tests/fault_tolerance_tests.cpp 
> f99413f56e96a796d3d45decad1f049e6a238789 
>   src/tests/health_check_tests.cpp d32164aeb1eb439bd062afa28614dd919e24f06b 
>   src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169 
> 
> Diff: https://reviews.apache.org/r/44553/diff/
> 
> 
> Testing
> ---
> 
> Tests were updated to use authentication when hitting the affected agent 
> endpoints, and new tests were added to probe the endpoint behavior when 
> invalid credentials are supplied.
> 
> `sudo make check` was used to test on both OSX and Ubuntu 14.04. The new test 
> was run 1000 times to look for flakiness.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 44944: Handled chunked responses in docker URI fetcher.

[Timothy Chen]

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


Ship it!




Ship It!

- Timothy Chen


On March 17, 2016, 1:50 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44944/
> ---
> 
> (Updated March 17, 2016, 1:50 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jojy Varghese, James Peach, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-4964
> https://issues.apache.org/jira/browse/MESOS-4964
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Handled chunked responses in docker URI fetcher.
> 
> This is because if the response is chunked, curl will output the dechunked 
> version. That will confuse the http response decoder because the header shows 
> that it is chunked.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/docker.cpp 2ceee199c6b1bf92c5eca80c8d3344e97e394a09 
> 
> Diff: https://reviews.apache.org/r/44944/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 44908: Update `MockDockerContainerizer` construction.

[haosdent huang]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Update `MockDockerContainerizer` construction.


Diffs
-

  src/tests/mesos.hpp 908a53a146208a6d41c0bda5208415e39e2eae05 
  src/tests/mesos.cpp 7cca4ed4753fa365b209d1d22f0df4650b19bc6a 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 44824: Remove `SlaveState` in `ComposingContainerizer` during recover.

[haosdent huang]

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

(Updated March 16, 2016, 11:26 a.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Rebase.


Repository: mesos


Description
---

Remove `SlaveState` in `ComposingContainerizer` during recover.


Diffs (updated)
-

  src/slave/containerizer/composing.hpp 
f3eebd19bc9e6b3b8a969a2ad967b3e2909e0ee4 
  src/slave/containerizer/composing.cpp 
15d059f0bbda4e8cb93c65c09327dde1e34d3e7b 
  src/tests/containerizer/composing_containerizer_tests.cpp 
e7e3b622b6606a812aef046c761bf92368d34af2 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 44555: Implemented the framework and create() method of "network/cni" isolator.

[Qian Zhang]

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

(Updated March 17, 2016, 5:47 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Implemented the framework and create() method of "network/cni" isolator.


Diffs (updated)
-

  src/CMakeLists.txt 0517dd1ae7125ac1ae85cc83b6daaca66a10b8b4 
  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 45039: Updated the scheduler `launchTasks()` comment.

[Ben Mahler]

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




include/mesos/scheduler.hpp (lines 219 - 221)


This change needs to propagate to the Java and Python side, in addition to 
what Joerg mentioned, grepping will show you:

```
$ grep -R "description of Filters" src include
src/java/src/org/apache/mesos/SchedulerDriver.java:   * unused resources 
(see mesos.proto for a description of Filters).
src/java/src/org/apache/mesos/SchedulerDriver.java:   * resources (see 
mesos.proto for a description of Filters).
src/python/interface/src/mesos/interface/__init__.py:  mesos.proto for 
a description of Filters). Available resources are
src/python/interface/src/mesos/interface/__init__.py:  unused resources 
(see mesos.proto for a description of Filters).
include/mesos/scheduler.hpp:  // resources (see mesos.proto for a 
description of Filters).
include/mesos/scheduler.hpp:  // resources (see mesos.proto for a 
description of Filters).
```


- Ben Mahler


On March 18, 2016, 5:13 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45039/
> ---
> 
> (Updated March 18, 2016, 5:13 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary,
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler.hpp 14c7ff964aa7b94f439d16e605380661d2279d54 
> 
> Diff: https://reviews.apache.org/r/45039/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44762: [WIP][PROPOSAL_1]Add CgroupsIsolator. This is only used for discussion.

[haosdent huang]

> On March 19, 2016, 12:30 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/info.hpp, line 75
> > 
> >
> > Why? Can that be vector?

If we make this as a vector and pass it to `MemSubystem`, `MemSubsytem` need 
find the `CgroupsMemInfo` in the vector and cast it from `SystemInfo`. Because 
different type `SystemInfo` have different internal members and could not share 
as same interfaces.


- haosdent


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


On March 13, 2016, 5:50 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44762/
> ---
> 
> (Updated March 13, 2016, 5:50 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4697
> https://issues.apache.org/jira/browse/MESOS-4697
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add CgroupsIsolator. This is only used for discussion.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0517dd1ae7125ac1ae85cc83b6daaca66a10b8b4 
>   src/Makefile.am f2a592dba3e963c99c48c4b9045b4a04d173cb22 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/info.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/info.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44762/diff/
> 
> 
> Testing
> ---
> 
> The relations between classes in this proposal is:
> 
> ```
> ++
> ||
> | CgroupsIsolatorProcess <-+
> || |
> +^---+ +
>  |   Belongs to
>  + +
> Belongs to |
>  + |
>  | |
>+-+-+   +---+-+
>|   |   | |
>| Subsystem <---+Used by+---+ CgroupsIsolatorInfo |
>|   |   | |
>+---+   +-+
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 44289: Added support for contender and detector modules.

[Anurag Singh]

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

(Updated March 18, 2016, 12:29 a.m.)


Review request for mesos and Benjamin Hindman.


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


Repository: mesos


Description
---

Added support for contender and detector modules.


Diffs (updated)
-

  include/mesos/module/contender.hpp PRE-CREATION 
  include/mesos/module/detector.hpp PRE-CREATION 
  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/examples/test_contender_module.cpp PRE-CREATION 
  src/examples/test_detector_module.cpp PRE-CREATION 
  src/local/local.cpp f8599e7378e9a0065bbd01ad8f23f11debb30c91 
  src/master/contenders/zookeeper.cpp 9ad49ce10439fb41d78d52eaa4c1e6b9c5c7f735 
  src/master/detectors/zookeeper.cpp PRE-CREATION 
  src/module/manager.cpp 8c9aaf7cd00c904daba9994a99df9e1329831c01 
  src/tests/module.hpp 4b32f29f2ce76100433621a5cb6b8cc87c9b38f8 
  src/tests/module.cpp 8cc305c0ef606b07eea39d548d3165a2bb2b042a 

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


Testing
---

See https://reviews.apache.org/r/44670/.


Thanks,

Anurag Singh

Re: Review Request 44661: Deprecated the `docker_stop_timeout` flag.

[Ben Mahler]

> On March 19, 2016, 2:16 a.m., Ben Mahler wrote:
> > Ok I misunderstood what the intent was here. Generally when we say 
> > "deprecated" we maintain the old behavior and we plan to remove the support 
> > in a future version. This doesn't maintain the old behavior, why not leave 
> > as is and we do a clean break once the deprecation cycle is finished?

Also, can you include a CHANGELOG update in this patch? It's much cleaner on 
master and easier for posterity if the deprecation is coupled with the user 
documentation (i.e. what they should do instead of using this flag).


- Ben


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


On March 18, 2016, 5:23 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44661/
> ---
> 
> (Updated March 18, 2016, 5:23 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Bugs: MESOS-4910
> https://issues.apache.org/jira/browse/MESOS-4910
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead, a combination of `executor_shutdown_grace_period`
> agent flag and task kill policies should be used.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md d10fa2e7fc7c477de2f0e30e10da6d817ecbf404 
>   src/docker/executor.hpp abbc419533ab40312e917931a2fc2ce78b38da41 
>   src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp b77afa956834bb5b1f85301d7a5f386ab9da41e3 
> 
> Diff: https://reviews.apache.org/r/44661/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42684: Multiple Disk: Added persistent volumes tests for `MOUNT` type.

[Joris Van Remoortere]

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

(Updated March 18, 2016, 10:46 p.m.)


Review request for mesos, Jie Yu and Michael Park.


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


Repository: mesos


Description
---

Multiple Disk: Added persistent volumes tests for `MOUNT` type.


Diffs (updated)
-

  src/tests/persistent_volume_tests.cpp 
26fff19daa8b175fdcc06fd9467224d5920a1967 
  src/tests/resources_tests.cpp 6b004d64bb25112b19fc5d98b5bca874c5329e8c 

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


Testing
---

sudo make check


Thanks,

Joris Van Remoortere

Re: Review Request 44707: Added validation for task's kill policy.

[Ben Mahler]

> On March 15, 2016, 8:35 p.m., Ben Mahler wrote:
> > Could you add a corresponding test? Having it in the header should make 
> > this really easy :)
> 
> Alexander Rukletsov wrote:
> Sure, but I'll write a more complex test than just unit testing the 
> validation function.

Per the suggestion from the ExecutorInfo change, how about we pull up the 
simple unit test for this within master_validation_tests.cpp to be consistent?


- Ben


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


On March 15, 2016, 3:47 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44707/
> ---
> 
> (Updated March 15, 2016, 3:47 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4909
> https://issues.apache.org/jira/browse/MESOS-4909
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
> 
> Diff: https://reviews.apache.org/r/44707/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 45070: Clean up libprocess gtest macros (MESOS-4112).

[Yong Tang]

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

Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

This review request tries to clean up gtest macros in libprocess.
The following items have been done:
1. AWAIT_EQ_FOR have be added for completeness.
2. Missing equivalents of EXPECT_TRUE/EXPECT_FALSE have been added:
a) AWAIT_ASSERT_TRUE_FOR
b) AWAIT_ASSERT_TRUE
c) AWAIT_ASSERT_FALSE_FOR
d) AWAIT_ASSERT_FALSE
e) AWAIT_EXPECT_TRUE_FOR
f) AWAIT_EXPECT_FALSE_FOR


Diffs
-

  3rdparty/libprocess/include/process/gtest.hpp 
049152c0535c78e6986346610735d301fb6876bc 

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


Testing
---

make check (Ubuntu 14.04)


Thanks,

Yong Tang

Re: Review Request 44435: Fixed a bug that causes the task stuck in staging state.

[Jie Yu]

> On March 18, 2016, 7 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 674
> > 
> >
> > Thanks for catching the bug. I think this is a pretty bad bug which 
> > might worth cutting 0.28.1.
> > 
> > Your fix for this issue does reveal another issue in the code: It's 
> > possible that we will call provisioner->destroy while 
> > provisioner->provision is in progress. This could cause leaking of the 
> > provisioned directory. I'll create another ticket for that. Can you add a 
> > TODO here?
> > 
> > For this patch, can you add a comment here stating why you need to set 
> > it to be a ready future here? E.g.:
> > ```
> > We need to set the 'launchInfos' to be a ready future initially before 
> > we starting calling isolator->prepare() because otherwise, the destroy will 
> > wait forever trying to wait for this future to be ready, which it never 
> > will.
> > ```
> 
> Jie Yu wrote:
> Please include the ticket number in the TODO.

BTW, I create a ticket for the other bug:
https://issues.apache.org/jira/browse/MESOS-4985


- Jie


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


On March 6, 2016, 9:23 p.m., Shuai Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44435/
> ---
> 
> (Updated March 6, 2016, 9:23 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4878
> https://issues.apache.org/jira/browse/MESOS-4878
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a bug that causes the task stuck in staging state.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> af3ff5750649497d8852b4761c78d4cae5455a02 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 
> 
> Diff: https://reviews.apache.org/r/44435/diff/
> 
> 
> Testing
> ---
> 
> - Added a new test `"MesosContainerizerProvisionerTest.ProvisionFailed"`
> - make check
> 
> 
> Thanks,
> 
> Shuai Lin
> 
>

Re: Review Request 44854: Added validation for executor's shutdown grace period.

[Ben Mahler]

> On March 18, 2016, 9:33 p.m., Ben Mahler wrote:
> > The change looks good but please add a test for this.
> > 
> > Ideally there could be a stateless validateExecutorInfo, in addition to the 
> > one that needs the master state. In the absence of this, you can write a 
> > test that spins up a framework and launches a task with a negative shutdown 
> > duration.
> 
> Ben Mahler wrote:
> I see now that you tested this in https://reviews.apache.org/r/44994/ 
> inside a larger test, would have been helpful to mention that here since the 
> testing done section points to https://reviews.apache.org/r/44662 which seems 
> to be only a CHANGELOG change..?

I suggested on https://reviews.apache.org/r/44994/ that we pull out a smaller 
test for the validation and place it inside `master_validation_tests.cpp` to be 
more consistent.


- Ben


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


On March 17, 2016, 11:34 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44854/
> ---
> 
> (Updated March 17, 2016, 11:34 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4949
> https://issues.apache.org/jira/browse/MESOS-4949
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
> 
> Diff: https://reviews.apache.org/r/44854/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44762: [WIP][PROPOSAL_1]Add CgroupsIsolator. This is only used for discussion.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/cgroups/info.hpp (line 75)


Why? Can that be vector?


- Jie Yu


On March 13, 2016, 5:50 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44762/
> ---
> 
> (Updated March 13, 2016, 5:50 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4697
> https://issues.apache.org/jira/browse/MESOS-4697
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add CgroupsIsolator. This is only used for discussion.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0517dd1ae7125ac1ae85cc83b6daaca66a10b8b4 
>   src/Makefile.am f2a592dba3e963c99c48c4b9045b4a04d173cb22 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/info.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/info.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44762/diff/
> 
> 
> Testing
> ---
> 
> The relations between classes in this proposal is:
> 
> ```
> ++
> ||
> | CgroupsIsolatorProcess <-+
> || |
> +^---+ +
>  |   Belongs to
>  + +
> Belongs to |
>  + |
>  | |
>+-+-+   +---+-+
>|   |   | |
>| Subsystem <---+Used by+---+ CgroupsIsolatorInfo |
>|   |   | |
>+---+   +-+
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 44435: Fixed a bug that causes the task stuck in staging state.

[Guangya Liu]

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




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


s/'launchInfos'/`launchInfos`



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


s/because otherwise,/because otherwise, if the `prepare` in isolator did 
not return `launchInfos` in some failure cases (MESOS-4878), ?



src/tests/containerizer/mesos_containerizer_tests.cpp (lines 713 - 719)


The jagged format is not good and a patch 
https://reviews.apache.org/r/44653/diff/5#index_header is already trying to fix 
such issues.

It is better use following format:
MOCK_METHOD2(
recover,
Future(
const list&,
const hashset&));



src/tests/containerizer/mesos_containerizer_tests.cpp (line 740)


a period


- Guangya Liu


On 三月 19, 2016, 2:29 a.m., Shuai Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44435/
> ---
> 
> (Updated 三月 19, 2016, 2:29 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4878
> https://issues.apache.org/jira/browse/MESOS-4878
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a bug that causes the task stuck in staging state.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 4638d08328127a8c4ae37555f2be74fe9695da31 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> e849932ab558ccd792080433169ae50dc208a12e 
> 
> Diff: https://reviews.apache.org/r/44435/diff/
> 
> 
> Testing
> ---
> 
> - Added a new test `"MesosContainerizerProvisionerTest.ProvisionFailed"`
> - make check
> 
> 
> Thanks,
> 
> Shuai Lin
> 
>

Re: Review Request 44288: Changed MasterDetector/Contender namespace.

[Anurag Singh]

> On March 18, 2016, 9:03 p.m., Joseph Wu wrote:
> > src/master/contender.cpp, line 91
> > 
> >
> > Another spacing change seems to have snuck in.

Apologies ... I forgot to add expandtab in my vimrc. Fixing it.


- Anurag


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


On March 18, 2016, 12:29 a.m., Anurag Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44288/
> ---
> 
> (Updated March 18, 2016, 12:29 a.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-4610
> https://issues.apache.org/jira/browse/MESOS-4610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also modified users of MasterContender and MasterDetector to use this
> namespace.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler.hpp 14c7ff964aa7b94f439d16e605380661d2279d54 
>   include/mesos/v1/scheduler.hpp 765935e97b6c1686ab464a5cf1cf2dfd816f51f1 
>   src/cli/resolve.cpp 257e29034abf32491511f9a4e476b6859714829d 
>   src/local/local.cpp f8599e7378e9a0065bbd01ad8f23f11debb30c91 
>   src/master/contender.cpp 9ad49ce10439fb41d78d52eaa4c1e6b9c5c7f735 
>   src/master/detector.cpp 9274435802d6292b183be48f42b43999476e016e 
>   src/master/main.cpp 61210d9f275d4073967c3468179307cf09e88551 
>   src/master/master.hpp 124d43931a5c8a00ee0aaa604feb1761795209f2 
>   src/master/master.cpp e6290ea686ccf17813d6faeaf2f2012f79cf3b7f 
>   src/sched/sched.cpp 525255eec808c3fe5c0e38b3d1a2086bbd4eb171 
>   src/scheduler/scheduler.cpp 6a834473ef35540eedac7e211b5204ab5f4eb7b2 
>   src/slave/main.cpp 33a1af84aeb079224b15e92caf97bcf081ea4646 
>   src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
>   src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
>   src/tests/authentication_tests.cpp 8143cd7a22bbdbcd0fc613cb44eae8b55fd458e7 
>   src/tests/cluster.hpp 06424dd741aed2261a926429bb0fc7dea141c11b 
>   src/tests/cluster.cpp 22167da70a855a39fd9c3ca980304372c70bd8d3 
>   src/tests/command_executor_tests.cpp 
> 970cdc39f4f2b0377d36acf2465d377d2a6e1d05 
>   src/tests/container_logger_tests.cpp 
> 71101c31cee6a400b89cf285cf0a105d2d1534a8 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> f6fce7df82417e029fadf805d6e0b793f396aa69 
>   src/tests/containerizer/external_containerizer_test.cpp 
> 5e2116355418f5a0716cfd1573bab48ba75df596 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> b3bd5a03266150a0cd83c966d646a32c419bf512 
>   src/tests/containerizer/isolator_tests.cpp 
> 6a2e25b967742c034364d19372f06aa9f9cdf828 
>   src/tests/containerizer/memory_pressure_tests.cpp 
> be6c3a118b528c39c534da423c15e9dcbb970dbc 
>   src/tests/containerizer/port_mapping_tests.cpp 
> de4b6f99f3a994bcedafa801eed9c4a7b79bac23 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 72d4c3e8756e1bea2332db20654af0a5fbb124f1 
>   src/tests/containerizer/runtime_isolator_tests.cpp 
> 9f3b0b08da7cebba722062a9932fae1b5f825efb 
>   src/tests/credentials_tests.cpp b61ba2ea5df8957f12659de219f6a57cf30d987a 
>   src/tests/disk_quota_tests.cpp 7f5e32f3239db3adf6e4cec2df15ccf89b4f13f4 
>   src/tests/exception_tests.cpp a50ccf1255dee59fdbc6fb1539bd1f6429458fb4 
>   src/tests/executor_http_api_tests.cpp 
> ff7b672e03185fca8b408b8805223a314fa3e483 
>   src/tests/fault_tolerance_tests.cpp 
> f99413f56e96a796d3d45decad1f049e6a238789 
>   src/tests/fetcher_cache_tests.cpp 645dae208cb2b0aa2d2181d96eb1fd8893975430 
>   src/tests/gc_tests.cpp 42059b2d6544f360cdc9230fe6ed33a11a15bc50 
>   src/tests/health_check_tests.cpp d32164aeb1eb439bd062afa28614dd919e24f06b 
>   src/tests/hook_tests.cpp 595991deab38c34e918601e85d250dc995d0f34c 
>   src/tests/http_fault_tolerance_tests.cpp 
> c06e07daf6d6519c10489310cd4275ae94f302c6 
>   src/tests/master_allocator_tests.cpp 
> b41ba2bda4d680f6fc42f525719973d56c11fe31 
>   src/tests/master_authorization_tests.cpp 
> 8b9b8991fbb8c5a5beb69416a9c4a4ef3525942d 
>   src/tests/master_contender_detector_tests.cpp 
> bbce379e5a0a0ca608579d0ab2b10970e9cd5ef1 
>   src/tests/master_maintenance_tests.cpp 
> b42a81fc2e0982e8fca669bffb798c0acda684fc 
>   src/tests/master_quota_tests.cpp c2b46d23002481e63ff162e8628f9b974e3e8ef9 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 988f1d46580ab5a707fe801824e24f94d4f50da7 
>   src/tests/master_tests.cpp d34ba0bdd71efd261850d8c205c16cecb701ac7c 
>   src/tests/master_validation_tests.cpp 
> 8d0070a1b8e8dcc7fe6360f8c6c6182ba9edef7d 
>   src/tests/mesos.hpp 93b9340d94d91663283fe5df5ad9febe69ffd2a3 
>   src/tests/mesos.cpp 90aef6bfe619dc0acdb4ccba6a7180482dd13ce5 
>   src/tests/metrics_tests.cpp