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



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (lines 600 - 
601)
<https://reviews.apache.org/r/38259/#comment155076>

    the `+` should be at the end of the line (before the newline).
    Can you also please change the error message to:
    ```
    "Command line flag '" + name +
    " duplicates environment variable definition with the same name"
    ```
    also, please feel free to use all 80 columns.
    
    Now, an interesting question (for the shepherd/committer to decide) is 
whether we should just emit a WARN and have the command line (explicit) setting 
override the OS Env (implicit).
    
    Also, whether we should only WARN, but proceed, if the **values** are the 
same:
    
    ```MESOS_IP=127.0.0.1
    ... --ip=127.0.0.1
    ```
    could (arguably) be considered a valid setting.
    
    As a general approach, I'd suggest however to stick with current behavior, 
to avoid breaking stuff that is currently working just fine (and relying on the 
executable to error out in case of misconfigurations).



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 605)
<https://reviews.apache.org/r/38259/#comment155077>

    nit: please retain `on` as in the original message.



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (lines 672 - 
681)
<https://reviews.apache.org/r/38259/#comment155078>

    would it be possible to factor out all this into a helper method?
    It's essentialy a copy & paste of the same code above.
    
    so long as it was only a couple of lines of code (maybe) it made sense to 
repeat it - here, it seems redundant.
    
    (and, obviously, same comments apply)



3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp (line 406)
<https://reviews.apache.org/r/38259/#comment155079>

    this would be a good opportunity to replace this test with something that 
relies less on exact string match, maybe?
    
    in pseudo-code, I'd do something like:
    ```
    EXPECT_TRUE(lower(load.error()).contains("duplicate") && 
contains(flag_name))
    ```
    
    (also, if you have a helper method, you can just unit test that one - once 
- for both cases; without having to repeat the test, below)


Thanks for doing this!
A few comments, but generally good.

Not sure if you have a shepherd for this - if not, please let me know and I can 
help find one.

- Marco Massenzio


On Sept. 11, 2015, 5:24 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38259/
> -----------------------------------------------------------
> 
> (Updated Sept. 11, 2015, 5:24 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Marco Massenzio.
> 
> 
> Bugs: MESOS-3340
>     https://issues.apache.org/jira/browse/MESOS-3340
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Currently, it appears that re-defining a flag on the command-line that was 
> already defined via a OS Env var (MESOS_*) causes the Master to fail with a 
> not very helpful message.
> 
> For example, if one has MESOS_QUORUM defined, this happens:
> 
>     $ ./mesos-master --zk=zk://192.168.1.4/mesos --quorum=1 
> --hostname=192.168.1.4 --ip=192.168.1.4
>     Duplicate flag 'quorum' on command line
> 
> which is not very helpful.
> 
> Current solution is to throw error if any duplication; over-write may make 
> user confused about the result.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9da213f 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp ebf8cd6 
> 
> Diff: https://reviews.apache.org/r/38259/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Reply via email to