Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-18 Thread Klaus Ma 

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

(Updated Sept. 19, 2015, 12:51 a.m.)


Review request for mesos, Bernd Mathiske, Marco Massenzio, and Michael Park.


Changes
---

Address comments


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 (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9da213f 
  3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 43c09f3 

Diff: https://reviews.apache.org/r/38259/diff/


Testing
---

make
make check


Thanks,

Klaus Ma

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-18 Thread Klaus Ma 


> On Sept. 18, 2015, 8:03 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, lines 
> > 240-241
> > 
> >
> > How about we remove this, inline the string literal at the 2 callsites 
> > below for now, and we'll remove the duplication when we do the refactor of 
> > the very-similar `load` function definitions?

Addresses; and MESOS-3454 is also logged to trace duplicated code in 
Flags::load.


> On Sept. 18, 2015, 8:03 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, lines 426-427
> > 
> >
> > Let's not test for specific error strings. Simply checking for the 
> > error is good enough for now. Once we get to introducing typed errors, 
> > we'll be able to capture it better within the type system.

Addressed; MESOS-3469 is logged to trace "typed error system", add `TODO` to 
highlight the plan.


- Klaus


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


On Sept. 13, 2015, 5:30 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38259/
> ---
> 
> (Updated Sept. 13, 2015, 5:30 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Marco Massenzio, and Michael Park.
> 
> 
> 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
> 
>

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-18 Thread Michael Park 

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

Ship it!



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (lines 240 - 
241)


How about we remove this, inline the string literal at the 2 callsites 
below for now, and we'll remove the duplication when we do the refactor of the 
very-similar `load` function definitions?



3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp (lines 426 - 427)


Let's not test for specific error strings. Simply checking for the error is 
good enough for now. Once we get to introducing typed errors, we'll be able to 
capture it better within the type system.


- Michael Park


On Sept. 13, 2015, 5:30 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38259/
> ---
> 
> (Updated Sept. 13, 2015, 5:30 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Marco Massenzio, and Michael Park.
> 
> 
> 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
> 
>

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-18 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [38259]

All tests passed.

- Mesos ReviewBot


On Sept. 19, 2015, 12:51 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38259/
> ---
> 
> (Updated Sept. 19, 2015, 12:51 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Marco Massenzio, and Michael Park.
> 
> 
> 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 43c09f3 
> 
> Diff: https://reviews.apache.org/r/38259/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-14 Thread Marco Massenzio 

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


LGTM
Can you please add a test with a few vars defined in Env and then in Cmd Line 
too, and verify that it "works as intended"?

Thanks for bearing with me and changing your code.
I think it looks clean and nice now :)

- Marco Massenzio


On Sept. 13, 2015, 5:30 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38259/
> ---
> 
> (Updated Sept. 13, 2015, 5:30 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Marco Massenzio, and Michael Park.
> 
> 
> 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
> 
>

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-14 Thread Klaus Ma 


> On Sept. 15, 2015, 4:54 a.m., Marco Massenzio wrote:
> > LGTM
> > Can you please add a test with a few vars defined in Env and then in Cmd 
> > Line too, and verify that it "works as intended"?
> > 
> > Thanks for bearing with me and changing your code.
> > I think it looks clean and nice now :)

There used to be a test case on "DuplicatesFromEnvironment", I updated it 
according to our design behavior: overwrite env by cli.


- Klaus


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


On Sept. 13, 2015, 5:30 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38259/
> ---
> 
> (Updated Sept. 13, 2015, 5:30 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Marco Massenzio, and Michael Park.
> 
> 
> 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
> 
>

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-13 Thread Klaus Ma 


> On Sept. 11, 2015, 6:56 a.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, lines 
> > 600-601
> > 
> >
> > 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).
> 
> Klaus Ma wrote:
> 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.

The behavior was updated to over-write environment by command line.


- Klaus


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


On Sept. 13, 2015, 5:30 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38259/
> ---
> 
> (Updated Sept. 13, 2015, 5:30 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Marco Massenzio, and Michael Park.
> 
> 
> 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
> 
>

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-12 Thread Klaus Ma 

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

(Updated Sept. 13, 2015, 4:34 a.m.)


Review request for mesos, Bernd Mathiske, Marco Massenzio, and Michael Park.


Changes
---

Update the code to overwrite env by cli


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 (updated)
-

  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

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-11 Thread Marco Massenzio 

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


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)


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



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (lines 672 - 
681)


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)


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

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-11 Thread Marco Massenzio 


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.
> 
> Klaus Ma wrote:
> No shepherd now; that'll be great if you or Bernd can find a shepherd for 
> this one.

ah - just noticed, it's @bernd -- all good, but please be aware he's away next 
week, he may take some time before he gets round to this.


- Marco


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


On Sept. 11, 2015, 9:54 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, 9:54 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
> 
>

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-11 Thread Marco Massenzio 


> On Sept. 11, 2015, 6:56 a.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, lines 
> > 672-681
> > 
> >
> > 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)
> 
> Klaus Ma wrote:
> 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.

there is a `master/constants.hpp` but it does not seem to be used for this - in 
fact, all error messages/logs are hard-coded strings (I don't guess anyone ever 
worried about internationalizing Mesos :D ) - I think your only option would be 
to put it at the head of this file.

please make sure to use ALL_CAPS for the const name.
Also, please make sure to modify the message with the one I suggested, thanks.


- Marco


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


On Sept. 11, 2015, 9:54 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, 9:54 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
> 
>

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-11 Thread Klaus Ma 

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

(Updated Sept. 11, 2015, 9:54 a.m.)


Review request for mesos, Bernd Mathiske and Marco Massenzio.


Changes
---

Address comments and propose a solution on duplicated logic


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 (updated)
-

  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

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-11 Thread Klaus Ma 


> On Sept. 11, 2015, 6:56 a.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 605
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> 
>

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-11 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [38259]

All tests passed.

- Mesos ReviewBot


On Sept. 11, 2015, 9:54 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, 9:54 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
> 
>

Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-10 Thread Klaus Ma 

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

Review request for mesos and Bernd Mathiske.


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 
9da213f802aec6a7768ce6f5aea7b437d980356a 
  3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
ebf8cd656625b7fd414cacaa87f156c95df29438 

Diff: https://reviews.apache.org/r/38259/diff/


Testing
---

make
make check


Thanks,

Klaus Ma

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-10 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [38259]

All tests passed.

- Mesos ReviewBot


On Sept. 10, 2015, 3:47 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38259/
> ---
> 
> (Updated Sept. 10, 2015, 3:47 p.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> 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 
> 9da213f802aec6a7768ce6f5aea7b437d980356a 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> ebf8cd656625b7fd414cacaa87f156c95df29438 
> 
> Diff: https://reviews.apache.org/r/38259/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-10 Thread haosdent huang 


> On Sept. 10, 2015, 4:40 p.m., haosdent huang wrote:
> > LGTM. But I not sure if this match @marco expect. Could you add he as 
> > reviewers? So currently your approach is show error message when 
> > `duplicates` is true. Use command line parameters when command line 
> > variables conflict with environment variables. I also suggest add comment 
> > and test cases about the last condition.

```
Use command line parameters when command line variables conflict with 
environment variables.
```
->
```
When duplicates is false, use command line parameters while ignore environment 
variables.
```


- haosdent


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


On Sept. 10, 2015, 3:47 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38259/
> ---
> 
> (Updated Sept. 10, 2015, 3:47 p.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> 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 
> 9da213f802aec6a7768ce6f5aea7b437d980356a 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> ebf8cd656625b7fd414cacaa87f156c95df29438 
> 
> Diff: https://reviews.apache.org/r/38259/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-10 Thread haosdent huang 

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


LGTM. But I not sure if this match @marco expect. Could you add he as 
reviewers? So currently your approach is show error message when `duplicates` 
is true. Use command line parameters when command line variables conflict with 
environment variables. I also suggest add comment and test cases about the last 
condition.


3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 569)


Is it possible to change to foreach here?



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 600)


The style is not correct here. Please change to
```
return Error(
"Duplicate flag '" + name "' in command line with environment");
```



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 675)


