You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Marco Massenzio <ma...@mesosphere.io> on 2015/05/18 19:28:07 UTC
Re: Review Request 34195: Refactoring to use BaseFlags common
functionality
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34195/
-----------------------------------------------------------
(Updated May 18, 2015, 5:28 p.m.)
Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
Bugs: MESOS-2711
https://issues.apache.org/jira/browse/MESOS-2711
Repository: mesos
Description
-------
Jira: MESOS-2711
All the main() methods have been refactored to use the
definition of BaseFlags::help flag and BaseFlags::printUsage().
This CL also tries to bring some uniformity to the
use of exit codes: if this is deemed to be worth
making it uniform, we can come up with common
rules and extend the changes here to be compliant.
This touches a lot of files, but keep scrolling, and you will see a pattern emerge ;-)
TODO(marco): we should also abstract away the error checking
Diffs (updated)
-----
src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b
src/examples/load_generator_framework.cpp be1a3bf5f16bd811cb4039c8f15478183712a426
src/examples/persistent_volume_framework.cpp 8a893fcfa3d5d988a88fdeaf0bfc08e0a49b7a65
src/health-check/main.cpp a4ce742ab8deff1ebd99359112670493fdaeeac3
src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
src/local/main.cpp a641b9e83862743890597a2981a9419517e7c589
src/log/tool/benchmark.cpp 01e55115f35d155efbea190b5308b294ba76e7cb
src/log/tool/initialize.cpp ccda7fb1c0f7113f865ec61adee76b2ea6180442
src/log/tool/read.cpp d14138502f5bc9a725deb83da505765865da017f
src/log/tool/replica.cpp 3985fc7df4f7153ae623589fbdd769ccbae57125
src/master/main.cpp d5666bc8ee8d7a0f0b8685f76d65dd1f9ac2a280
src/slave/containerizer/isolators/network/port_mapping.hpp c72fb47f60f40cda8d84a10497b9133f83cf018e
src/slave/containerizer/isolators/network/port_mapping.cpp a4abaff30bb4646b1b1edfdbbc243c9e3f6851df
src/slave/main.cpp f762f5b06be74c391cbc336b2da28f8358952ba4
src/tests/main.cpp e3fff5d60c0468c0d258f2bb301efc1309c071b0
src/usage/main.cpp 97f55e938dc7678f8331970d8953d09218f70902
Diff: https://reviews.apache.org/r/34195/diff/
Testing
-------
make check
**NOTE** this fixes completely the chained changes from 3419{3,4} and makes all the tests pass.
Thanks,
Marco Massenzio
Re: Review Request 34195: Refactoring to use BaseFlags common
functionality
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34195/#review84186
-----------------------------------------------------------
Bad patch!
Reviews applied: [34193, 34193]
Failed command: ./support/apply-review.sh -n -r 34193
Error:
2015-05-18 18:35:17 URL:https://reviews.apache.org/r/34193/diff/raw/ [5544/5544] -> "34193.patch" [1]
error: patch failed: 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp:42
error: 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp: patch does not apply
error: patch failed: 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp:483
error: 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On May 18, 2015, 5:28 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34195/
> -----------------------------------------------------------
>
> (Updated May 18, 2015, 5:28 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Bugs: MESOS-2711
> https://issues.apache.org/jira/browse/MESOS-2711
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2711
>
> All the main() methods have been refactored to use the
> definition of BaseFlags::help flag and BaseFlags::printUsage().
>
> This CL also tries to bring some uniformity to the
> use of exit codes: if this is deemed to be worth
> making it uniform, we can come up with common
> rules and extend the changes here to be compliant.
>
> This touches a lot of files, but keep scrolling, and you will see a pattern emerge ;-)
>
> TODO(marco): we should also abstract away the error checking
>
>
> Diffs
> -----
>
> src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
> src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b
> src/examples/load_generator_framework.cpp be1a3bf5f16bd811cb4039c8f15478183712a426
> src/examples/persistent_volume_framework.cpp 8a893fcfa3d5d988a88fdeaf0bfc08e0a49b7a65
> src/health-check/main.cpp a4ce742ab8deff1ebd99359112670493fdaeeac3
> src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
> src/local/main.cpp a641b9e83862743890597a2981a9419517e7c589
> src/log/tool/benchmark.cpp 01e55115f35d155efbea190b5308b294ba76e7cb
> src/log/tool/initialize.cpp ccda7fb1c0f7113f865ec61adee76b2ea6180442
> src/log/tool/read.cpp d14138502f5bc9a725deb83da505765865da017f
> src/log/tool/replica.cpp 3985fc7df4f7153ae623589fbdd769ccbae57125
> src/master/main.cpp d5666bc8ee8d7a0f0b8685f76d65dd1f9ac2a280
> src/slave/containerizer/isolators/network/port_mapping.hpp c72fb47f60f40cda8d84a10497b9133f83cf018e
> src/slave/containerizer/isolators/network/port_mapping.cpp a4abaff30bb4646b1b1edfdbbc243c9e3f6851df
> src/slave/main.cpp f762f5b06be74c391cbc336b2da28f8358952ba4
> src/tests/main.cpp e3fff5d60c0468c0d258f2bb301efc1309c071b0
> src/usage/main.cpp 97f55e938dc7678f8331970d8953d09218f70902
>
> Diff: https://reviews.apache.org/r/34195/diff/
>
>
> Testing
> -------
>
> make check
>
> **NOTE** this fixes completely the chained changes from 3419{3,4} and makes all the tests pass.
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 34195: Refactoring to use BaseFlags common
functionality
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34195/#review85321
-----------------------------------------------------------
Patch looks great!
Reviews applied: [34193, 34195]
All tests passed.
- Mesos ReviewBot
On May 26, 2015, 8:58 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34195/
> -----------------------------------------------------------
>
> (Updated May 26, 2015, 8:58 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Bugs: MESOS-2711
> https://issues.apache.org/jira/browse/MESOS-2711
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2711
>
> All the main() methods have been refactored to use the
> definition of FlagsBase::help flag and FlagsBase::usage().
>
> This CL also tries to bring some uniformity to the
> use of exit codes: if this is deemed to be worth
> making it uniform, we can come up with common
> rules and extend the changes here to be compliant.
>
> This touches a lot of files, but keep scrolling, and you will see a pattern emerge.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
> src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b
> src/examples/load_generator_framework.cpp be1a3bf5f16bd811cb4039c8f15478183712a426
> src/examples/persistent_volume_framework.cpp 8a893fcfa3d5d988a88fdeaf0bfc08e0a49b7a65
> src/health-check/main.cpp a4ce742ab8deff1ebd99359112670493fdaeeac3
> src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
> src/local/main.cpp a641b9e83862743890597a2981a9419517e7c589
> src/log/tool/benchmark.cpp 01e55115f35d155efbea190b5308b294ba76e7cb
> src/log/tool/initialize.cpp ccda7fb1c0f7113f865ec61adee76b2ea6180442
> src/log/tool/read.cpp d14138502f5bc9a725deb83da505765865da017f
> src/log/tool/replica.cpp 3985fc7df4f7153ae623589fbdd769ccbae57125
> src/master/main.cpp d5666bc8ee8d7a0f0b8685f76d65dd1f9ac2a280
> src/slave/containerizer/isolators/network/port_mapping.hpp c72fb47f60f40cda8d84a10497b9133f83cf018e
> src/slave/containerizer/isolators/network/port_mapping.cpp 49e983edab598e2ac487bb488fdd12840a9e7dfc
> src/slave/main.cpp f762f5b06be74c391cbc336b2da28f8358952ba4
> src/tests/main.cpp e3fff5d60c0468c0d258f2bb301efc1309c071b0
> src/usage/main.cpp 97f55e938dc7678f8331970d8953d09218f70902
>
> Diff: https://reviews.apache.org/r/34195/diff/
>
>
> Testing
> -------
>
> make check
>
> **NOTE** this fixes completely the chained changes from 34193 and makes all the tests pass.
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 34195: Refactoring to use FlagsBase common
functionality
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34195/#review85671
-----------------------------------------------------------
Patch looks great!
Reviews applied: [34193, 34195]
All tests passed.
- Mesos ReviewBot
On May 29, 2015, 1:10 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34195/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 1:10 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Bugs: MESOS-2711
> https://issues.apache.org/jira/browse/MESOS-2711
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2711
>
> All the main() methods have been refactored to use the
> definition of FlagsBase::help flag and FlagsBase::usage().
>
> This CL also tries to bring some uniformity to the
> use of exit codes: if this is deemed to be worth
> making it uniform, we can come up with common
> rules and extend the changes here to be compliant.
>
> This touches a lot of files, but keep scrolling, and you will see a pattern emerge.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
> src/cli/mesos.cpp 171a707cd2ba2348898e7fbe8fe9f0634edd6d86
> src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b
> src/docker/executor.cpp 075c6b5b787532bb1a85018b966aa7e2a1add4f5
> src/examples/load_generator_framework.cpp be1a3bf5f16bd811cb4039c8f15478183712a426
> src/examples/low_level_scheduler_libprocess.cpp bee2e7ef8432cc42733260c668f1100c68f73b8d
> src/examples/low_level_scheduler_pthread.cpp fb8cd66c2e94270971184c1e3dcc92eccab8b223
> src/examples/persistent_volume_framework.cpp 8a893fcfa3d5d988a88fdeaf0bfc08e0a49b7a65
> src/health-check/main.cpp a4ce742ab8deff1ebd99359112670493fdaeeac3
> src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
> src/local/main.cpp a641b9e83862743890597a2981a9419517e7c589
> src/log/tool/benchmark.hpp e0109e20d354a6545ab10750e5d2c3e800f7a6a8
> src/log/tool/benchmark.cpp 01e55115f35d155efbea190b5308b294ba76e7cb
> src/log/tool/initialize.hpp 10ac269627052d89e1936e891127486d88c74253
> src/log/tool/initialize.cpp ccda7fb1c0f7113f865ec61adee76b2ea6180442
> src/log/tool/read.hpp 9a6971bca7efc1ddd45efaa44733c1a474804da0
> src/log/tool/read.cpp d14138502f5bc9a725deb83da505765865da017f
> src/log/tool/replica.cpp 3985fc7df4f7153ae623589fbdd769ccbae57125
> src/master/main.cpp d5666bc8ee8d7a0f0b8685f76d65dd1f9ac2a280
> src/slave/containerizer/isolators/network/port_mapping.hpp 6579286b779882bec493a8e0f10486adc316dc6e
> src/slave/containerizer/isolators/network/port_mapping.cpp 843e52d6f9923d0ee0a0297cd5c464b8b72f5de3
> src/slave/main.cpp f762f5b06be74c391cbc336b2da28f8358952ba4
> src/tests/main.cpp e3fff5d60c0468c0d258f2bb301efc1309c071b0
> src/usage/main.cpp 97f55e938dc7678f8331970d8953d09218f70902
>
> Diff: https://reviews.apache.org/r/34195/diff/
>
>
> Testing
> -------
>
> make check
>
> **NOTE** this fixes completely the chained changes from 34193 and makes all the tests pass.
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 34195: Refactoring to use FlagsBase common
functionality
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34195/#review86116
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Hindman
On May 29, 2015, 1:10 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34195/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 1:10 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Bugs: MESOS-2711
> https://issues.apache.org/jira/browse/MESOS-2711
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2711
>
> All the main() methods have been refactored to use the
> definition of FlagsBase::help flag and FlagsBase::usage().
>
> This CL also tries to bring some uniformity to the
> use of exit codes: if this is deemed to be worth
> making it uniform, we can come up with common
> rules and extend the changes here to be compliant.
>
> This touches a lot of files, but keep scrolling, and you will see a pattern emerge.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
> src/cli/mesos.cpp 171a707cd2ba2348898e7fbe8fe9f0634edd6d86
> src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b
> src/docker/executor.cpp 075c6b5b787532bb1a85018b966aa7e2a1add4f5
> src/examples/load_generator_framework.cpp be1a3bf5f16bd811cb4039c8f15478183712a426
> src/examples/low_level_scheduler_libprocess.cpp bee2e7ef8432cc42733260c668f1100c68f73b8d
> src/examples/low_level_scheduler_pthread.cpp fb8cd66c2e94270971184c1e3dcc92eccab8b223
> src/examples/persistent_volume_framework.cpp 8a893fcfa3d5d988a88fdeaf0bfc08e0a49b7a65
> src/health-check/main.cpp a4ce742ab8deff1ebd99359112670493fdaeeac3
> src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
> src/local/main.cpp a641b9e83862743890597a2981a9419517e7c589
> src/log/tool/benchmark.hpp e0109e20d354a6545ab10750e5d2c3e800f7a6a8
> src/log/tool/benchmark.cpp 01e55115f35d155efbea190b5308b294ba76e7cb
> src/log/tool/initialize.hpp 10ac269627052d89e1936e891127486d88c74253
> src/log/tool/initialize.cpp ccda7fb1c0f7113f865ec61adee76b2ea6180442
> src/log/tool/read.hpp 9a6971bca7efc1ddd45efaa44733c1a474804da0
> src/log/tool/read.cpp d14138502f5bc9a725deb83da505765865da017f
> src/log/tool/replica.cpp 3985fc7df4f7153ae623589fbdd769ccbae57125
> src/master/main.cpp d5666bc8ee8d7a0f0b8685f76d65dd1f9ac2a280
> src/slave/containerizer/isolators/network/port_mapping.hpp 6579286b779882bec493a8e0f10486adc316dc6e
> src/slave/containerizer/isolators/network/port_mapping.cpp 843e52d6f9923d0ee0a0297cd5c464b8b72f5de3
> src/slave/main.cpp f762f5b06be74c391cbc336b2da28f8358952ba4
> src/tests/main.cpp e3fff5d60c0468c0d258f2bb301efc1309c071b0
> src/usage/main.cpp 97f55e938dc7678f8331970d8953d09218f70902
>
> Diff: https://reviews.apache.org/r/34195/diff/
>
>
> Testing
> -------
>
> make check
>
> **NOTE** this fixes completely the chained changes from 34193 and makes all the tests pass.
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 34195: Refactoring to use FlagsBase common
functionality
Posted by Ben Mahler <be...@gmail.com>.
> On June 2, 2015, 5:52 p.m., Ben Mahler wrote:
> > I'm seeing the following pattern:
> >
> > ```
> > if (flags.master.isNone()) {
> > cerr << flags.usage("Missing required option --master") << endl;
> > return EXIT_FAILURE;
> > }
> > ```
> >
> > Why not leverage EXIT here?
> >
> > ```
> > if (flags.master.isNone()) {
> > EXIT(EXIT_FAILURE) << flags.usage("Missing required option --master");
> > }
> > ```
> >
> > I'm assuming there's probably also a ticket or TODO somewhere to be able to declare when an argument is required to avoid these manual checks entirely. :)
Specifically, it seems like EXIT_FAILURE/EXIT_SUCCESS are documented as arguments for exit(), so returning these from main() seems a little less intuitive than passing them into EXIT to me.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34195/#review86258
-----------------------------------------------------------
On May 29, 2015, 1:10 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34195/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 1:10 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Bugs: MESOS-2711
> https://issues.apache.org/jira/browse/MESOS-2711
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2711
>
> All the main() methods have been refactored to use the
> definition of FlagsBase::help flag and FlagsBase::usage().
>
> This CL also tries to bring some uniformity to the
> use of exit codes: if this is deemed to be worth
> making it uniform, we can come up with common
> rules and extend the changes here to be compliant.
>
> This touches a lot of files, but keep scrolling, and you will see a pattern emerge.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
> src/cli/mesos.cpp 171a707cd2ba2348898e7fbe8fe9f0634edd6d86
> src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b
> src/docker/executor.cpp 075c6b5b787532bb1a85018b966aa7e2a1add4f5
> src/examples/load_generator_framework.cpp be1a3bf5f16bd811cb4039c8f15478183712a426
> src/examples/low_level_scheduler_libprocess.cpp bee2e7ef8432cc42733260c668f1100c68f73b8d
> src/examples/low_level_scheduler_pthread.cpp fb8cd66c2e94270971184c1e3dcc92eccab8b223
> src/examples/persistent_volume_framework.cpp 8a893fcfa3d5d988a88fdeaf0bfc08e0a49b7a65
> src/health-check/main.cpp a4ce742ab8deff1ebd99359112670493fdaeeac3
> src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
> src/local/main.cpp a641b9e83862743890597a2981a9419517e7c589
> src/log/tool/benchmark.hpp e0109e20d354a6545ab10750e5d2c3e800f7a6a8
> src/log/tool/benchmark.cpp 01e55115f35d155efbea190b5308b294ba76e7cb
> src/log/tool/initialize.hpp 10ac269627052d89e1936e891127486d88c74253
> src/log/tool/initialize.cpp ccda7fb1c0f7113f865ec61adee76b2ea6180442
> src/log/tool/read.hpp 9a6971bca7efc1ddd45efaa44733c1a474804da0
> src/log/tool/read.cpp d14138502f5bc9a725deb83da505765865da017f
> src/log/tool/replica.cpp 3985fc7df4f7153ae623589fbdd769ccbae57125
> src/master/main.cpp d5666bc8ee8d7a0f0b8685f76d65dd1f9ac2a280
> src/slave/containerizer/isolators/network/port_mapping.hpp 6579286b779882bec493a8e0f10486adc316dc6e
> src/slave/containerizer/isolators/network/port_mapping.cpp 843e52d6f9923d0ee0a0297cd5c464b8b72f5de3
> src/slave/main.cpp f762f5b06be74c391cbc336b2da28f8358952ba4
> src/tests/main.cpp e3fff5d60c0468c0d258f2bb301efc1309c071b0
> src/usage/main.cpp 97f55e938dc7678f8331970d8953d09218f70902
>
> Diff: https://reviews.apache.org/r/34195/diff/
>
>
> Testing
> -------
>
> make check
>
> **NOTE** this fixes completely the chained changes from 34193 and makes all the tests pass.
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 34195: Refactoring to use FlagsBase common
functionality
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On June 2, 2015, 5:52 p.m., Ben Mahler wrote:
> > I'm seeing the following pattern:
> >
> > ```
> > if (flags.master.isNone()) {
> > cerr << flags.usage("Missing required option --master") << endl;
> > return EXIT_FAILURE;
> > }
> > ```
> >
> > Why not leverage EXIT here?
> >
> > ```
> > if (flags.master.isNone()) {
> > EXIT(EXIT_FAILURE) << flags.usage("Missing required option --master");
> > }
> > ```
> >
> > I'm assuming there's probably also a ticket or TODO somewhere to be able to declare when an argument is required to avoid these manual checks entirely. :)
>
> Ben Mahler wrote:
> Specifically, it seems like EXIT_FAILURE/EXIT_SUCCESS are documented as arguments for exit(), so returning these from main() seems a little less intuitive than passing them into EXIT to me.
Yes, please see: [MESOS-2766](https://issues.apache.org/jira/browse/MESOS-2766)
Regarding the different behavior, I tried to upset the least possible existing code.
I completely agree with you, we should (possibly in the style guide? elsewhere?) formalize:
1. the use of `EXIT_{FAILURE,SUCCESS}` as return codes from `main()`
2. the use of `EXIT(code)` (or `return code`) as the expected behavior
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34195/#review86258
-----------------------------------------------------------
On May 29, 2015, 1:10 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34195/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 1:10 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Bugs: MESOS-2711
> https://issues.apache.org/jira/browse/MESOS-2711
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2711
>
> All the main() methods have been refactored to use the
> definition of FlagsBase::help flag and FlagsBase::usage().
>
> This CL also tries to bring some uniformity to the
> use of exit codes: if this is deemed to be worth
> making it uniform, we can come up with common
> rules and extend the changes here to be compliant.
>
> This touches a lot of files, but keep scrolling, and you will see a pattern emerge.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
> src/cli/mesos.cpp 171a707cd2ba2348898e7fbe8fe9f0634edd6d86
> src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b
> src/docker/executor.cpp 075c6b5b787532bb1a85018b966aa7e2a1add4f5
> src/examples/load_generator_framework.cpp be1a3bf5f16bd811cb4039c8f15478183712a426
> src/examples/low_level_scheduler_libprocess.cpp bee2e7ef8432cc42733260c668f1100c68f73b8d
> src/examples/low_level_scheduler_pthread.cpp fb8cd66c2e94270971184c1e3dcc92eccab8b223
> src/examples/persistent_volume_framework.cpp 8a893fcfa3d5d988a88fdeaf0bfc08e0a49b7a65
> src/health-check/main.cpp a4ce742ab8deff1ebd99359112670493fdaeeac3
> src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
> src/local/main.cpp a641b9e83862743890597a2981a9419517e7c589
> src/log/tool/benchmark.hpp e0109e20d354a6545ab10750e5d2c3e800f7a6a8
> src/log/tool/benchmark.cpp 01e55115f35d155efbea190b5308b294ba76e7cb
> src/log/tool/initialize.hpp 10ac269627052d89e1936e891127486d88c74253
> src/log/tool/initialize.cpp ccda7fb1c0f7113f865ec61adee76b2ea6180442
> src/log/tool/read.hpp 9a6971bca7efc1ddd45efaa44733c1a474804da0
> src/log/tool/read.cpp d14138502f5bc9a725deb83da505765865da017f
> src/log/tool/replica.cpp 3985fc7df4f7153ae623589fbdd769ccbae57125
> src/master/main.cpp d5666bc8ee8d7a0f0b8685f76d65dd1f9ac2a280
> src/slave/containerizer/isolators/network/port_mapping.hpp 6579286b779882bec493a8e0f10486adc316dc6e
> src/slave/containerizer/isolators/network/port_mapping.cpp 843e52d6f9923d0ee0a0297cd5c464b8b72f5de3
> src/slave/main.cpp f762f5b06be74c391cbc336b2da28f8358952ba4
> src/tests/main.cpp e3fff5d60c0468c0d258f2bb301efc1309c071b0
> src/usage/main.cpp 97f55e938dc7678f8331970d8953d09218f70902
>
> Diff: https://reviews.apache.org/r/34195/diff/
>
>
> Testing
> -------
>
> make check
>
> **NOTE** this fixes completely the chained changes from 34193 and makes all the tests pass.
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 34195: Refactoring to use FlagsBase common
functionality
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34195/#review86258
-----------------------------------------------------------
I'm seeing the following pattern:
```
if (flags.master.isNone()) {
cerr << flags.usage("Missing required option --master") << endl;
return EXIT_FAILURE;
}
```
Why not leverage EXIT here?
```
if (flags.master.isNone()) {
EXIT(EXIT_FAILURE) << flags.usage("Missing required option --master");
}
```
I'm assuming there's probably also a ticket or TODO somewhere to be able to declare when an argument is required to avoid these manual checks entirely. :)
- Ben Mahler
On May 29, 2015, 1:10 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34195/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 1:10 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Bugs: MESOS-2711
> https://issues.apache.org/jira/browse/MESOS-2711
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2711
>
> All the main() methods have been refactored to use the
> definition of FlagsBase::help flag and FlagsBase::usage().
>
> This CL also tries to bring some uniformity to the
> use of exit codes: if this is deemed to be worth
> making it uniform, we can come up with common
> rules and extend the changes here to be compliant.
>
> This touches a lot of files, but keep scrolling, and you will see a pattern emerge.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
> src/cli/mesos.cpp 171a707cd2ba2348898e7fbe8fe9f0634edd6d86
> src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b
> src/docker/executor.cpp 075c6b5b787532bb1a85018b966aa7e2a1add4f5
> src/examples/load_generator_framework.cpp be1a3bf5f16bd811cb4039c8f15478183712a426
> src/examples/low_level_scheduler_libprocess.cpp bee2e7ef8432cc42733260c668f1100c68f73b8d
> src/examples/low_level_scheduler_pthread.cpp fb8cd66c2e94270971184c1e3dcc92eccab8b223
> src/examples/persistent_volume_framework.cpp 8a893fcfa3d5d988a88fdeaf0bfc08e0a49b7a65
> src/health-check/main.cpp a4ce742ab8deff1ebd99359112670493fdaeeac3
> src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
> src/local/main.cpp a641b9e83862743890597a2981a9419517e7c589
> src/log/tool/benchmark.hpp e0109e20d354a6545ab10750e5d2c3e800f7a6a8
> src/log/tool/benchmark.cpp 01e55115f35d155efbea190b5308b294ba76e7cb
> src/log/tool/initialize.hpp 10ac269627052d89e1936e891127486d88c74253
> src/log/tool/initialize.cpp ccda7fb1c0f7113f865ec61adee76b2ea6180442
> src/log/tool/read.hpp 9a6971bca7efc1ddd45efaa44733c1a474804da0
> src/log/tool/read.cpp d14138502f5bc9a725deb83da505765865da017f
> src/log/tool/replica.cpp 3985fc7df4f7153ae623589fbdd769ccbae57125
> src/master/main.cpp d5666bc8ee8d7a0f0b8685f76d65dd1f9ac2a280
> src/slave/containerizer/isolators/network/port_mapping.hpp 6579286b779882bec493a8e0f10486adc316dc6e
> src/slave/containerizer/isolators/network/port_mapping.cpp 843e52d6f9923d0ee0a0297cd5c464b8b72f5de3
> src/slave/main.cpp f762f5b06be74c391cbc336b2da28f8358952ba4
> src/tests/main.cpp e3fff5d60c0468c0d258f2bb301efc1309c071b0
> src/usage/main.cpp 97f55e938dc7678f8331970d8953d09218f70902
>
> Diff: https://reviews.apache.org/r/34195/diff/
>
>
> Testing
> -------
>
> make check
>
> **NOTE** this fixes completely the chained changes from 34193 and makes all the tests pass.
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 34195: Refactoring to use FlagsBase common
functionality
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34195/
-----------------------------------------------------------
(Updated May 29, 2015, 1:10 a.m.)
Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
Changes
-------
Documented inconsistent return values in the health-check/main.cpp (as a TODO)
Bugs: MESOS-2711
https://issues.apache.org/jira/browse/MESOS-2711
Repository: mesos
Description
-------
Jira: MESOS-2711
All the main() methods have been refactored to use the
definition of FlagsBase::help flag and FlagsBase::usage().
This CL also tries to bring some uniformity to the
use of exit codes: if this is deemed to be worth
making it uniform, we can come up with common
rules and extend the changes here to be compliant.
This touches a lot of files, but keep scrolling, and you will see a pattern emerge.
Diffs (updated)
-----
src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
src/cli/mesos.cpp 171a707cd2ba2348898e7fbe8fe9f0634edd6d86
src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b
src/docker/executor.cpp 075c6b5b787532bb1a85018b966aa7e2a1add4f5
src/examples/load_generator_framework.cpp be1a3bf5f16bd811cb4039c8f15478183712a426
src/examples/low_level_scheduler_libprocess.cpp bee2e7ef8432cc42733260c668f1100c68f73b8d
src/examples/low_level_scheduler_pthread.cpp fb8cd66c2e94270971184c1e3dcc92eccab8b223
src/examples/persistent_volume_framework.cpp 8a893fcfa3d5d988a88fdeaf0bfc08e0a49b7a65
src/health-check/main.cpp a4ce742ab8deff1ebd99359112670493fdaeeac3
src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
src/local/main.cpp a641b9e83862743890597a2981a9419517e7c589
src/log/tool/benchmark.hpp e0109e20d354a6545ab10750e5d2c3e800f7a6a8
src/log/tool/benchmark.cpp 01e55115f35d155efbea190b5308b294ba76e7cb
src/log/tool/initialize.hpp 10ac269627052d89e1936e891127486d88c74253
src/log/tool/initialize.cpp ccda7fb1c0f7113f865ec61adee76b2ea6180442
src/log/tool/read.hpp 9a6971bca7efc1ddd45efaa44733c1a474804da0
src/log/tool/read.cpp d14138502f5bc9a725deb83da505765865da017f
src/log/tool/replica.cpp 3985fc7df4f7153ae623589fbdd769ccbae57125
src/master/main.cpp d5666bc8ee8d7a0f0b8685f76d65dd1f9ac2a280
src/slave/containerizer/isolators/network/port_mapping.hpp 6579286b779882bec493a8e0f10486adc316dc6e
src/slave/containerizer/isolators/network/port_mapping.cpp 843e52d6f9923d0ee0a0297cd5c464b8b72f5de3
src/slave/main.cpp f762f5b06be74c391cbc336b2da28f8358952ba4
src/tests/main.cpp e3fff5d60c0468c0d258f2bb301efc1309c071b0
src/usage/main.cpp 97f55e938dc7678f8331970d8953d09218f70902
Diff: https://reviews.apache.org/r/34195/diff/
Testing
-------
make check
**NOTE** this fixes completely the chained changes from 34193 and makes all the tests pass.
Thanks,
Marco Massenzio
Re: Review Request 34195: Refactoring to use FlagsBase common
functionality
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34195/
-----------------------------------------------------------
(Updated May 29, 2015, 1:03 a.m.)
Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
Changes
-------
Changes from Ben's review
Expanded scope to make all main() exits consistently use EXIT_SUCCESS/EXIT_FAILURE
Bugs: MESOS-2711
https://issues.apache.org/jira/browse/MESOS-2711
Repository: mesos
Description
-------
Jira: MESOS-2711
All the main() methods have been refactored to use the
definition of FlagsBase::help flag and FlagsBase::usage().
This CL also tries to bring some uniformity to the
use of exit codes: if this is deemed to be worth
making it uniform, we can come up with common
rules and extend the changes here to be compliant.
This touches a lot of files, but keep scrolling, and you will see a pattern emerge.
Diffs (updated)
-----
src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
src/cli/mesos.cpp 171a707cd2ba2348898e7fbe8fe9f0634edd6d86
src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b
src/docker/executor.cpp 075c6b5b787532bb1a85018b966aa7e2a1add4f5
src/examples/load_generator_framework.cpp be1a3bf5f16bd811cb4039c8f15478183712a426
src/examples/low_level_scheduler_libprocess.cpp bee2e7ef8432cc42733260c668f1100c68f73b8d
src/examples/low_level_scheduler_pthread.cpp fb8cd66c2e94270971184c1e3dcc92eccab8b223
src/examples/persistent_volume_framework.cpp 8a893fcfa3d5d988a88fdeaf0bfc08e0a49b7a65
src/health-check/main.cpp a4ce742ab8deff1ebd99359112670493fdaeeac3
src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
src/local/main.cpp a641b9e83862743890597a2981a9419517e7c589
src/log/tool/benchmark.hpp e0109e20d354a6545ab10750e5d2c3e800f7a6a8
src/log/tool/benchmark.cpp 01e55115f35d155efbea190b5308b294ba76e7cb
src/log/tool/initialize.hpp 10ac269627052d89e1936e891127486d88c74253
src/log/tool/initialize.cpp ccda7fb1c0f7113f865ec61adee76b2ea6180442
src/log/tool/read.hpp 9a6971bca7efc1ddd45efaa44733c1a474804da0
src/log/tool/read.cpp d14138502f5bc9a725deb83da505765865da017f
src/log/tool/replica.cpp 3985fc7df4f7153ae623589fbdd769ccbae57125
src/master/main.cpp d5666bc8ee8d7a0f0b8685f76d65dd1f9ac2a280
src/slave/containerizer/isolators/network/port_mapping.hpp 6579286b779882bec493a8e0f10486adc316dc6e
src/slave/containerizer/isolators/network/port_mapping.cpp 843e52d6f9923d0ee0a0297cd5c464b8b72f5de3
src/slave/main.cpp f762f5b06be74c391cbc336b2da28f8358952ba4
src/tests/main.cpp e3fff5d60c0468c0d258f2bb301efc1309c071b0
src/usage/main.cpp 97f55e938dc7678f8331970d8953d09218f70902
Diff: https://reviews.apache.org/r/34195/diff/
Testing
-------
make check
**NOTE** this fixes completely the chained changes from 34193 and makes all the tests pass.
Thanks,
Marco Massenzio
Re: Review Request 34195: Refactoring to use FlagsBase common
functionality
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On May 28, 2015, 11:45 a.m., Benjamin Hindman wrote:
> > src/cli/execute.cpp, line 321
> > <https://reviews.apache.org/r/34195/diff/3/?file=972213#file972213line321>
> >
> > I'm not convinced the 'errorMessage' is more help here. You're basically saving doing a 'return EXIT_FAILURE;' in each of these if's, which is not a big deal and arguably more explicit and clear. On the other hand, this code was originally meant to be structured so the line that did a 'flags.master.get()' was as close to the line that checked 'flags.master.isSome()' and by moving that code farther away it makes it so a reader has to look harder to confirm that when someone is "dereferencing" 'flags.master' it's indeed safe.
As it happens, I too was only half-convinced that mine was a real improvement.
However, in the latest change, the `errorMessage` is actually a concatenation of various errors: the benefit is that I don't have to (in theory) launch the darn thing three or four times, each time discovering I missed yet another flag :)
```
$ execute
Missing --master
{darn!}
$ execute --master=localhost
Missing --name
{oh, ffs...}
$ execute --master=localhost --name=my-master
Missing --command
{goddammit!}
$ execute --master=localhost --name=my-master --command=foobar
Could not parse master=localhost
{@#@#!!"£}
```
With this patch, one gets:
```
$ execute
Missing --master=IP:PORT
Missing --name=NAME
Missing --command=COMMAND
...
```
I don't know, I believe in karma... ;)
However, happy to change it back to the original version (with improvements) if you think this is not really worth the extra effort.
> On May 28, 2015, 11:45 a.m., Benjamin Hindman wrote:
> > src/cli/execute.cpp, line 336
> > <https://reviews.apache.org/r/34195/diff/3/?file=972213#file972213line336>
> >
> > This pattern is why we use Option!
I totally get it, trust me!
However, (see comment above) concatenating Option<string>s is a major pain, while here, there is actually the same level of indirection/etc.
> On May 28, 2015, 11:45 a.m., Benjamin Hindman wrote:
> > src/cli/resolve.cpp, line 81
> > <https://reviews.apache.org/r/34195/diff/3/?file=972214#file972214line81>
> >
> > I liked your pattern of doing 'setUsageMessage' immediately after instantiating the class. Any reason not to do that here?
indeed, why not?
> On May 28, 2015, 11:45 a.m., Benjamin Hindman wrote:
> > src/cli/resolve.cpp, line 128
> > <https://reviews.apache.org/r/34195/diff/3/?file=972214#file972214line128>
> >
> > Let's also use this review as an opportunity to do 'EXIT_FAILURE' anyplace we 'return -1' and 'EXIT_SUCCESS' anyplace we 'return 0'. Please do a sweep in every file you've touched and make it so, thanks Marco!
oh, yes! thanks!!!
I really was worried people would yell at me, but I really was itching to do that :)
With pleasure!
> On May 28, 2015, 11:45 a.m., Benjamin Hindman wrote:
> > src/examples/persistent_volume_framework.cpp, line 423
> > <https://reviews.apache.org/r/34195/diff/3/?file=972216#file972216line423>
> >
> > How come this 'usage' isn't getting killed?
I'm not very good with tedious tasks :)
thanks for catching me!
> On May 28, 2015, 11:45 a.m., Benjamin Hindman wrote:
> > src/log/tool/read.cpp, line 73
> > <https://reviews.apache.org/r/34195/diff/3/?file=972222#file972222line73>
> >
> > Please end sentence with period (here and everywhere please).
or, even better, actually fix the TODO!
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34195/#review85536
-----------------------------------------------------------
On May 28, 2015, 10:06 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34195/
> -----------------------------------------------------------
>
> (Updated May 28, 2015, 10:06 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Bugs: MESOS-2711
> https://issues.apache.org/jira/browse/MESOS-2711
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2711
>
> All the main() methods have been refactored to use the
> definition of FlagsBase::help flag and FlagsBase::usage().
>
> This CL also tries to bring some uniformity to the
> use of exit codes: if this is deemed to be worth
> making it uniform, we can come up with common
> rules and extend the changes here to be compliant.
>
> This touches a lot of files, but keep scrolling, and you will see a pattern emerge.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
> src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b
> src/examples/load_generator_framework.cpp be1a3bf5f16bd811cb4039c8f15478183712a426
> src/examples/persistent_volume_framework.cpp 8a893fcfa3d5d988a88fdeaf0bfc08e0a49b7a65
> src/health-check/main.cpp a4ce742ab8deff1ebd99359112670493fdaeeac3
> src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
> src/local/main.cpp a641b9e83862743890597a2981a9419517e7c589
> src/log/tool/benchmark.cpp 01e55115f35d155efbea190b5308b294ba76e7cb
> src/log/tool/initialize.cpp ccda7fb1c0f7113f865ec61adee76b2ea6180442
> src/log/tool/read.cpp d14138502f5bc9a725deb83da505765865da017f
> src/log/tool/replica.cpp 3985fc7df4f7153ae623589fbdd769ccbae57125
> src/master/main.cpp d5666bc8ee8d7a0f0b8685f76d65dd1f9ac2a280
> src/slave/containerizer/isolators/network/port_mapping.hpp c72fb47f60f40cda8d84a10497b9133f83cf018e
> src/slave/containerizer/isolators/network/port_mapping.cpp 49e983edab598e2ac487bb488fdd12840a9e7dfc
> src/slave/main.cpp f762f5b06be74c391cbc336b2da28f8358952ba4
> src/tests/main.cpp e3fff5d60c0468c0d258f2bb301efc1309c071b0
> src/usage/main.cpp 97f55e938dc7678f8331970d8953d09218f70902
>
> Diff: https://reviews.apache.org/r/34195/diff/
>
>
> Testing
> -------
>
> make check
>
> **NOTE** this fixes completely the chained changes from 34193 and makes all the tests pass.
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 34195: Refactoring to use FlagsBase common
functionality
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On May 28, 2015, 11:45 a.m., Benjamin Hindman wrote:
> > src/cli/execute.cpp, line 321
> > <https://reviews.apache.org/r/34195/diff/3/?file=972213#file972213line321>
> >
> > I'm not convinced the 'errorMessage' is more help here. You're basically saving doing a 'return EXIT_FAILURE;' in each of these if's, which is not a big deal and arguably more explicit and clear. On the other hand, this code was originally meant to be structured so the line that did a 'flags.master.get()' was as close to the line that checked 'flags.master.isSome()' and by moving that code farther away it makes it so a reader has to look harder to confirm that when someone is "dereferencing" 'flags.master' it's indeed safe.
>
> Marco Massenzio wrote:
> As it happens, I too was only half-convinced that mine was a real improvement.
>
> However, in the latest change, the `errorMessage` is actually a concatenation of various errors: the benefit is that I don't have to (in theory) launch the darn thing three or four times, each time discovering I missed yet another flag :)
> ```
> $ execute
> Missing --master
> {darn!}
> $ execute --master=localhost
> Missing --name
> {oh, ffs...}
> $ execute --master=localhost --name=my-master
> Missing --command
> {goddammit!}
> $ execute --master=localhost --name=my-master --command=foobar
> Could not parse master=localhost
> {@#@#!!"£}
> ```
>
> With this patch, one gets:
> ```
> $ execute
> Missing --master=IP:PORT
> Missing --name=NAME
> Missing --command=COMMAND
> ...
> ```
> I don't know, I believe in karma... ;)
>
> However, happy to change it back to the original version (with improvements) if you think this is not really worth the extra effort.
Given that (1) there are lots of other instances where we could also do this but don't and (2) there is probably a better way to capture "required" fields let's not do it for now (I'll just pull this out for you though since I'm going to commit now).
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34195/#review85536
-----------------------------------------------------------
On May 29, 2015, 1:10 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34195/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 1:10 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Bugs: MESOS-2711
> https://issues.apache.org/jira/browse/MESOS-2711
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2711
>
> All the main() methods have been refactored to use the
> definition of FlagsBase::help flag and FlagsBase::usage().
>
> This CL also tries to bring some uniformity to the
> use of exit codes: if this is deemed to be worth
> making it uniform, we can come up with common
> rules and extend the changes here to be compliant.
>
> This touches a lot of files, but keep scrolling, and you will see a pattern emerge.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
> src/cli/mesos.cpp 171a707cd2ba2348898e7fbe8fe9f0634edd6d86
> src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b
> src/docker/executor.cpp 075c6b5b787532bb1a85018b966aa7e2a1add4f5
> src/examples/load_generator_framework.cpp be1a3bf5f16bd811cb4039c8f15478183712a426
> src/examples/low_level_scheduler_libprocess.cpp bee2e7ef8432cc42733260c668f1100c68f73b8d
> src/examples/low_level_scheduler_pthread.cpp fb8cd66c2e94270971184c1e3dcc92eccab8b223
> src/examples/persistent_volume_framework.cpp 8a893fcfa3d5d988a88fdeaf0bfc08e0a49b7a65
> src/health-check/main.cpp a4ce742ab8deff1ebd99359112670493fdaeeac3
> src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
> src/local/main.cpp a641b9e83862743890597a2981a9419517e7c589
> src/log/tool/benchmark.hpp e0109e20d354a6545ab10750e5d2c3e800f7a6a8
> src/log/tool/benchmark.cpp 01e55115f35d155efbea190b5308b294ba76e7cb
> src/log/tool/initialize.hpp 10ac269627052d89e1936e891127486d88c74253
> src/log/tool/initialize.cpp ccda7fb1c0f7113f865ec61adee76b2ea6180442
> src/log/tool/read.hpp 9a6971bca7efc1ddd45efaa44733c1a474804da0
> src/log/tool/read.cpp d14138502f5bc9a725deb83da505765865da017f
> src/log/tool/replica.cpp 3985fc7df4f7153ae623589fbdd769ccbae57125
> src/master/main.cpp d5666bc8ee8d7a0f0b8685f76d65dd1f9ac2a280
> src/slave/containerizer/isolators/network/port_mapping.hpp 6579286b779882bec493a8e0f10486adc316dc6e
> src/slave/containerizer/isolators/network/port_mapping.cpp 843e52d6f9923d0ee0a0297cd5c464b8b72f5de3
> src/slave/main.cpp f762f5b06be74c391cbc336b2da28f8358952ba4
> src/tests/main.cpp e3fff5d60c0468c0d258f2bb301efc1309c071b0
> src/usage/main.cpp 97f55e938dc7678f8331970d8953d09218f70902
>
> Diff: https://reviews.apache.org/r/34195/diff/
>
>
> Testing
> -------
>
> make check
>
> **NOTE** this fixes completely the chained changes from 34193 and makes all the tests pass.
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 34195: Refactoring to use FlagsBase common
functionality
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34195/#review85536
-----------------------------------------------------------
src/cli/execute.cpp
<https://reviews.apache.org/r/34195/#comment137114>
Let's just change this everywhere to EXIT_SUCCESS but without the comment.
src/cli/execute.cpp
<https://reviews.apache.org/r/34195/#comment137115>
s/errMessage/errorMessage/
src/cli/execute.cpp
<https://reviews.apache.org/r/34195/#comment137118>
I'm not convinced the 'errorMessage' is more help here. You're basically saving doing a 'return EXIT_FAILURE;' in each of these if's, which is not a big deal and arguably more explicit and clear. On the other hand, this code was originally meant to be structured so the line that did a 'flags.master.get()' was as close to the line that checked 'flags.master.isSome()' and by moving that code farther away it makes it so a reader has to look harder to confirm that when someone is "dereferencing" 'flags.master' it's indeed safe.
src/cli/execute.cpp
<https://reviews.apache.org/r/34195/#comment137116>
This pattern is why we use Option!
src/cli/resolve.cpp
<https://reviews.apache.org/r/34195/#comment137120>
s/argv/argv./
src/cli/resolve.cpp
<https://reviews.apache.org/r/34195/#comment137119>
I liked your pattern of doing 'setUsageMessage' immediately after instantiating the class. Any reason not to do that here?
src/cli/resolve.cpp
<https://reviews.apache.org/r/34195/#comment137122>
Let's also use this review as an opportunity to do 'EXIT_FAILURE' anyplace we 'return -1' and 'EXIT_SUCCESS' anyplace we 'return 0'. Please do a sweep in every file you've touched and make it so, thanks Marco!
src/examples/load_generator_framework.cpp
<https://reviews.apache.org/r/34195/#comment137123>
Another request: let's replace 'exit(1)' with 'return EXIT_FAILURE' here and everywhere else. Likewise 'exit(0)' with 'return EXIT_SUCCESS'.
src/examples/load_generator_framework.cpp
<https://reviews.apache.org/r/34195/#comment137124>
I see that you changed some of these 'EXIT()' calls below to be:
cerr << flags.usage(message) << endl;
return EXIT_FAILURE;
But not all of them have been changed, like these ones. I'd like to make things consistent now even if we weren't consistent in the past.
src/examples/persistent_volume_framework.cpp
<https://reviews.apache.org/r/34195/#comment137125>
How come this 'usage' isn't getting killed?
src/examples/persistent_volume_framework.cpp
<https://reviews.apache.org/r/34195/#comment137126>
Here's another example of an EXIT() that we can change. Note that I don't so much mind:
EXIT(EXIT_FAILURE) << flags.usage(load.error());
But let's just be consistent please.
src/launcher/executor.cpp
<https://reviews.apache.org/r/34195/#comment137127>
s/[ERROR]//
src/local/main.cpp
<https://reviews.apache.org/r/34195/#comment137106>
To capture what we were printing before, this should be:
"Usage: " + os::basename(argv[0]).get() << " [...]\n\n" +
"Launches an in-memory cluster within a single process.";
src/log/tool/benchmark.cpp
<https://reviews.apache.org/r/34195/#comment137128>
Why are we not killing this 'usage'?
src/log/tool/initialize.cpp
<https://reviews.apache.org/r/34195/#comment137129>
Why are we not killing this 'usage'?
src/log/tool/read.cpp
<https://reviews.apache.org/r/34195/#comment137130>
Please end sentence with period (here and everywhere please).
src/usage/main.cpp
<https://reviews.apache.org/r/34195/#comment137132>
s/ERROR: //
- Benjamin Hindman
On May 28, 2015, 10:06 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34195/
> -----------------------------------------------------------
>
> (Updated May 28, 2015, 10:06 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Bugs: MESOS-2711
> https://issues.apache.org/jira/browse/MESOS-2711
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2711
>
> All the main() methods have been refactored to use the
> definition of FlagsBase::help flag and FlagsBase::usage().
>
> This CL also tries to bring some uniformity to the
> use of exit codes: if this is deemed to be worth
> making it uniform, we can come up with common
> rules and extend the changes here to be compliant.
>
> This touches a lot of files, but keep scrolling, and you will see a pattern emerge.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
> src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b
> src/examples/load_generator_framework.cpp be1a3bf5f16bd811cb4039c8f15478183712a426
> src/examples/persistent_volume_framework.cpp 8a893fcfa3d5d988a88fdeaf0bfc08e0a49b7a65
> src/health-check/main.cpp a4ce742ab8deff1ebd99359112670493fdaeeac3
> src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
> src/local/main.cpp a641b9e83862743890597a2981a9419517e7c589
> src/log/tool/benchmark.cpp 01e55115f35d155efbea190b5308b294ba76e7cb
> src/log/tool/initialize.cpp ccda7fb1c0f7113f865ec61adee76b2ea6180442
> src/log/tool/read.cpp d14138502f5bc9a725deb83da505765865da017f
> src/log/tool/replica.cpp 3985fc7df4f7153ae623589fbdd769ccbae57125
> src/master/main.cpp d5666bc8ee8d7a0f0b8685f76d65dd1f9ac2a280
> src/slave/containerizer/isolators/network/port_mapping.hpp c72fb47f60f40cda8d84a10497b9133f83cf018e
> src/slave/containerizer/isolators/network/port_mapping.cpp 49e983edab598e2ac487bb488fdd12840a9e7dfc
> src/slave/main.cpp f762f5b06be74c391cbc336b2da28f8358952ba4
> src/tests/main.cpp e3fff5d60c0468c0d258f2bb301efc1309c071b0
> src/usage/main.cpp 97f55e938dc7678f8331970d8953d09218f70902
>
> Diff: https://reviews.apache.org/r/34195/diff/
>
>
> Testing
> -------
>
> make check
>
> **NOTE** this fixes completely the chained changes from 34193 and makes all the tests pass.
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 34195: Refactoring to use FlagsBase common
functionality
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34195/
-----------------------------------------------------------
(Updated May 28, 2015, 10:06 a.m.)
Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
Changes
-------
s/BaseFlags/FlagsBase/ ;-)
Summary (updated)
-----------------
Refactoring to use FlagsBase common functionality
Bugs: MESOS-2711
https://issues.apache.org/jira/browse/MESOS-2711
Repository: mesos
Description
-------
Jira: MESOS-2711
All the main() methods have been refactored to use the
definition of FlagsBase::help flag and FlagsBase::usage().
This CL also tries to bring some uniformity to the
use of exit codes: if this is deemed to be worth
making it uniform, we can come up with common
rules and extend the changes here to be compliant.
This touches a lot of files, but keep scrolling, and you will see a pattern emerge.
Diffs
-----
src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b
src/examples/load_generator_framework.cpp be1a3bf5f16bd811cb4039c8f15478183712a426
src/examples/persistent_volume_framework.cpp 8a893fcfa3d5d988a88fdeaf0bfc08e0a49b7a65
src/health-check/main.cpp a4ce742ab8deff1ebd99359112670493fdaeeac3
src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
src/local/main.cpp a641b9e83862743890597a2981a9419517e7c589
src/log/tool/benchmark.cpp 01e55115f35d155efbea190b5308b294ba76e7cb
src/log/tool/initialize.cpp ccda7fb1c0f7113f865ec61adee76b2ea6180442
src/log/tool/read.cpp d14138502f5bc9a725deb83da505765865da017f
src/log/tool/replica.cpp 3985fc7df4f7153ae623589fbdd769ccbae57125
src/master/main.cpp d5666bc8ee8d7a0f0b8685f76d65dd1f9ac2a280
src/slave/containerizer/isolators/network/port_mapping.hpp c72fb47f60f40cda8d84a10497b9133f83cf018e
src/slave/containerizer/isolators/network/port_mapping.cpp 49e983edab598e2ac487bb488fdd12840a9e7dfc
src/slave/main.cpp f762f5b06be74c391cbc336b2da28f8358952ba4
src/tests/main.cpp e3fff5d60c0468c0d258f2bb301efc1309c071b0
src/usage/main.cpp 97f55e938dc7678f8331970d8953d09218f70902
Diff: https://reviews.apache.org/r/34195/diff/
Testing
-------
make check
**NOTE** this fixes completely the chained changes from 34193 and makes all the tests pass.
Thanks,
Marco Massenzio
Re: Review Request 34195: Refactoring to use BaseFlags common
functionality
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34195/
-----------------------------------------------------------
(Updated May 26, 2015, 8:58 p.m.)
Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
Changes
-------
Updated to be compliant with changes introduced after review comments in r/34193
Bugs: MESOS-2711
https://issues.apache.org/jira/browse/MESOS-2711
Repository: mesos
Description (updated)
-------
Jira: MESOS-2711
All the main() methods have been refactored to use the
definition of FlagsBase::help flag and FlagsBase::usage().
This CL also tries to bring some uniformity to the
use of exit codes: if this is deemed to be worth
making it uniform, we can come up with common
rules and extend the changes here to be compliant.
This touches a lot of files, but keep scrolling, and you will see a pattern emerge.
Diffs (updated)
-----
src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b
src/examples/load_generator_framework.cpp be1a3bf5f16bd811cb4039c8f15478183712a426
src/examples/persistent_volume_framework.cpp 8a893fcfa3d5d988a88fdeaf0bfc08e0a49b7a65
src/health-check/main.cpp a4ce742ab8deff1ebd99359112670493fdaeeac3
src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
src/local/main.cpp a641b9e83862743890597a2981a9419517e7c589
src/log/tool/benchmark.cpp 01e55115f35d155efbea190b5308b294ba76e7cb
src/log/tool/initialize.cpp ccda7fb1c0f7113f865ec61adee76b2ea6180442
src/log/tool/read.cpp d14138502f5bc9a725deb83da505765865da017f
src/log/tool/replica.cpp 3985fc7df4f7153ae623589fbdd769ccbae57125
src/master/main.cpp d5666bc8ee8d7a0f0b8685f76d65dd1f9ac2a280
src/slave/containerizer/isolators/network/port_mapping.hpp c72fb47f60f40cda8d84a10497b9133f83cf018e
src/slave/containerizer/isolators/network/port_mapping.cpp 49e983edab598e2ac487bb488fdd12840a9e7dfc
src/slave/main.cpp f762f5b06be74c391cbc336b2da28f8358952ba4
src/tests/main.cpp e3fff5d60c0468c0d258f2bb301efc1309c071b0
src/usage/main.cpp 97f55e938dc7678f8331970d8953d09218f70902
Diff: https://reviews.apache.org/r/34195/diff/
Testing
-------
make check
**NOTE** this fixes completely the chained changes from 34193 and makes all the tests pass.
Thanks,
Marco Massenzio
Re: Review Request 34195: Refactoring to use BaseFlags common
functionality
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34195/
-----------------------------------------------------------
(Updated May 18, 2015, 7:01 p.m.)
Review request for Benjamin Hindman and Joris Van Remoortere.
Bugs: MESOS-2711
https://issues.apache.org/jira/browse/MESOS-2711
Repository: mesos
Description
-------
Jira: MESOS-2711
All the main() methods have been refactored to use the
definition of BaseFlags::help flag and BaseFlags::printUsage().
This CL also tries to bring some uniformity to the
use of exit codes: if this is deemed to be worth
making it uniform, we can come up with common
rules and extend the changes here to be compliant.
This touches a lot of files, but keep scrolling, and you will see a pattern emerge ;-)
TODO(marco): we should also abstract away the error checking
Diffs
-----
src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b
src/examples/load_generator_framework.cpp be1a3bf5f16bd811cb4039c8f15478183712a426
src/examples/persistent_volume_framework.cpp 8a893fcfa3d5d988a88fdeaf0bfc08e0a49b7a65
src/health-check/main.cpp a4ce742ab8deff1ebd99359112670493fdaeeac3
src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
src/local/main.cpp a641b9e83862743890597a2981a9419517e7c589
src/log/tool/benchmark.cpp 01e55115f35d155efbea190b5308b294ba76e7cb
src/log/tool/initialize.cpp ccda7fb1c0f7113f865ec61adee76b2ea6180442
src/log/tool/read.cpp d14138502f5bc9a725deb83da505765865da017f
src/log/tool/replica.cpp 3985fc7df4f7153ae623589fbdd769ccbae57125
src/master/main.cpp d5666bc8ee8d7a0f0b8685f76d65dd1f9ac2a280
src/slave/containerizer/isolators/network/port_mapping.hpp c72fb47f60f40cda8d84a10497b9133f83cf018e
src/slave/containerizer/isolators/network/port_mapping.cpp a4abaff30bb4646b1b1edfdbbc243c9e3f6851df
src/slave/main.cpp f762f5b06be74c391cbc336b2da28f8358952ba4
src/tests/main.cpp e3fff5d60c0468c0d258f2bb301efc1309c071b0
src/usage/main.cpp 97f55e938dc7678f8331970d8953d09218f70902
Diff: https://reviews.apache.org/r/34195/diff/
Testing (updated)
-------
make check
**NOTE** this fixes completely the chained changes from 34193 and makes all the tests pass.
Thanks,
Marco Massenzio