mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request 34195: Refactoring to use FlagsBase common functionality
Date Thu, 28 May 2015 11:45:50 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message