Re: Review Request 69243: Used a `switch` statement to match enum values.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [69243]

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

- Mesos Reviewbot


On Nov. 2, 2018, 8:51 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69243/
> ---
> 
> (Updated Nov. 2, 2018, 8:51 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used a `switch` statement to match enum values.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp f42fb31d7edc4a9353cbf1d74b761cae15d20dbe 
> 
> 
> Diff: https://reviews.apache.org/r/69243/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69242: Saved some disk space in mesos-tidy Docker image.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [69242]

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

- Mesos Reviewbot


On Nov. 3, 2018, 4:50 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69242/
> ---
> 
> (Updated Nov. 3, 2018, 4:50 a.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Saved some disk space in mesos-tidy Docker image.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy/Dockerfile 2885beb19582e79ae49e2c2d106cc22bc860b5c4 
> 
> 
> Diff: https://reviews.apache.org/r/69242/diff/1/
> 
> 
> Testing
> ---
> 
> Successfully ran `mesos-tidy.sh` using this image.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69211: Improved the code comments for `getContainerDevicesPath`.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69211 was successfully built and tested.

Reviews applied: `['69086', '69210', '69211']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2570/mesos-review-69211

- Mesos Reviewbot Windows


On Oct. 30, 2018, 9:04 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69211/
> ---
> 
> (Updated Oct. 30, 2018, 9:04 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-9319
> https://issues.apache.org/jira/browse/MESOS-9319
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved the code comments for `getContainerDevicesPath`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/paths.hpp 
> de3981db7eb08e53901547037c947f594c8d46ab 
> 
> 
> Diff: https://reviews.apache.org/r/69211/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 28)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 69208: Updated new CLI task attach/exec exit strategy.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Okt. 30, 2018, 12:55 vorm., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69208/
> ---
> 
> (Updated Okt. 30, 2018, 12:55 vorm.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-9363
> https://issues.apache.org/jira/browse/MESOS-9363
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated new CLI task attach/exec exit strategy.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/lib/cli/mesos.py 
> 4a5777bf712c3e5fd20a2bfd6da8fb9f3975d120 
>   src/python/cli_new/lib/cli/plugins/task/main.py 
> 5866a23aa0c0dfe141fd8fbe4c9ccd04a2c13a08 
>   src/python/cli_new/lib/cli/tests/task.py 
> ce3a55d4b66ca74b6be892d058644281ee5c803d 
> 
> 
> Diff: https://reviews.apache.org/r/69208/diff/2/
> 
> 
> Testing
> ---
> 
> ```
> (mesos-cli) ?  cli_new (exec-attach) ? mesos-cli-tests
> Running the Mesos CLI unit tests
> 
> TestAgentPlugin
> test_list (cli.tests.agent.TestAgentPlugin) ... ok
> 
> TestInfrastructure
> test_capture_output (cli.tests.tests.TestInfrastructure) ... ok
> test_launch_binaries (cli.tests.tests.TestInfrastructure) ... ok
> 
> TestTaskPlugin
> test_exec (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_exit_status (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_interactive (cli.tests.task.TestTaskPlugin) ... ok
> test_list (cli.tests.task.TestTaskPlugin) ... ok
> 
> --
> Ran 7 tests in 17.238s
> 
> OK
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 69208: Updated new CLI task attach/exec exit strategy.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69208 was successfully built and tested.

Reviews applied: `['69206', '69208']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2569/mesos-review-69208

- Mesos Reviewbot Windows


On Oct. 30, 2018, 12:55 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69208/
> ---
> 
> (Updated Oct. 30, 2018, 12:55 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-9363
> https://issues.apache.org/jira/browse/MESOS-9363
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated new CLI task attach/exec exit strategy.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/lib/cli/mesos.py 
> 4a5777bf712c3e5fd20a2bfd6da8fb9f3975d120 
>   src/python/cli_new/lib/cli/plugins/task/main.py 
> 5866a23aa0c0dfe141fd8fbe4c9ccd04a2c13a08 
>   src/python/cli_new/lib/cli/tests/task.py 
> ce3a55d4b66ca74b6be892d058644281ee5c803d 
> 
> 
> Diff: https://reviews.apache.org/r/69208/diff/2/
> 
> 
> Testing
> ---
> 
> ```
> (mesos-cli) ?  cli_new (exec-attach) ? mesos-cli-tests
> Running the Mesos CLI unit tests
> 
> TestAgentPlugin
> test_list (cli.tests.agent.TestAgentPlugin) ... ok
> 
> TestInfrastructure
> test_capture_output (cli.tests.tests.TestInfrastructure) ... ok
> test_launch_binaries (cli.tests.tests.TestInfrastructure) ... ok
> 
> TestTaskPlugin
> test_exec (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_exit_status (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_interactive (cli.tests.task.TestTaskPlugin) ... ok
> test_list (cli.tests.task.TestTaskPlugin) ... ok
> 
> --
> Ran 7 tests in 17.238s
> 
> OK
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 69237: Simplified newline handling in 'test_exec()' test for new CLI.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69237 was successfully built and tested.

Reviews applied: `['69237']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2568/mesos-review-69237

- Mesos Reviewbot Windows


On Nov. 2, 2018, 6:28 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69237/
> ---
> 
> (Updated Nov. 2, 2018, 6:28 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-9343
> https://issues.apache.org/jira/browse/MESOS-9343
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test was previously using '.strip()' to compare the command stdout
> and the result we expect. By replacing the task command to use 'printf'
> instead of 'echo', we are able to simplify this assertion.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/lib/cli/tests/task.py 
> ce3a55d4b66ca74b6be892d058644281ee5c803d 
> 
> 
> Diff: https://reviews.apache.org/r/69237/diff/2/
> 
> 
> Testing
> ---
> 
> ```
> (mesos-cli) ?  cli_new (exec-attach) ? mesos-cli-tests
> Running the Mesos CLI unit tests
> 
> TestAgentPlugin
> test_list (cli.tests.agent.TestAgentPlugin) ... ok
> 
> TestInfrastructure
> test_capture_output (cli.tests.tests.TestInfrastructure) ... ok
> test_launch_binaries (cli.tests.tests.TestInfrastructure) ... ok
> 
> TestTaskPlugin
> test_exec (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_exit_status (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_interactive (cli.tests.task.TestTaskPlugin) ... ok
> test_list (cli.tests.task.TestTaskPlugin) ... ok
> 
> --
> Ran 7 tests in 17.238s
> 
> OK
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 69116: Added 'popen_tty()' to test util functions for the new CLI.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69116 was successfully built and tested.

Reviews applied: `['69116']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2567/mesos-review-69116

- Mesos Reviewbot Windows


On Oct. 22, 2018, 12:23 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69116/
> ---
> 
> (Updated Oct. 22, 2018, 12:23 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-9343
> https://issues.apache.org/jira/browse/MESOS-9343
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This code was pulled directly from:
> https://github.com/dcos/dcos-core-cli/blob/
> 7fd55421939a7782c237e2b8719c0fe2f543acd7/
> python/lib/dcoscli/dcoscli/test/common.py
> 
> This function will be used by tests requiring a TTY. This will be the
> case for tests concerning the 'task attach' subcommand.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/lib/cli/tests/base.py 
> 7fd39af665164eeef8bb1359de340ef923dc2272 
> 
> 
> Diff: https://reviews.apache.org/r/69116/diff/3/
> 
> 
> Testing
> ---
> 
> ```
> (mesos-cli) ?  cli_new (exec-attach) ? mesos-cli-tests
> Running the Mesos CLI unit tests
> 
> TestAgentPlugin
> test_list (cli.tests.agent.TestAgentPlugin) ... ok
> 
> TestInfrastructure
> test_capture_output (cli.tests.tests.TestInfrastructure) ... ok
> test_launch_binaries (cli.tests.tests.TestInfrastructure) ... ok
> 
> TestTaskPlugin
> test_exec (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_exit_status (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_interactive (cli.tests.task.TestTaskPlugin) ... ok
> test_list (cli.tests.task.TestTaskPlugin) ... ok
> 
> --
> Ran 7 tests in 17.238s
> 
> OK
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 69206: Updated new CLI commands for new CLI to return proper exit status.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Okt. 30, 2018, 12:51 vorm., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69206/
> ---
> 
> (Updated Okt. 30, 2018, 12:51 vorm.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-9363
> https://issues.apache.org/jira/browse/MESOS-9363
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In a later commit, this will be used by the two subcommands 'task
> attach' and 'task exec' to return their proper exit sequences.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/bin/main.py 27783caf4eab6ea18eee5a7b90b0dd368c8b4020 
>   src/python/cli_new/lib/cli/plugins/base.py 
> c85abd6755c4b046ab4ecbda825f3a1615fb4ce5 
> 
> 
> Diff: https://reviews.apache.org/r/69206/diff/2/
> 
> 
> Testing
> ---
> 
> ```
> (mesos-cli) ?  cli_new (exec-attach) ? mesos-cli-tests
> Running the Mesos CLI unit tests
> 
> TestAgentPlugin
> test_list (cli.tests.agent.TestAgentPlugin) ... ok
> 
> TestInfrastructure
> test_capture_output (cli.tests.tests.TestInfrastructure) ... ok
> test_launch_binaries (cli.tests.tests.TestInfrastructure) ... ok
> 
> TestTaskPlugin
> test_exec (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_exit_status (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_interactive (cli.tests.task.TestTaskPlugin) ... ok
> test_list (cli.tests.task.TestTaskPlugin) ... ok
> 
> --
> Ran 7 tests in 17.238s
> 
> OK
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 69237: Simplified newline handling in 'test_exec()' test for new CLI.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Nov. 1, 2018, 10:28 nachm., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69237/
> ---
> 
> (Updated Nov. 1, 2018, 10:28 nachm.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-9343
> https://issues.apache.org/jira/browse/MESOS-9343
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test was previously using '.strip()' to compare the command stdout
> and the result we expect. By replacing the task command to use 'printf'
> instead of 'echo', we are able to simplify this assertion.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/lib/cli/tests/task.py 
> ce3a55d4b66ca74b6be892d058644281ee5c803d 
> 
> 
> Diff: https://reviews.apache.org/r/69237/diff/2/
> 
> 
> Testing
> ---
> 
> ```
> (mesos-cli) ?  cli_new (exec-attach) ? mesos-cli-tests
> Running the Mesos CLI unit tests
> 
> TestAgentPlugin
> test_list (cli.tests.agent.TestAgentPlugin) ... ok
> 
> TestInfrastructure
> test_capture_output (cli.tests.tests.TestInfrastructure) ... ok
> test_launch_binaries (cli.tests.tests.TestInfrastructure) ... ok
> 
> TestTaskPlugin
> test_exec (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_exit_status (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_interactive (cli.tests.task.TestTaskPlugin) ... ok
> test_list (cli.tests.task.TestTaskPlugin) ... ok
> 
> --
> Ran 7 tests in 17.238s
> 
> OK
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 69207: Moved 'updated_tasks()' to new CLI tests base.

[Kevin Klues]

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


Ship it!




I reworked this commit slightly before pushing it in order to keep the test 
code cleaner.

- Kevin Klues


On Okt. 30, 2018, 12:53 vorm., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69207/
> ---
> 
> (Updated Okt. 30, 2018, 12:53 vorm.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-9343
> https://issues.apache.org/jira/browse/MESOS-9343
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved 'updated_tasks()' to new CLI tests base.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/lib/cli/tests/base.py 
> 7fd39af665164eeef8bb1359de340ef923dc2272 
>   src/python/cli_new/lib/cli/tests/task.py 
> ce3a55d4b66ca74b6be892d058644281ee5c803d 
> 
> 
> Diff: https://reviews.apache.org/r/69207/diff/3/
> 
> 
> Testing
> ---
> 
> ```
> (mesos-cli) ?  cli_new (exec-attach) ? mesos-cli-tests
> Running the Mesos CLI unit tests
> 
> TestAgentPlugin
> test_list (cli.tests.agent.TestAgentPlugin) ... ok
> 
> TestInfrastructure
> test_capture_output (cli.tests.tests.TestInfrastructure) ... ok
> test_launch_binaries (cli.tests.tests.TestInfrastructure) ... ok
> 
> TestTaskPlugin
> test_exec (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_exit_status (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_interactive (cli.tests.task.TestTaskPlugin) ... ok
> test_list (cli.tests.task.TestTaskPlugin) ... ok
> 
> --
> Ran 7 tests in 17.238s
> 
> OK
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 69161: Renamed a function argument to not reuse member name.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!




Thanks for cleaning this up!


src/resource_provider/storage/provider.cpp
Line 3065 (original), 3065 (patched)


Maybe `operationInfo`, or just `operation`?
Also if we go with this naming it would be nice to rename it in the 
declaration as well.


- Chun-Hung Hsiao


On Oct. 25, 2018, 10:54 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69161/
> ---
> 
> (Updated Oct. 25, 2018, 10:54 a.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While a function argument shadowing a member variable is perfectly
> legal, it is still confusing. In this patch we rename the function
> argument to remove one case of such shadowing.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> db783b53558811081fb2671e005e8bbbd9edbede 
> 
> 
> Diff: https://reviews.apache.org/r/69161/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69207: Moved 'updated_tasks()' to new CLI tests base.

[Kevin Klues]

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




src/python/cli_new/lib/cli/tests/base.py
Lines 507 (patched)


As a helper, calling this `updated_tasks()` is weird. Let's call it 
`running_tasks()`.


- Kevin Klues


On Okt. 30, 2018, 12:53 vorm., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69207/
> ---
> 
> (Updated Okt. 30, 2018, 12:53 vorm.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-9343
> https://issues.apache.org/jira/browse/MESOS-9343
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved 'updated_tasks()' to new CLI tests base.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/lib/cli/tests/base.py 
> 7fd39af665164eeef8bb1359de340ef923dc2272 
>   src/python/cli_new/lib/cli/tests/task.py 
> ce3a55d4b66ca74b6be892d058644281ee5c803d 
> 
> 
> Diff: https://reviews.apache.org/r/69207/diff/3/
> 
> 
> Testing
> ---
> 
> ```
> (mesos-cli) ?  cli_new (exec-attach) ? mesos-cli-tests
> Running the Mesos CLI unit tests
> 
> TestAgentPlugin
> test_list (cli.tests.agent.TestAgentPlugin) ... ok
> 
> TestInfrastructure
> test_capture_output (cli.tests.tests.TestInfrastructure) ... ok
> test_launch_binaries (cli.tests.tests.TestInfrastructure) ... ok
> 
> TestTaskPlugin
> test_exec (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_exit_status (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_interactive (cli.tests.task.TestTaskPlugin) ... ok
> test_list (cli.tests.task.TestTaskPlugin) ... ok
> 
> --
> Ran 7 tests in 17.238s
> 
> OK
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 69116: Added 'popen_tty()' to test util functions for the new CLI.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Okt. 22, 2018, 12:23 nachm., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69116/
> ---
> 
> (Updated Okt. 22, 2018, 12:23 nachm.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-9343
> https://issues.apache.org/jira/browse/MESOS-9343
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This code was pulled directly from:
> https://github.com/dcos/dcos-core-cli/blob/
> 7fd55421939a7782c237e2b8719c0fe2f543acd7/
> python/lib/dcoscli/dcoscli/test/common.py
> 
> This function will be used by tests requiring a TTY. This will be the
> case for tests concerning the 'task attach' subcommand.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/lib/cli/tests/base.py 
> 7fd39af665164eeef8bb1359de340ef923dc2272 
> 
> 
> Diff: https://reviews.apache.org/r/69116/diff/3/
> 
> 
> Testing
> ---
> 
> ```
> (mesos-cli) ?  cli_new (exec-attach) ? mesos-cli-tests
> Running the Mesos CLI unit tests
> 
> TestAgentPlugin
> test_list (cli.tests.agent.TestAgentPlugin) ... ok
> 
> TestInfrastructure
> test_capture_output (cli.tests.tests.TestInfrastructure) ... ok
> test_launch_binaries (cli.tests.tests.TestInfrastructure) ... ok
> 
> TestTaskPlugin
> test_exec (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_exit_status (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_interactive (cli.tests.task.TestTaskPlugin) ... ok
> test_list (cli.tests.task.TestTaskPlugin) ... ok
> 
> --
> Ran 7 tests in 17.238s
> 
> OK
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 69243: Used a `switch` statement to match enum values.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On Nov. 2, 2018, 8:51 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69243/
> ---
> 
> (Updated Nov. 2, 2018, 8:51 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used a `switch` statement to match enum values.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp f42fb31d7edc4a9353cbf1d74b761cae15d20dbe 
> 
> 
> Diff: https://reviews.apache.org/r/69243/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69242: Saved some disk space in mesos-tidy Docker image.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On Nov. 2, 2018, 8:50 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69242/
> ---
> 
> (Updated Nov. 2, 2018, 8:50 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Saved some disk space in mesos-tidy Docker image.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy/Dockerfile 2885beb19582e79ae49e2c2d106cc22bc860b5c4 
> 
> 
> Diff: https://reviews.apache.org/r/69242/diff/1/
> 
> 
> Testing
> ---
> 
> Successfully ran `mesos-tidy.sh` using this image.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69207: Moved 'updated_tasks()' to new CLI tests base.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the current review.

Failed command: `python.exe .\support\apply-reviews.py -n -r 69207`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2566/mesos-review-69207

Relevant logs:

- 
[apply-review-69207.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2566/mesos-review-69207/logs/apply-review-69207.log):

```
error: patch failed: src/python/cli_new/lib/cli/tests/base.py:498
error: src/python/cli_new/lib/cli/tests/base.py: patch does not apply
```

- Mesos Reviewbot Windows


On Oct. 30, 2018, 12:53 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69207/
> ---
> 
> (Updated Oct. 30, 2018, 12:53 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-9343
> https://issues.apache.org/jira/browse/MESOS-9343
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved 'updated_tasks()' to new CLI tests base.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/lib/cli/tests/base.py 
> 7fd39af665164eeef8bb1359de340ef923dc2272 
>   src/python/cli_new/lib/cli/tests/task.py 
> ce3a55d4b66ca74b6be892d058644281ee5c803d 
> 
> 
> Diff: https://reviews.apache.org/r/69207/diff/2/
> 
> 
> Testing
> ---
> 
> ```
> (mesos-cli) ?  cli_new (exec-attach) ? mesos-cli-tests
> Running the Mesos CLI unit tests
> 
> TestAgentPlugin
> test_list (cli.tests.agent.TestAgentPlugin) ... ok
> 
> TestInfrastructure
> test_capture_output (cli.tests.tests.TestInfrastructure) ... ok
> test_launch_binaries (cli.tests.tests.TestInfrastructure) ... ok
> 
> TestTaskPlugin
> test_exec (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_exit_status (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_interactive (cli.tests.task.TestTaskPlugin) ... ok
> test_list (cli.tests.task.TestTaskPlugin) ... ok
> 
> --
> Ran 7 tests in 17.238s
> 
> OK
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 69097: Added an allocator benchmark for quota performance.

[Benjamin Mahler]

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


Fix it, then Ship it!





src/tests/hierarchical_allocator_benchmarks.cpp
Lines 498-499 (patched)


Why not make it even smaller (e.g. 1/4 or 1/10th) to be more pathological?



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 601-609 (patched)


No need for the comments, seems self explanatory?


- Benjamin Mahler


On Oct. 22, 2018, 10:32 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69097/
> ---
> 
> (Updated Oct. 22, 2018, 10:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
> 
> 
> Bugs: MESOS-6630
> https://issues.apache.org/jira/browse/MESOS-6630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This benchmark evaluates the allocator performance in
> the presence of roles with both small quota (which can
> be satisfied by half an agent) as well as large quota
> (which need resources from two agents). We setup the cluster,
> trigger one allocation cycle and measure the elapsed time.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_benchmarks.cpp 
> bf9167b63747f7b8a402d950947028436307082a 
> 
> 
> Diff: https://reviews.apache.org/r/69097/diff/1/
> 
> 
> Testing
> ---
> 
> Result from optimized build on a multicore 2.2GHz machine:
> 
> Benchmark setup: 20 agents, 25 roles, 25 frameworks
> Made 30 allocations in 11.01225ms
> Made 0 allocation in 5.758192ms
> 
> Benchmark setup: 200 agents, 250 roles, 250 frameworks
> Made 300 allocations in 337.974387ms
> Made 0 allocation in 313.298636ms
> 
> Benchmark setup: 2000 agents, 2500 roles, 2500 frameworks
> Made 3000 allocations in 27.881684393secs
> Made 0 allocation in 25.013398926secs
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69098: Added a benchmark to compare quota and nonquota allocation performance.

[Benjamin Mahler]

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



Hm.. why is this one not just extending the one you added in the previous 
review?

- Benjamin Mahler


On Oct. 22, 2018, 10:39 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69098/
> ---
> 
> (Updated Oct. 22, 2018, 10:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
> 
> 
> Bugs: MESOS-6630
> https://issues.apache.org/jira/browse/MESOS-6630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This benchmark evaluates the performance difference between nonquota
> and quota settings. In both settings, the same allocations are made
> for fair comparison.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_benchmarks.cpp 
> bf9167b63747f7b8a402d950947028436307082a 
> 
> 
> Diff: https://reviews.apache.org/r/69098/diff/1/
> 
> 
> Testing
> ---
> 
> Result from optimized build on a multicore 2.2GHz machine:
> 
> Benchmark setup: 20 agents, 10 roles, 20 frameworks
> Made 20 allocations in 3.508676ms for nonquota roles
> Made 20 allocations in 7.901269ms for quota roles
> 
> Benchmark setup: 200 agents, 100 roles, 200 frameworks
> Made 200 allocations in 63.407391ms for nonquota roles
> Made 200 allocations in 279.002319ms for quota roles
> 
> Benchmark setup: 2000 agents, 1000 roles, 2000 frameworks
> Made 2000 allocations in 4.003802373secs for nonquota roles
> Made 2000 allocations in 20.503188535secs for quota roles
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69036: WIP: Changed the semantics of `CREATE_DISK` and `DESTROY_DISK` operations.

[Chun-Hung Hsiao]

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

(Updated Nov. 2, 2018, 10:22 p.m.)


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


Changes
---

Addressed James' and Benjamin's comments.


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


Repository: mesos


Description
---

The semantics of these two operations has been updated to provide
primitives to import CSI volumes and recover CSI volumes against agent
ID changes and metadata loss.


Diffs (updated)
-

  include/mesos/mesos.proto f6989cd5e7a47c3fec29bdeccc38474bf0045341 
  include/mesos/v1/mesos.proto edad35a928705f21aa1aca31f3e9cfd30810 


Diff: https://reviews.apache.org/r/69036/diff/5/

Changes: https://reviews.apache.org/r/69036/diff/4-5/


Testing
---

Test will be done later in the chain.


Thanks,

Chun-Hung Hsiao

Re: Review Request 69243: Used a `switch` statement to match enum values.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69243 was successfully built and tested.

Reviews applied: `['69243']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2564/mesos-review-69243

- Mesos Reviewbot Windows


On Nov. 2, 2018, 8:51 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69243/
> ---
> 
> (Updated Nov. 2, 2018, 8:51 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used a `switch` statement to match enum values.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp f42fb31d7edc4a9353cbf1d74b761cae15d20dbe 
> 
> 
> Diff: https://reviews.apache.org/r/69243/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69242: Saved some disk space in mesos-tidy Docker image.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69242 was successfully built and tested.

Reviews applied: `['69242']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2565/mesos-review-69242

- Mesos Reviewbot Windows


On Nov. 2, 2018, 8:50 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69242/
> ---
> 
> (Updated Nov. 2, 2018, 8:50 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Saved some disk space in mesos-tidy Docker image.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy/Dockerfile 2885beb19582e79ae49e2c2d106cc22bc860b5c4 
> 
> 
> Diff: https://reviews.apache.org/r/69242/diff/1/
> 
> 
> Testing
> ---
> 
> Successfully ran `mesos-tidy.sh` using this image.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69096: Moved a few allocator test helpers to `tests/allocator.hpp`.

[Benjamin Mahler]

> On Oct. 25, 2018, 12:22 a.m., Benjamin Mahler wrote:
> > src/tests/allocator.hpp
> > Lines 40-45 (patched)
> > 
> >
> > I think we some other create helpers lying around, e.g. createTask. Is 
> > this where these belong?
> > 
> > Do you think we need them?
> 
> Meng Zhu wrote:
> I do not have use cases of other helpers in mind atm. Thus I prefer to 
> pull in only these two.

Do we even need these helpers? They seem pretty trivial, and I wonder how much 
code it's actually simplifying. I'm fine with the patch, but it feels heavy to 
have a new header and compliation unit just for these, especially since they're 
only used in two files and are trivial.


- Benjamin


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


On Oct. 20, 2018, 1:28 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69096/
> ---
> 
> (Updated Oct. 20, 2018, 1:28 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This helps to share the helpers between
> `hierarchical_allocator_tests.cpp` and
> `hierarchical_allocator_benchmarks.cpp`.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5795c7097a9ed1f659e169ad81a9f2c09481aa81 
>   src/tests/CMakeLists.txt 00dbdee1c06e7571fa799850cdaf7f2ccadc8bea 
>   src/tests/allocator.hpp 15a5396986ea6939f25b8d23b87a91c27338fc7b 
>   src/tests/allocator.cpp PRE-CREATION 
>   src/tests/hierarchical_allocator_tests.cpp 
> 27fbd9cf0c4442e7675362a806d35bad141ffb6d 
> 
> 
> Diff: https://reviews.apache.org/r/69096/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69093: Removed `used` argument in `AgentProfile` in the allocator benchmark.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On Oct. 20, 2018, 1:27 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69093/
> ---
> 
> (Updated Oct. 20, 2018, 1:27 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently in the `HierarchicalAllocations_BENCHMARK_TestBase`,
> it is not easy to add agents with used resources because
> frameworks are created during the initialization which is
> after the agent profiles creation.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_benchmarks.cpp 
> bf9167b63747f7b8a402d950947028436307082a 
> 
> 
> Diff: https://reviews.apache.org/r/69093/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69093: Removed `used` argument in `AgentProfile` in the allocator benchmark.

[Benjamin Mahler]

> On Oct. 25, 2018, 12:20 a.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_benchmarks.cpp
> > Line 234 (original), 234 (patched)
> > 
> >
> > Hm.. how were you able to change this while it remained correct? Was 
> > this just an empty map always?
> 
> Meng Zhu wrote:
> Yeah, it is currently not used. (So far there is only one benchmark in 
> the suite and it does not utilize it). During the fixture code review, I 
> recommended Kapil to add the option to specify used resources because some of 
> the existing benchmarks in `hierarchical_allocator_test.cpp ` setup agents 
> with pre-allocated resources.
> 
> Now I realize that we can't easily do this during our cluster setup. 
> Because when we specify the `agentProfile`, no framework has been created 
> yet. I am actually not quite sure why we need to specify `used`. In the 
> future if necessary, we can always create a bunch of frameworks with their 
> own quota roles to simulate this right after the cluster initialization.

Can you clarify some of this in the description?


- Benjamin


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


On Oct. 20, 2018, 1:27 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69093/
> ---
> 
> (Updated Oct. 20, 2018, 1:27 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently in the `HierarchicalAllocations_BENCHMARK_TestBase`,
> it is not easy to add agents with used resources because
> frameworks are created during the initialization which is
> after the agent profiles creation.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_benchmarks.cpp 
> bf9167b63747f7b8a402d950947028436307082a 
> 
> 
> Diff: https://reviews.apache.org/r/69093/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69092: Added default arguments to `FrameworkProfile` in allocator benchmark.

[Benjamin Mahler]

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


Ship it!




- Benjamin Mahler


On Oct. 20, 2018, 1:27 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69092/
> ---
> 
> (Updated Oct. 20, 2018, 1:27 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For frameworks that do not care about launching tasks, we should
> provide some default task launch settings to simplify the benchmark
> settings.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_benchmarks.cpp 
> bf9167b63747f7b8a402d950947028436307082a 
> 
> 
> Diff: https://reviews.apache.org/r/69092/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69092: Added default arguments to `FrameworkProfile` in allocator benchmark.

[Benjamin Mahler]

> On Oct. 25, 2018, 12:18 a.m., Benjamin Mahler wrote:
> > Hm.. I don't understand what this is doing. Is this setting default 
> > framework behavior if someone writing a benchmark doesn't want to have to 
> > pass the arguments?
> 
> Meng Zhu wrote:
> That is correct.

Ok, could you update the description to clarify? It says frameworks that do not 
care about launching tasks, but this actually makes frameworks launch tasks by 
default?


- Benjamin


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


On Oct. 20, 2018, 1:27 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69092/
> ---
> 
> (Updated Oct. 20, 2018, 1:27 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For frameworks that do not care about launching tasks, we should
> provide some default task launch settings to simplify the benchmark
> settings.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_benchmarks.cpp 
> bf9167b63747f7b8a402d950947028436307082a 
> 
> 
> Diff: https://reviews.apache.org/r/69092/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69234: Made debug container runs as its parent container's user by default.

[Gilbert Song]

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




src/slave/http.cpp
Line 2516 (original), 2519 (patched)


I would suggest to change the behavior for all nested container. If user is 
not specified, always inherit from parent user.


- Gilbert Song


On Nov. 1, 2018, 2:25 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69234/
> ---
> 
> (Updated Nov. 1, 2018, 2:25 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-9332
> https://issues.apache.org/jira/browse/MESOS-9332
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made debug container runs as its parent container's user by default.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 03a4e0f1567b27b2efd8a443caea3a2a087d858c 
>   src/slave/http.cpp 0d27ab576d5fa52c06826703e3dad38c2d374cc9 
> 
> 
> Diff: https://reviews.apache.org/r/69234/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 69243: Used a `switch` statement to match enum values.

[Benjamin Bannier]

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

Review request for mesos and Chun-Hung Hsiao.


Repository: mesos


Description
---

Used a `switch` statement to match enum values.


Diffs
-

  src/common/protobuf_utils.cpp f42fb31d7edc4a9353cbf1d74b761cae15d20dbe 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 69242: Saved some disk space in mesos-tidy Docker image.

[Benjamin Bannier]

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

Review request for mesos and Chun-Hung Hsiao.


Repository: mesos


Description
---

Saved some disk space in mesos-tidy Docker image.


Diffs
-

  support/mesos-tidy/Dockerfile 2885beb19582e79ae49e2c2d106cc22bc860b5c4 


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


Testing
---

Successfully ran `mesos-tidy.sh` using this image.


Thanks,

Benjamin Bannier

Re: Review Request 68706: Added master failover reregistration progress metrics.

[Jiang Yan Xu]

> On Oct. 18, 2018, 11:14 a.m., Jiang Yan Xu wrote:
> > src/master/master.cpp
> > Lines 1874-1875 (patched)
> > 
> >
> > If we use equality operator and only set the timer when such a number 
> > of reregistered agent is reached we are guaranteed to only set each timer 
> > once (but we may need to set multiple timers in one call) right? This 
> > alleviates the need to check if the timer is already set!
> > 
> > This should also work with boundary cases like the total recovered 
> > agents count being 0 or 1 (overlapping percentiles) etc. Right?
> 
> Jiang Yan Xu wrote:
> We should comment on the reasons for dropping issues.
> 
> Xudong Ni wrote:
> This is actaully fixed and marked as dropped somehow. We used equality 
> operator to 0.0 to check whether the percentile was reached or not; The 
> reason we used push gauge not timer is explained in push gauge vs timer 
> comments section

The point here is "This alleviates the need to check if the timer is already 
set!". It's still in the new revision. I made another comment about it.


> On Oct. 18, 2018, 11:14 a.m., Jiang Yan Xu wrote:
> > src/master/metrics.hpp
> > Lines 51 (patched)
> > 
> >
> > On using Timer (e.g., like 
> > [state_fetch](https://github.com/apache/mesos/blob/7f36ebc1775398a43b2aa3a81bb647fb296b8313/src/master/registrar.cpp#L172))
> >  vs. PushGauge, after looking at how it'll be used I think the main 
> > advantage of Timer is that it doesn't export any value if you haven't set 
> > it.
> > 
> > Consider the two cases:
> > 
> > 1. There are 1000 recovered agents and 0 have reregsitered, should all 
> > the timers have zero values or should they be absent?
> > 
> > 2. There are 0 recovered agents (e.g., brand new cluster), should all 
> > of the metrics be zero or non-existent? I feel like they should be zero, as 
> > in, e.g., 100% of all 0 agents are reregistered within 0 secs.
> > 
> > So timer handles this natrually. Also it sets the `_secs` name for you 
> > but that's a minor conveninence.
> 
> Jiang Yan Xu wrote:
> We should comment on the reasons for dropping issues.
> 
> Xudong Ni wrote:
> Sorry about this, I did make the comments but it must be in one of draft 
> which was not saved. 
> 
> I agree that metrics should be zero but not absent when certain 
> percentige were not reached. I did tried both PushGauge and Timer 
> implementation and tested in our clusters.
> 
> If we used the timer, when the value was not set, that metric is actually 
> missing. PushGauge is set with the initial value 0.0 and we can tell whether 
> it's set yet, the metric will always exist no matter that percentile reached 
> or not, and it has better performance.
> 
> The brand new cluster case was tested and the results were included in 
> the test results.

My point was the opposite. When a percentage is not reached, I feel that the 
value being absent is preferrable and I used the above two cases to demonstrate 
it. (There is a subtle difference between the two IMO).

It's in fact the current practice used by metrics like `state_fetch`.


- Jiang Yan


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


On Oct. 19, 2018, 4:56 p.m., Xudong Ni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68706/
> ---
> 
> (Updated Oct. 19, 2018, 4:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, James Peach, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9178
> https://issues.apache.org/jira/browse/MESOS-9178
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> During the master failover, the time that the master elected is
> considered as the start of failover. In the progress of
> reregistration, the percentile represents the time when such
> percentile of agents finished registration again; The percentile of
> these data as in this metrics can represent overall reregistration
> progress; In case of degradation towards to the end of reregistration,
> the high percentile can reflect it; In the case there are unreachable
> agents in the failover, if certain percentile recovery couldn't be
> reached, the intiail value of that percentile will not be updated.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 868787bb2f9d879531402f83507b322462322efc 
>   src/master/metrics.hpp e1da18e6ba2737f729e1e30653020538150ae898 
> 
> 
> Diff: https://reviews.apache.org/r/68706/diff/7/
> 
> 
> Testing
> ---
> 
> Automation:
> [ RUN  ] MasterTest.MetricsInMetricsEndpoint

Re: Review Request 68706: Added master failover reregistration progress metrics.

[Jiang Yan Xu]

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



There were changes added for a test but it's gone in the latest revision?


src/master/master.cpp
Lines 1720 (patched)


You can construct the entire `Option 
agentReregistrations` struct here using the `expectedAgentCount` as well as the 
`electedTime` which is a class member (Rationale explained below).

Initializing `agentReregistrations` is just one step in the list of steps 
taken in `Master::_recover` so you don't have to squeeze it between the two 
lines for allocator recover code. It's not appropriate as the block comment 
above clearly tries to explain the allocator reocvery. 

You can just put it somewhere below by itself (blank lines above and below 
the statement). The `expectedAgentCount` variable is already set and you can 
use; an frankly you can just use `registry.slaves().slaves().size()` as well.



src/master/master.cpp
Lines 2099 (patched)


- End sentences with `.`
- s/master/the master/



src/master/metrics.hpp
Lines 87 (patched)


- End sentences with `.`.
- s/when/during/



src/master/metrics.hpp
Lines 90 (patched)


No need for `explicit` as it's only for single argument constructors, you 
don't need it either with zero or two arguments (one proposal below).



src/master/metrics.hpp
Lines 90-123 (patched)


To be consistent let's put the implementation in the cpp file 
master/metrics.cpp. Here and for other methods as well.



src/master/metrics.hpp
Lines 92 (patched)


Indent two more spaces, we can see examples in master/metrics.cpp, here and 
elsewhere.



src/master/metrics.hpp
Lines 130 (patched)


We generally don't use one letter variable names. `t` here seemingly refers 
to `Time` but it's a `Duration`. It won't hurt to keep it strongly types until 
we need to convert it, i.e., Use `Duration duration = Clock::now() - 
electedTime;` should be fine.



src/master/metrics.hpp
Lines 132 (patched)


A space between `if` and `(`. Here and elsewhere.



src/master/metrics.hpp
Lines 133 (patched)


Let's `#include ` for ceil, in general include all needed headers 
without relying on transitive dependencies is a good pratice.



src/master/metrics.hpp
Lines 138 (patched)


As I commented in the previous revision, we don't need to check if a timer 
is already set. A percentage timer corresponds to one precise 
`reregisteredAgentCount` (and not vice versa) so we just need to check that. 
Does it make sense?



src/master/metrics.hpp
Lines 176-177 (patched)


Semantically it's hard to explain why `recoveredAgentCount` is an `Option` 
while `reregisteredAgentCount` is not? Why not have all three fields as 
Options? But then one may ask why not have the entire struct by an Option?

It's true that we don't have a `recoveredAgentCount` before they are 
recovered and `electedTime` before the master is elected but we can just use 
`Option agentReregistrations` like I suggested?

The semantics of `Option agentReregistrations` seems 
natural to me: it is optional because agent reregistrations only happen after 
the master is recovered.


- Jiang Yan Xu


On Oct. 19, 2018, 4:56 p.m., Xudong Ni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68706/
> ---
> 
> (Updated Oct. 19, 2018, 4:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, James Peach, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9178
> https://issues.apache.org/jira/browse/MESOS-9178
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> During the master failover, the time that the master elected is
> considered as the start of failover. In the progress of
> reregistration, the percentile represents the time when such
> percentile of agents finished registration again; The percentile of
> these data as in this metrics can represent overall reregistration
> progress; In case of degradation towards to the end of reregistration,
> the high percentile can reflect it; In the case there are unreachable
> agents in the failover, if certain percentile recovery couldn't be
> reached, the