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 Mon, 01 Jun 2015 22:59:20 GMT


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


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