Re: Review Request 45357: Add HTTP response related macros (AWAIT_ASSERT_RESPONSE_...).

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45356, 45357]

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 26, 2016, 10:56 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45357/
> ---
> 
> (Updated March 26, 2016, 10:56 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-4112
> https://issues.apache.org/jira/browse/MESOS-4112
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This RR is the item (3) for MESOS-4112. It adds HTTP respose
> related macros that were not defined before:
> 1. AWAIT_ASSERT_RESPONSE_STATUS_EQ_FOR
> 2. AWAIT_ASSERT_RESPONSE_STATUS_EQ
> 3. AWAIT_ASSERT_RESPONSE_BODY_EQ_FOR
> 4. AWAIT_ASSERT_RESPONSE_BODY_EQ
> 5. AWAIT_ASSERT_RESPONSE_HEADER_EQ_FOR
> 6. AWAIT_ASSERT_RESPONSE_HEADER_EQ
> (The above are the counterpart of AWAIT_EXPECT_RESPONSE_...)
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gtest.hpp 
> 30d51d3704bd0ad82c6d21f1222d6158aaa61298 
> 
> Diff: https://reviews.apache.org/r/45357/diff/
> 
> 
> Testing
> ---
> 
> make check (Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 45354: Migrate `UserCgroupIsolatorTest` to cgroups unified isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45083, 45084, 45350, 45085, 45086, 45087, 45351, 45352, 
45353, 45354]

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 26, 2016, 7:35 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45354/
> ---
> 
> (Updated March 26, 2016, 7:35 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5043
> https://issues.apache.org/jira/browse/MESOS-5043
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Migrate `UserCgroupIsolatorTest` to cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> c4e467c8227f9e4129b05d173812592f39a04e06 
> 
> Diff: https://reviews.apache.org/r/45354/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Review Request 45357: Add HTTP response related macros (AWAIT_ASSERT_RESPONSE_...).

[Yong Tang]

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

Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

This RR is the item (3) for MESOS-4112. It adds HTTP respose
related macros that were not defined before:
1. AWAIT_ASSERT_RESPONSE_STATUS_EQ_FOR
2. AWAIT_ASSERT_RESPONSE_STATUS_EQ
3. AWAIT_ASSERT_RESPONSE_BODY_EQ_FOR
4. AWAIT_ASSERT_RESPONSE_BODY_EQ
5. AWAIT_ASSERT_RESPONSE_HEADER_EQ_FOR
6. AWAIT_ASSERT_RESPONSE_HEADER_EQ
(The above are the counterpart of AWAIT_EXPECT_RESPONSE_...)


Diffs
-

  3rdparty/libprocess/include/process/gtest.hpp 
30d51d3704bd0ad82c6d21f1222d6158aaa61298 

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


Testing
---

make check (Ubuntu 14.04)


Thanks,

Yong Tang

Review Request 45356: Replace `AWAIT_ASSERT_EQ(true/false` with `AWAIT_ASSERT_TRUE/_FALSE`.

[Yong Tang]

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

Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

This RR is a follow up to MESOS-4112 to replace `AWAIT_ASSERT_EQ(true/false`
with the newly defined `AWAIT_ASSERT_TRUE/_FALSE`.


Diffs
-

  src/tests/cram_md5_authentication_tests.cpp 
67f74e208ce138dc1e4667af108af3538b34aee9 
  src/tests/gc_tests.cpp 42059b2d6544f360cdc9230fe6ed33a11a15bc50 
  src/tests/group_tests.cpp ed10f1fe8a5cd2a353175e13ec3c464da69956f4 
  src/tests/registrar_tests.cpp 39caf9bb950c0b229a66becb039c7a830a18f6bc 

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


Testing
---

make check (Ubuntu 14.04)


Thanks,

Yong Tang

Re: Review Request 44843: Replace NULL with nullptr.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [44843]

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

Error:
2016-03-26 21:02:02 URL:https://reviews.apache.org/r/44843/diff/raw/ 
[212202/212202] -> "44843.patch" [1]
error: patch failed: src/slave/slave.cpp:651
error: src/slave/slave.cpp: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/12159/console

- Mesos ReviewBot


On March 15, 2016, 9:15 a.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44843/
> ---
> 
> (Updated March 15, 2016, 9:15 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3243
> https://issues.apache.org/jira/browse/MESOS-3243
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replace NULLs with `run-clang-tidy.py` and then with sed
> 
> 
> Diffs
> -
> 
>   src/authentication/cram_md5/authenticatee.cpp 
> 7d4994333428e5d8aa9ce6b85e0a7f42a0ddc4c8 
>   src/authentication/cram_md5/authenticator.cpp 
> 027eba4f433fff328d68c41005bc59c41c7ae668 
>   src/authentication/cram_md5/auxprop.cpp 
> d82d2d2f7793859772d89cc91bee09240624c613 
>   src/authorizer/local/authorizer.cpp 
> 0f0d9276337858984f0b19a82ffca74ee84dc650 
>   src/examples/balloon_executor.cpp 399218478a596387814d123290eeeb33102fad7a 
>   src/examples/example_module_impl.cpp 
> aeffdc0e95fa7badc8cfa09e2b57462e427fec55 
>   src/examples/load_generator_framework.cpp 
> 7d64b6617564f43ef383ee60d92da92b2c958c47 
>   src/examples/test_allocator_module.cpp 
> 1255a4a22a9d1a5724bef78dfc4dee498ed50fff 
>   src/examples/test_anonymous_module.cpp 
> dd291cff3b5d47337e371cd2c1082fd6716af3fc 
>   src/examples/test_authorizer_module.cpp 
> 19ec7cd114562f74c660b83b39235127d25001ee 
>   src/examples/test_container_logger_module.cpp 
> 76dd494fa4c32514ba14b1f4498b588ac9051b4d 
>   src/examples/test_hook_module.cpp abd132b3f39265683542a9d1533d2a31bd81769a 
>   src/examples/test_http_authenticator_module.cpp 
> 459b7046bd76d3043d2484a2dd30c10d7deaedd4 
>   src/examples/test_isolator_module.cpp 
> a4a2103b1e449837b95948c8b5c25e05c5d13860 
>   src/examples/test_qos_controller_module.cpp 
> f382fc443fa0a4545ba78dc28016e4d0a0dd16b1 
>   src/examples/test_resource_estimator_module.cpp 
> 229b2931e9129c194850f04051060d33bfa06570 
>   src/exec/exec.cpp 741786132f3a8cc43f5b9ced262429038832a946 
>   src/java/jni/construct.cpp 0bfe6291e94a69a787c965fa3a3a90d2ebae8d72 
>   src/java/jni/convert.cpp f1a486d8d2f6d43efd89407c928734bd9715e591 
>   src/java/jni/org_apache_mesos_Log.cpp 
> 140b950136417eed7cba363a89537ed2f33832ff 
>   src/java/jni/org_apache_mesos_MesosExecutorDriver.cpp 
> 9a92ade5d54bd814353acb40170d7aa4d8fe4a77 
>   src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp 
> c723e2d7bce98e6ac5ab587ce8c42fa9ba8b47e4 
>   src/java/jni/org_apache_mesos_state_AbstractState.cpp 
> 4fd43ca31c6917e81ea1b331b8507ca42a2249cf 
>   src/java/jni/org_apache_mesos_state_Variable.cpp 
> 7a69b3d878e6eaa87bfaf56c0500e11efe4ac44d 
>   src/java/jni/org_apache_mesos_state_ZooKeeperState.cpp 
> 23a89c3e5c5309a8a9b45c168fb61b12d89db1ec 
>   src/jvm/jvm.cpp 909d34a0112b219456dce76126029c603077e66d 
>   src/launcher/executor.cpp 4149f084b0b234fb995df04e9d1ca704feec082e 
>   src/linux/cgroups.cpp df18ed46a2a96871f67c7eb4233c3b4c27b7aa1c 
>   src/linux/fs.cpp dbf94759b6382bfafd3f3b8b4c2047af36a53ad5 
>   src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 
>   src/linux/routing/diagnosis/diagnosis.cpp 
> 8b86a8864c08b078425dcc242d323763d6ec15dd 
>   src/linux/routing/route.cpp 4b33350dc3761e19121c29c0d090cfdae544e4ef 
>   src/linux/systemd.cpp e120d2ceffe6bb400859f2a132f02a99f561f856 
>   src/local/local.cpp f8599e7378e9a0065bbd01ad8f23f11debb30c91 
>   src/log/leveldb.cpp ba2f62bc97e4a0eaa8a54824551686467db4b9e4 
>   src/log/log.cpp a37676068dae14b1adc61ef75e2742c16e7a6e42 
>   src/log/tool/benchmark.cpp 770c6d85fec76826ca8369b2afc721c07899e32f 
>   src/log/tool/initialize.cpp bd1e9ef1922ae972a5999b6e7412e08eac92c1ac 
>   src/log/tool/read.cpp b9e90e44c8cd7351767e523af338d8c662e0848c 
>   src/log/tool/replica.cpp 49415821a32960c78192b89f9a0f2067b9157a63 
>   src/logging/logging.cpp 8d9e4e9b200a0df1c67d4e7cd57107b7780f9812 
>   src/master/contender.cpp 9ad49ce10439fb41d78d52eaa4c1e6b9c5c7f735 
>   src/master/http.cpp 1c2ca334567611578d9c6edb9ee7b0f43f0f1e18 
>   src/master/main.cpp 7c1656bcc266f6c94cb4befad37fa813a218b2fa 
>   src/master/master.cpp d0380db3b90a9166607445f8dd50cc63d547228e 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
>   src/module/manager.cpp 8c9aaf7cd00c904daba9994a99df9e1329831c01 
>   

Re: Review Request 45311: Added an additional template parameter to 'Try' for typeful error.

[Daniel Pravat]

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




3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp (line 33)


With this default parameter the code has to be changed on every place where 
we return or handle a WindowsError object. 

One sugestion is to use different defaults on Windows and Posix and handle 
the conversion from ErrnoError/Error to WindowsError inside WindowsError class.


- Daniel Pravat


On March 24, 2016, 8:23 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45311/
> ---
> 
> (Updated March 24, 2016, 8:23 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp 
> c444c0118d39ee6a5da4618d7c62784464377280 
> 
> Diff: https://reviews.apache.org/r/45311/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 45312: Captured the error code in `ErrnoError` and `WindowsError`.

[Daniel Pravat]

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


Ship it!




Ship It!

- Daniel Pravat


On March 24, 2016, 8:23 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45312/
> ---
> 
> (Updated March 24, 2016, 8:23 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/errorbase.hpp 
> 1e9db7e1cd2f411efb94893ba2c29bd0694e4ddc 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/error.hpp 
> 64102e1f31437d4271a1126e339fb2f33f0181b8 
> 
> Diff: https://reviews.apache.org/r/45312/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 45314: Updated `network::connect` to use the typeful `Try` error state.

[Daniel Pravat]

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




3rdparty/libprocess/include/process/network.hpp (line 75)


We tried to avoid axecuting too much code between ::connect 
WSAGetLastError() calls. 
Constructing the parameter for WindowsSocketError may overwrite the last 
error. It should be better to pass in the return from WSAGetLastError() as a 
parameter to the constructor, that is evaluate before the std::string parameter.


- Daniel Pravat


On March 24, 2016, 8:23 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45314/
> ---
> 
> (Updated March 24, 2016, 8:23 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/network.hpp 
> 9976257d2d13316062bc95a22ab564ca0df165e5 
>   3rdparty/libprocess/src/poll_socket.cpp 
> 6e6634b4b352e3723096521843546cf56ec6dd8b 
> 
> Diff: https://reviews.apache.org/r/45314/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 44950: Add XFS disk isolator documentation.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44945, 44946, 44947, 44948, 44949, 44950]

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 26, 2016, 5:06 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44950/
> ---
> 
> (Updated March 26, 2016, 5:06 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 XFS disk isolator documentation.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 9ad0c2a201130ac23b22b1a844396dc97ec5ff70 
>   docs/mesos-containerizer.md 15fb5bdbe74e059614b8948108f32cd04b623305 
> 
> Diff: https://reviews.apache.org/r/44950/diff/
> 
> 
> Testing
> ---
> 
> Make check. Source inspection.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 45310: Removed unnecessary constructors in `Try`.

[Daniel Pravat]

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




3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp (line 56)


Without additional context the reviewer may think that this change will 
break the build. Maybe you can change the order between this review and the 
next or eliminate the constructors in the next commit.


- Daniel Pravat


On March 24, 2016, 8:22 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45310/
> ---
> 
> (Updated March 24, 2016, 8:22 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Correctly constrained the templated constructor.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp 
> c444c0118d39ee6a5da4618d7c62784464377280 
> 
> Diff: https://reviews.apache.org/r/45310/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 45354: Migrate `UserCgroupIsolatorTest` to cgroups unified isolator.

[haosdent huang]

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

(Updated March 26, 2016, 7:35 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebase


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


Repository: mesos


Description
---

Migrate `UserCgroupIsolatorTest` to cgroups unified isolator.


Diffs (updated)
-

  src/tests/containerizer/cgroups_isolator_tests.cpp 
c4e467c8227f9e4129b05d173812592f39a04e06 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 45350: Add `--cgroups_subsystems` in agent flags.

[haosdent huang]

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

(Updated March 26, 2016, 7:33 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebase


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


Repository: mesos


Description
---

Add `--cgroups_subsystems` in agent flags.


Diffs (updated)
-

  docs/configuration.md 9ad0c2a201130ac23b22b1a844396dc97ec5ff70 
  src/slave/flags.hpp 9ee7f349cc2bb3fa76141645f4a06fad57664367 
  src/slave/flags.cpp fd9fbbaa911cc77a21574ba314c50ac226fa49ce 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 43935: Allow setting role in mesos-execute.

[Michael Park]

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




src/cli/execute.cpp (line 129)


Other example frameworks seem to use `"Role to use when registering"`. Can 
we keep it consistent?


- Michael Park


On March 25, 2016, 1:54 a.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43935/
> ---
> 
> (Updated March 25, 2016, 1:54 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Shuai Lin, and Michael Park.
> 
> 
> Bugs: MESOS-4744
> https://issues.apache.org/jira/browse/MESOS-4744
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow setting role in mesos-execute.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e 
> 
> Diff: https://reviews.apache.org/r/43935/diff/
> 
> 
> Testing
> ---
> 
> make & make check
> 
> start master.
> ./bin/mesos-master.sh --work_dir=/tmp/mesos
> 
> start slave.
> ./bin/mesos-slave.sh --work_dir=/tmp/mesos --master=192.168.99.1:5050 
> --resources="cpus:1;cpus(test):1;mem:7500;mem(test):7500"
> 
> running mesos-execute without specifying role succeeds.
> ./src/mesos-execute --master=192.168.99.1:5050 --name="test" --command="sleep 
> 10" --resources="cpus:1;mem:512"
> 
> running mesos-execute with role test succeeds.
> ./src/mesos-execute --master=192.168.99.1:5050 --name="test" --command="sleep 
> 10" --role="test" --resources="cpus:2;mem:512"
> 
> running mesos-execute with role test1 fails.
> ./src/mesos-execute --master=192.168.99.1:5050 --name="test" --command="sleep 
> 10" --role="test1" --resources="cpus:2;mem:512"
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 45083: Added a any mechanism for futures.

[haosdent huang]

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

(Updated March 26, 2016, 7:32 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Fix test error


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


Repository: mesos


Description
---

Added a any mechanism for futures.


Diffs (updated)
-

  3rdparty/libprocess/include/process/collect.hpp 
5a92b72eb7668494dc832ec446a41b3d673a20cc 
  3rdparty/libprocess/src/tests/collect_tests.cpp 
155e0bb75cf723a0a6c29020f9f767e3ba3d7401 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 45313: Introduced `WindowsSocketError` and factored out `WindowsErrorBase`.

[Daniel Pravat]

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




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


Same name for member variable and input parameter. I think is confusing.


- Daniel Pravat


On March 24, 2016, 8:23 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45313/
> ---
> 
> (Updated March 24, 2016, 8:23 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/error.hpp 
> 64102e1f31437d4271a1126e339fb2f33f0181b8 
> 
> Diff: https://reviews.apache.org/r/45313/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 44843: Replace NULL with nullptr.

[Michael Park]

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




src/exec/exec.cpp (lines 637 - 638)


This is clearly beyond just a `s/NULL/nullptr`. I assume this is 
`clang-tidy` magic, right?



src/linux/fs.cpp (lines 587 - 589)


```
  Try mount =
fs::mount(None(), "/", None(), MS_REC | MS_SLAVE, nullptr);

  if (mount.isError()) {
...
  }
```



src/python/executor/src/mesos/executor/proxy_executor.cpp (lines 54 - 55)


Hm, how come these `NULL`s were not replaced?



src/python/scheduler/src/mesos/scheduler/proxy_scheduler.cpp (line 335)


What about the `NULL` here?



src/tests/script.cpp (line 96)


Another instance of `NULL` not being replaced?



src/tests/zookeeper_test_server.cpp (line 115)


Another instance of `NULL` not replaced.


- Michael Park


On March 15, 2016, 9:15 a.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44843/
> ---
> 
> (Updated March 15, 2016, 9:15 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3243
> https://issues.apache.org/jira/browse/MESOS-3243
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replace NULLs with `run-clang-tidy.py` and then with sed
> 
> 
> Diffs
> -
> 
>   src/authentication/cram_md5/authenticatee.cpp 
> 7d4994333428e5d8aa9ce6b85e0a7f42a0ddc4c8 
>   src/authentication/cram_md5/authenticator.cpp 
> 027eba4f433fff328d68c41005bc59c41c7ae668 
>   src/authentication/cram_md5/auxprop.cpp 
> d82d2d2f7793859772d89cc91bee09240624c613 
>   src/authorizer/local/authorizer.cpp 
> 0f0d9276337858984f0b19a82ffca74ee84dc650 
>   src/examples/balloon_executor.cpp 399218478a596387814d123290eeeb33102fad7a 
>   src/examples/example_module_impl.cpp 
> aeffdc0e95fa7badc8cfa09e2b57462e427fec55 
>   src/examples/load_generator_framework.cpp 
> 7d64b6617564f43ef383ee60d92da92b2c958c47 
>   src/examples/test_allocator_module.cpp 
> 1255a4a22a9d1a5724bef78dfc4dee498ed50fff 
>   src/examples/test_anonymous_module.cpp 
> dd291cff3b5d47337e371cd2c1082fd6716af3fc 
>   src/examples/test_authorizer_module.cpp 
> 19ec7cd114562f74c660b83b39235127d25001ee 
>   src/examples/test_container_logger_module.cpp 
> 76dd494fa4c32514ba14b1f4498b588ac9051b4d 
>   src/examples/test_hook_module.cpp abd132b3f39265683542a9d1533d2a31bd81769a 
>   src/examples/test_http_authenticator_module.cpp 
> 459b7046bd76d3043d2484a2dd30c10d7deaedd4 
>   src/examples/test_isolator_module.cpp 
> a4a2103b1e449837b95948c8b5c25e05c5d13860 
>   src/examples/test_qos_controller_module.cpp 
> f382fc443fa0a4545ba78dc28016e4d0a0dd16b1 
>   src/examples/test_resource_estimator_module.cpp 
> 229b2931e9129c194850f04051060d33bfa06570 
>   src/exec/exec.cpp 741786132f3a8cc43f5b9ced262429038832a946 
>   src/java/jni/construct.cpp 0bfe6291e94a69a787c965fa3a3a90d2ebae8d72 
>   src/java/jni/convert.cpp f1a486d8d2f6d43efd89407c928734bd9715e591 
>   src/java/jni/org_apache_mesos_Log.cpp 
> 140b950136417eed7cba363a89537ed2f33832ff 
>   src/java/jni/org_apache_mesos_MesosExecutorDriver.cpp 
> 9a92ade5d54bd814353acb40170d7aa4d8fe4a77 
>   src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp 
> c723e2d7bce98e6ac5ab587ce8c42fa9ba8b47e4 
>   src/java/jni/org_apache_mesos_state_AbstractState.cpp 
> 4fd43ca31c6917e81ea1b331b8507ca42a2249cf 
>   src/java/jni/org_apache_mesos_state_Variable.cpp 
> 7a69b3d878e6eaa87bfaf56c0500e11efe4ac44d 
>   src/java/jni/org_apache_mesos_state_ZooKeeperState.cpp 
> 23a89c3e5c5309a8a9b45c168fb61b12d89db1ec 
>   src/jvm/jvm.cpp 909d34a0112b219456dce76126029c603077e66d 
>   src/launcher/executor.cpp 4149f084b0b234fb995df04e9d1ca704feec082e 
>   src/linux/cgroups.cpp df18ed46a2a96871f67c7eb4233c3b4c27b7aa1c 
>   src/linux/fs.cpp dbf94759b6382bfafd3f3b8b4c2047af36a53ad5 
>   src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 
>   src/linux/routing/diagnosis/diagnosis.cpp 
> 8b86a8864c08b078425dcc242d323763d6ec15dd 
>   src/linux/routing/route.cpp 4b33350dc3761e19121c29c0d090cfdae544e4ef 
>   src/linux/systemd.cpp e120d2ceffe6bb400859f2a132f02a99f561f856 
>   src/local/local.cpp f8599e7378e9a0065bbd01ad8f23f11debb30c91 
>   src/log/leveldb.cpp ba2f62bc97e4a0eaa8a54824551686467db4b9e4 
>   src/log/log.cpp a37676068dae14b1adc61ef75e2742c16e7a6e42 
>   src/log/tool/benchmark.cpp 770c6d85fec76826ca8369b2afc721c07899e32f 
>   

Re: Review Request 45313: Introduced `WindowsSocketError` and factored out `WindowsErrorBase`.

[Daniel Pravat]

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




3rdparty/libprocess/3rdparty/stout/include/stout/windows/error.hpp (line 30)


The constructor should be WindowsErrorBase



3rdparty/libprocess/3rdparty/stout/include/stout/windows/error.hpp (line 33)


Constructor name == WindowsErrorBase



3rdparty/libprocess/3rdparty/stout/include/stout/windows/error.hpp (line 105)


Make the constructor public



3rdparty/libprocess/3rdparty/stout/include/stout/windows/error.hpp (line 113)


Make the constructor public



3rdparty/libprocess/3rdparty/stout/include/stout/windows/error.hpp (line 115)


This shoulf be WindowsSocketError


- Daniel Pravat


On March 24, 2016, 8:23 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45313/
> ---
> 
> (Updated March 24, 2016, 8:23 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/error.hpp 
> 64102e1f31437d4271a1126e339fb2f33f0181b8 
> 
> Diff: https://reviews.apache.org/r/45313/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 45082: Implemented cleanup() method of "network/cni" isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44004, 44200, 44549, 44555, 44622, 44514, 44706, 45082]

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 26, 2016, 2:59 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45082/
> ---
> 
> (Updated March 26, 2016, 2:59 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 cleanup() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> b1b7205f4f10b6dc256fcc4ecb3210105c1240b4 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 7cda5715814a0cfc4b394eb04437831e6dc44e3f 
> 
> Diff: https://reviews.apache.org/r/45082/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On March 19, 2016, 2:09 a.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45070/
> ---
> 
> (Updated March 19, 2016, 2:09 a.m.)
> 
> 
> 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 44948: Add XFS disk resource isolator.

[James Peach]

> On March 24, 2016, 4:54 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/disk/xfs.cpp, lines 156-157
> > 
> >
> > How does `openat` facilitate DFS better?

If we used ``openat(2)`` we would not need to construct the full path name.


- James


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


On March 22, 2016, 11:24 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44948/
> ---
> 
> (Updated March 22, 2016, 11:24 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
> ---
> 
> Track sandbox directory usage by dynamically assigning XFS project
> quotas. We track a range of XFS project IDs, assigning a project ID
> and a project quota to each sandbox as it is created. When the task
> reaches the quota, writes will fail with EDQUOT, and the task will have
> an opportunity to handle that.
> 
> Quotas are not applied to volume resources since the isolator interface
> has no insight into the volume lifecycle. Thus it is not currently
> possible to accurately assign and reclaim project IDs.
> 
> If LOW is the lower bound of the project ID range and HIGH is the upper
> bound, you can show the currently allocated project quotas using the
> xfs_quota command:
> 
>   $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"
> 
> To show the project ID assigned to the file PATH, use the xfs_io command:
> 
>   $ xfs_io -r -c stat PATH
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/containerizer.cpp 
> ee7a265975323ca891114a286357c8e42901560c 
>   src/slave/containerizer/mesos/isolators/disk/xfs.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/disk/xfs.cpp PRE-CREATION 
>   src/slave/flags.hpp 3067c128f1fded4f130f4d45f191584c2f30ad9c 
>   src/slave/flags.cpp ce028825ad99f54a231b4b18dde277b63aa0525c 
> 
> Diff: https://reviews.apache.org/r/44948/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44948: Add XFS disk resource isolator.

[James Peach]

> On March 24, 2016, 4:54 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 217
> > 
> >
> > IMO "xfs/disk" is probably better than "disk/xfs" (compared with 
> > "posix/disk") and "xfs/quota" is even better ("posix/quota" would have been 
> > more confusing but xfs being a filesystem, "xfs/quota" seem pretty clear to 
> > me what it means)

I agree that "xfs/disk" would be a reasonable choice.


- James


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


On March 22, 2016, 11:24 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44948/
> ---
> 
> (Updated March 22, 2016, 11:24 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
> ---
> 
> Track sandbox directory usage by dynamically assigning XFS project
> quotas. We track a range of XFS project IDs, assigning a project ID
> and a project quota to each sandbox as it is created. When the task
> reaches the quota, writes will fail with EDQUOT, and the task will have
> an opportunity to handle that.
> 
> Quotas are not applied to volume resources since the isolator interface
> has no insight into the volume lifecycle. Thus it is not currently
> possible to accurately assign and reclaim project IDs.
> 
> If LOW is the lower bound of the project ID range and HIGH is the upper
> bound, you can show the currently allocated project quotas using the
> xfs_quota command:
> 
>   $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"
> 
> To show the project ID assigned to the file PATH, use the xfs_io command:
> 
>   $ xfs_io -r -c stat PATH
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/containerizer.cpp 
> ee7a265975323ca891114a286357c8e42901560c 
>   src/slave/containerizer/mesos/isolators/disk/xfs.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/disk/xfs.cpp PRE-CREATION 
>   src/slave/flags.hpp 3067c128f1fded4f130f4d45f191584c2f30ad9c 
>   src/slave/flags.cpp ce028825ad99f54a231b4b18dde277b63aa0525c 
> 
> Diff: https://reviews.apache.org/r/44948/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated March 26, 2016, 5:06 p.m.)


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


Changes
---

Rebased and addressed review comments.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
  src/slave/containerizer/mesos/isolators/disk/xfs/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp PRE-CREATION 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44949: Add XFS disk isolator tests.

[James Peach]

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

(Updated March 26, 2016, 5:06 p.m.)


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


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add basic XFS disk isolator tests by cloning the POSIX disk isolator
tests and making minor changes for the differences in semantics.


Diffs (updated)
-

  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 

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


Testing
---

Make check. Manual testing.


Thanks,

James Peach

Re: Review Request 44950: Add XFS disk isolator documentation.

[James Peach]

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

(Updated March 26, 2016, 5:06 p.m.)


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


Changes
---

Rebased.


Bugs: MESOs-4828
https://issues.apache.org/jira/browse/MESOs-4828


Repository: mesos


Description
---

Add XFS disk isolator documentation.


Diffs (updated)
-

  docs/configuration.md 9ad0c2a201130ac23b22b1a844396dc97ec5ff70 
  docs/mesos-containerizer.md 15fb5bdbe74e059614b8948108f32cd04b623305 

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


Testing
---

Make check. Source inspection.


Thanks,

James Peach

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

[James Peach]

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

(Updated March 26, 2016, 5:05 p.m.)


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


Changes
---

Rebased and addressed review comments.


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


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  configure.ac 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 

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


Testing
---

Make check. Manual verification.


Thanks,

James Peach

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated March 26, 2016, 5:05 p.m.)


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


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp 7617e43587cb81104786d06f753f08565a6c2d0a 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

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

[James Peach]

> On March 26, 2016, 6:43 a.m., Jiang Yan Xu wrote:
> > configure.ac, lines 957-960
> > 
> >
> > Can we put AC_MSG_CHECKING before `AC_CHECK_HEADERS` and inside `AS_IF` 
> > above?
> > 
> > i.e., AC_MSG_CHECKING/AC_MSG_CHECKING/AC_MSG_ERROR inform user what 
> > they don't already know (whether the system dependencies are met), not what 
> > they already know (the fact that they provided the 
> > `--enable_xfs_disk_isolator` flag).
> > 
> > And this check is conditional: we only print `AC_MSG_CHECKING` and do 
> > these checks when `test "x$enable_xfs_disk_isolator" = "xyes"`; we print 
> > `AC_MSG_ERROR([...])` when such checks fail and `AC_MSG_RESULT([yes])` when 
> > all checks succeed.
> > 
> > Would this work?
> > 
> > If we do this, then the message could be `AC_MSG_CHECKING([whether we 
> > can enable the XFS disk isolator])`

No, this message should be outside the check so that the builder always knows 
what happened, independent of what configure flags they passed. Typically, 
people start by not passing any flags, and it is helpful to explicitly log that 
a feature was not enabled. Even if you pass the --enable flags, it is 
comforting to get an explicit confirmation that the feature you asked for is 
enabled.


- James


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


On March 26, 2016, 5:05 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> ---
> 
> (Updated March 26, 2016, 5:05 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 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 45085: Add cgroups unified isolator.

[haosdent huang]

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

(Updated March 26, 2016, 4:28 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Update description.


Summary (updated)
-

Add cgroups unified isolator.


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


Repository: mesos


Description (updated)
---

Add cgroups unified isolator.


Diffs
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 45083: Added a any mechanism for futures.

[haosdent huang]

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



@jieyu My concern about this ticket is the unclear behaviour of `any` when some 
of futures success while some of futures failed. If we don't have this helper 
function, we still could do simliar thing in `CgroupsIsolatorProcess`. If you 
think we'd better not to add this, feel free to discard this patch.

- haosdent huang


On March 26, 2016, 1:46 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45083/
> ---
> 
> (Updated March 26, 2016, 1:46 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5038
> https://issues.apache.org/jira/browse/MESOS-5038
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a any mechanism for futures.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/collect.hpp 
> 5a92b72eb7668494dc832ec446a41b3d673a20cc 
>   3rdparty/libprocess/src/tests/collect_tests.cpp 
> 155e0bb75cf723a0a6c29020f9f767e3ba3d7401 
> 
> Diff: https://reviews.apache.org/r/45083/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 45086: Enable cgroups unified isolator in isolation.

[haosdent huang]

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

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


Review request for mesos and Jie Yu.


Changes
---

Fix indent


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


Repository: mesos


Description
---

Enable cgroups unified isolator in isolation.


Diffs (updated)
-

  src/CMakeLists.txt 7bda2ac684f38da94e334f0cef843614687ae4aa 
  src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
  src/slave/containerizer/mesos/containerizer.cpp 
e7f7e7fd1304e14dbfaab8b53cea16efc0417911 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 45082: Implemented cleanup() method of "network/cni" isolator.

[Qian Zhang]

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

(Updated March 26, 2016, 10:59 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 cleanup() method of "network/cni" isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
b1b7205f4f10b6dc256fcc4ecb3210105c1240b4 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
7cda5715814a0cfc4b394eb04437831e6dc44e3f 

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


Testing
---

make check


Thanks,

Qian Zhang

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

[Qian Zhang]

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

(Updated March 26, 2016, 10:53 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 (updated)
-

  src/CMakeLists.txt 7bda2ac684f38da94e334f0cef843614687ae4aa 
  src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
b1b7205f4f10b6dc256fcc4ecb3210105c1240b4 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
7cda5715814a0cfc4b394eb04437831e6dc44e3f 
  src/slave/containerizer/mesos/isolators/network/cni/paths.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/paths.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 45354: Migrate `UserCgroupIsolatorTest` to cgroups unified isolator.

[haosdent huang]

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

(Updated March 26, 2016, 2:36 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebase


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


Repository: mesos


Description
---

Migrate `UserCgroupIsolatorTest` to cgroups unified isolator.


Diffs (updated)
-

  src/tests/containerizer/cgroups_isolator_tests.cpp 
c4e467c8227f9e4129b05d173812592f39a04e06 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 45086: Enable cgroups unified isolator in isolation.

[haosdent huang]

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

(Updated March 26, 2016, 2:35 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebase


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


Repository: mesos


Description
---

Enable cgroups unified isolator in isolation.


Diffs (updated)
-

  src/CMakeLists.txt 7bda2ac684f38da94e334f0cef843614687ae4aa 
  src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
  src/slave/containerizer/mesos/containerizer.cpp 
e7f7e7fd1304e14dbfaab8b53cea16efc0417911 

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


Testing
---


Thanks,

haosdent huang

Review Request 45352: Add `CpuacctSubsystem` for cgroups unified isolator.

[haosdent huang]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Add `CpuacctSubsystem` for cgroups unified isolator.


Diffs
-

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


Testing
---


Thanks,

haosdent huang

Review Request 45354: Migrate `UserCgroupIsolatorTest` to cgroups unified isolator.

[haosdent huang]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Migrate `UserCgroupIsolatorTest` to cgroups unified isolator.


Diffs
-

  src/tests/containerizer/cgroups_isolator_tests.cpp 
c4e467c8227f9e4129b05d173812592f39a04e06 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 45086: Enable cgroups unified isolator in isolation.

[haosdent huang]

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

(Updated March 26, 2016, 1:48 p.m.)


Review request for mesos and Jie Yu.


Summary (updated)
-

Enable cgroups unified isolator in isolation.


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


Repository: mesos


Description
---

Enable cgroups unified isolator in isolation.


Diffs (updated)
-

  src/CMakeLists.txt 7bda2ac684f38da94e334f0cef843614687ae4aa 
  src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
  src/slave/containerizer/mesos/containerizer.cpp 
e7f7e7fd1304e14dbfaab8b53cea16efc0417911 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 45087: Add `CpuSubsystem` for cgroups unified isolator.

[haosdent huang]

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

(Updated March 26, 2016, 1:49 p.m.)


Review request for mesos and Jie Yu.


Summary (updated)
-

Add `CpuSubsystem` for cgroups unified isolator.


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


Repository: mesos


Description
---

Add `CpuSubsystem` for cgroups unified isolator.


Diffs (updated)
-

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


Testing
---


Thanks,

haosdent huang

Review Request 45351: Migrate test cases for `cpu` subsystem to cgroups unified isolator.

[haosdent huang]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Migrate test cases for `cpu` subsystem to cgroups unified isolator.


Diffs
-

  src/tests/containerizer/cgroups_isolator_tests.cpp 
c4e467c8227f9e4129b05d173812592f39a04e06 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 45085: Add cgroup unified isolator.

[haosdent huang]

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

(Updated March 26, 2016, 1:48 p.m.)


Review request for mesos and Jie Yu.


Summary (updated)
-

Add cgroup unified isolator.


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


Repository: mesos


Description
---

Add cgroup unified isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 

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


Testing
---


Thanks,

haosdent huang

Review Request 45350: Add `--cgroups_subsystems` in agent flags.

[haosdent huang]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Add `--cgroups_subsystems` in agent flags.


Diffs
-

  docs/configuration.md 9ad0c2a201130ac23b22b1a844396dc97ec5ff70 
  src/slave/flags.hpp 9ee7f349cc2bb3fa76141645f4a06fad57664367 
  src/slave/flags.cpp fd9fbbaa911cc77a21574ba314c50ac226fa49ce 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 45084: Add `Subsystem` abstraction for cgroups.

[haosdent huang]

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

(Updated March 26, 2016, 1:46 p.m.)


Review request for mesos and Jie Yu.


Summary (updated)
-

Add `Subsystem` abstraction for cgroups.


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


Repository: mesos


Description
---

Add `Subsystem` abstraction for cgroups.


Diffs (updated)
-

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 45083: Added a any mechanism for futures.

[haosdent huang]

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

(Updated March 26, 2016, 1:46 p.m.)


Review request for mesos and Jie Yu.


Summary (updated)
-

Added a any mechanism for futures.


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


Repository: mesos


Description
---

Added a any mechanism for futures.


Diffs (updated)
-

  3rdparty/libprocess/include/process/collect.hpp 
5a92b72eb7668494dc832ec446a41b3d673a20cc 
  3rdparty/libprocess/src/tests/collect_tests.cpp 
155e0bb75cf723a0a6c29020f9f767e3ba3d7401 

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


Testing
---


Thanks,

haosdent huang

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

[Qian Zhang]

> On March 25, 2016, 8:20 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 438
> > 
> >
> > Why do you need to get os::environment()?

The reason is, CNI plugin needs to call `iptables` to set up IPMasq, if we do 
not do this, the plugin will fail with an error:
  {
  "code": 100,
  "msg": "failed to locate iptables: exec: \"iptables\": executable file 
not found in $PATH"
  }
  
Maybe we could explicitly set `$PATH` as 
`/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin` rather than call 
`os::environment()`?


> On March 25, 2016, 8:20 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 464-472
> > 
> >
> > We cannot read stdout and stderr after the process has finished. THis 
> > is becasuse the PIPE size is bounded. If the subprocess writes a lot of 
> > data, the write will block if there's no reader.
> > 
> > Please follow the same pattern in:
> > 
> > https://github.com/apache/mesos/blob/master/src/uri/fetchers/curl.cpp#L98

Agree. However I see there are other code doing it as well, e.g., 
https://github.com/apache/mesos/blob/0.28.0/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp#L3173:L3199
https://github.com/apache/mesos/blob/0.28.0/src/docker/docker.cpp#L153:L173

Maybe we need to fix them as well?


- Qian


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


On March 23, 2016, 10:22 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 23, 2016, 10:22 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/CMakeLists.txt 0bd7a978306488b687a7e2eeeb8a5c9766d43548 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3878a7e85fe24175b3bd5e3a6268cf32a07f2d8b 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 9552312f9ba1e4df6564cfb737cc41e041cf4407 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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

[Qian Zhang]

> On March 21, 2016, 11:57 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 445
> > 
> >
> > Why the `CHECK_READY` here ? If the future is not READY it could be in 
> > error, which is fine we should just return an ERROR ?
> 
> Qian Zhang wrote:
> Because when `NetworkCniIsolatorProcess::__connect` is called, we expect 
> `output` must be ready otherwise `NetworkCniIsolatorProcess::__connect` 
> should not be called. I think it is a common way to use `then`, please take a 
> look at: 
> https://github.com/apache/mesos/blob/0.28.0/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp#L3173:L3186
>  as a reference.
> 
> Avinash sridharan wrote:
> The fact that a plugin might misbehave should not cause the agent to 
> crash. I think this defensive check doesn't serve much purpose apart from 
> causing a potential panic in the Agent.

I have revised these code based on Jie's comments, so we do not need to call 
`CHECK_READY()` anymore.


- Qian


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


On March 23, 2016, 10:22 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 23, 2016, 10:22 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/CMakeLists.txt 0bd7a978306488b687a7e2eeeb8a5c9766d43548 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3878a7e85fe24175b3bd5e3a6268cf32a07f2d8b 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 9552312f9ba1e4df6564cfb737cc41e041cf4407 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44087: Moved logic to assign process to freezer hierarchy into parentHook.

[Joris Van Remoortere]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/linux_launcher.cpp (line 251)


new line before the `NOTE:`



src/slave/containerizer/mesos/linux_launcher.cpp (line 305)


`NOTE: The child process...`



src/slave/containerizer/mesos/linux_launcher.cpp (lines 315 - 324)


why not create the lambda inline?



src/slave/containerizer/mesos/linux_launcher.cpp (line 317)


Can we follow the explicit address `&` pattern here?



src/slave/containerizer/mesos/linux_launcher.cpp (lines 322 - 323)


maybe put this comment above the first hook? The `at this point` is 
confusing based on the explicit position of the comment.


- Joris Van Remoortere


On March 14, 2016, 3:44 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44087/
> ---
> 
> (Updated March 14, 2016, 3:44 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4839
> https://issues.apache.org/jira/browse/MESOS-4839
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously assigning the process to the freezer hierarchy basically 
> reimplemented the same blocking and error handling logic provided by 
> parentHooks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 9c80cfb621ef2e28aabfb2649846892964d2d4f3 
> 
> Diff: https://reviews.apache.org/r/44087/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (on Linux)
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 44791: Changed subprocess comment to reflect Error handling of parent Hooks.

[Joris Van Remoortere]

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


Ship it!




- Joris Van Remoortere


On March 14, 2016, 3:45 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44791/
> ---
> 
> (Updated March 14, 2016, 3:45 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4839
> https://issues.apache.org/jira/browse/MESOS-4839
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously the Error behavior (in particular where the child process is 
> killed) was not consistent among Error cases.
> We should establish the convention that the Hook itself returns an error, 
> while the subprocess code is resposible for
> killing the child process in case of such Error.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess.cpp 
> 16c327db6fde1a1bac92d9e8f7cc80e57b028c1a 
> 
> Diff: https://reviews.apache.org/r/44791/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (on Linux)
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 44790: Refactored Error behavior of 'extendLifetime' parentHook.

[Joris Van Remoortere]

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


Fix it, then Ship it!





src/linux/systemd.cpp (lines 73 - 74)


let's add a `NOTE` here explaining that returning an error implies the 
child will be killed.
I know you added this to the header, but it's great to have it with the 
implementation.



src/linux/systemd.cpp (line 81)


`is not configured as enabled`


- Joris Van Remoortere


On March 14, 2016, 3:44 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44790/
> ---
> 
> (Updated March 14, 2016, 3:44 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4839
> https://issues.apache.org/jira/browse/MESOS-4839
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously the Error behavior (in particular where the child process is 
> killed) was not consistent among Error cases.
> We should establish the convention that the Hook itself returns an error, 
> while the subprocess code is resposible for
> killing the child process in case of such Error.
> 
> 
> Diffs
> -
> 
>   src/linux/systemd.hpp 6b240b9cc2fe559d111a4235e70f00e07f3d4495 
>   src/linux/systemd.cpp e120d2ceffe6bb400859f2a132f02a99f561f856 
> 
> Diff: https://reviews.apache.org/r/44790/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (on Linux)
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

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

[Jiang Yan Xu]

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




configure.ac (line 258)


Two spaces to the right.

Also, append "default: no"



configure.ac (line 259)


We probably shouldn't take option arugments, i.e., 
`--enable-xfs-disk-isolator` is sufficient. Otherwise there is confusion with 
`--enable-xfs-disk-isolator=yes|true|1|absolutely`. :)

s/[enable_xfs_disk_isolator=no]/[] for consistency.



configure.ac (line 930)


Kill this blank line.

Also, I know the headers below imply Linux but I guess it won't hurt to add 

```
  AS_IF([test "$OS_NAME" = "linux"],
[],
[AC_MSG_ERROR([cannot build xfs disk isolator
---
XFS disk isolator is only supported on Linux.
---
  ])])
```

so the error message is clearer.



configure.ac (line 934)


Move one space to the right.



configure.ac (line 954)


This could be the place we insert 

```
AC_DEFINE([ENABLE_XFS_DISK_ISOLATOR])
```



configure.ac (lines 957 - 960)


Can we put AC_MSG_CHECKING before `AC_CHECK_HEADERS` and inside `AS_IF` 
above?

i.e., AC_MSG_CHECKING/AC_MSG_CHECKING/AC_MSG_ERROR inform user what they 
don't already know (whether the system dependencies are met), not what they 
already know (the fact that they provided the `--enable_xfs_disk_isolator` 
flag).

And this check is conditional: we only print `AC_MSG_CHECKING` and do these 
checks when `test "x$enable_xfs_disk_isolator" = "xyes"`; we print 
`AC_MSG_ERROR([...])` when such checks fail and `AC_MSG_RESULT([yes])` when all 
checks succeed.

Would this work?

If we do this, then the message could be `AC_MSG_CHECKING([whether we can 
enable the XFS disk isolator])`



configure.ac (lines 962 - 963)


If we pull this up into the aforementioned place we don't need to `test 
"x$enable_xfs_disk_isolator" = "xyes"` repeatedly and therefore be more concise.


- Jiang Yan Xu


On March 22, 2016, 4:21 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> ---
> 
> (Updated March 22, 2016, 4:21 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 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[haosdent huang]

> On March 19, 2016, 10:10 p.m., Jie Yu wrote:
> > src/linux/xfs.cpp, line 17
> > 
> >
> > Move this after below stout headers.
> 
> James Peach wrote:
> The Mesos C++ style guide says that this should come first.
> 
> Jie Yu wrote:
> Sampled a few files, non of them are in this style. Either the style 
> guide is not accurate, or we don't follow Mesos style guide consistently 
> (which is bad).
> 
> Fot the time being, let's just be consistent?

The rule in [our 
clang-format](https://github.com/apache/mesos/blob/master/support/clang-format#L38-L46)
 is 

```
IncludeCategories:
  - Regex:   '^<.*.h>'
Priority:1
  - Regex:   '^<.*.hpp>'
Priority:3
  - Regex:   '^<.*>'
Priority:2
  - Regex:   '.*'
Priority:4
```


- haosdent


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


On March 22, 2016, 11:24 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated March 22, 2016, 11:24 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 utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/isolators/disk/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>