mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Klaus Ma" <kl...@cguru.net>
Subject Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables
Date Fri, 11 Sep 2015 09:53:20 GMT


> On Sept. 11, 2015, 6:56 a.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 605
> > <https://reviews.apache.org/r/38259/diff/2/?file=1068027#file1068027line605>
> >
> >     nit: please retain `on` as in the original message.

Addressed


> On Sept. 11, 2015, 6:56 a.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, lines 600-601
> > <https://reviews.apache.org/r/38259/diff/2/?file=1068027#file1068027line600>
> >
> >     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).

I used @haosdent's suggestion as follow, it looks better than mine :).

    return Error(
        "Duplicate flag '" + name "' in command line with environment");

Your question is interesting, and I think it's a great improvement. Agree with you to keep
current behavior, we can log a new ticket to trace that question.


> On Sept. 11, 2015, 6:56 a.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, lines 672-681
> > <https://reviews.apache.org/r/38259/diff/2/?file=1068027#file1068027line672>
> >
> >     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)

I'm thinking to define a const massage format for re-usage; but not sure where to add such
a const format. For example:

    const string dup_err_msg("Duplicate flag '%s' in command line with environment"); // No
sure where to put this const string :(.
    ...
    return Error(strings::format(dup_err_msg, name));
    
And re-check those two log/function, it seems we can re-use some logic: in `FlagsBase::load(...
const char* const* argv ...)`, dump `argv` and call `FlagsBase::load(... char*** argv ...)`
to load the flags, the args that are not a flag.


On Sept. 11, 2015, 6:56 a.m., Klaus Ma wrote:
> > 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.

No shepherd now; that'll be great if you or Bernd can find a shepherd for this one.


- Klaus


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


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


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