Ditto


- haosdent huang


On Sept. 10, 2015, 3:47 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38259/
> ---
> 
> (Updated Sept. 10, 2015, 3:47 p.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> 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 
> 9da213f802aec6a7768ce6f5aea7b437d980356a 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> ebf8cd656625b7fd414cacaa87f156c95df29438 
> 
> Diff: https://reviews.apache.org/r/38259/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-10 Thread Klaus Ma 


> On Sept. 10, 2015, 4:40 p.m., haosdent huang wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 569
> > 
> >
> > Is it possible to change to foreach here?
> 
> Klaus Ma wrote:
> OK, let me address it.

Just checked the code, we can not do that because 1) it's an array, 2) no 
requirement that argv is ended with NULL.


- Klaus


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


On Sept. 11, 2015, 2:43 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, 2:43 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 
> 9da213f802aec6a7768ce6f5aea7b437d980356a 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> ebf8cd656625b7fd414cacaa87f156c95df29438 
> 
> Diff: https://reviews.apache.org/r/38259/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-10 Thread Klaus Ma 

---
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 (updated)
-

  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

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-10 Thread Klaus Ma 


> On Sept. 10, 2015, 4:40 p.m., haosdent huang wrote:
> > LGTM. But I not sure if this match @marco expect. Could you add he as 
> > reviewers? So currently your approach is show error message when 
> > `duplicates` is true. Use command line parameters when command line 
> > variables conflict with environment variables. I also suggest add comment 
> > and test cases about the last condition.
> 
> haosdent huang wrote:
> ```
> Use command line parameters when command line variables conflict with 
> environment variables.
> ```
> ->
> ```
> When duplicates is false, use command line parameters while ignore 
> environment variables.
> ```

Add Macro as reviewer. Current solution is to throw error message and abort the 
command line if any duplicated items, so uer'll not be confused by overwrite.


> On Sept. 10, 2015, 4:40 p.m., haosdent huang wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 600
> > 
> >
> > The style is not correct here. Please change to
> > ```
> > return Error(
> > "Duplicate flag '" + name "' in command line with 
> > environment");
> > ```

Sure, I'll correct it :).


> On Sept. 10, 2015, 4:40 p.m., haosdent huang wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 569
> > 
> >
> > Is it possible to change to foreach here?

OK, let me address it.


- Klaus


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


On Sept. 11, 2015, 2:43 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, 2:43 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 
> 9da213f802aec6a7768ce6f5aea7b437d980356a 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> ebf8cd656625b7fd414cacaa87f156c95df29438 
> 
> Diff: https://reviews.apache.org/r/38259/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>