You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2016/04/25 07:44:40 UTC
Review Request 46621: Add alias support for flags.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46621/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Bugs: MESOS-5271
https://issues.apache.org/jira/browse/MESOS-5271
Repository: mesos
Description
-------
Add alias support for flags.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 2502d9e8a515b9adc1b3aa2f719e5710a0e7ef29
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp c3cbcdb781e1c282d381de1ad2bf4f386ee1db21
Diff: https://reviews.apache.org/r/46621/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 46621: Add alias support for flags.
Posted by Vinod Kone <vi...@gmail.com>.
> On April 25, 2016, 2:48 p.m., Benjamin Bannier wrote:
> > Did a quick review since I spent some time on working on a fix for MESOS-3335. It would be great if we wouldn't aggreviate that problem further.
> >
> > What seems unclear to me ATM is how alias'ed flags can override each other, or what the preferred order would be.
There is no override or preference order. Only one of name/alias is allowed. Otherwise we throw an error.
> On April 25, 2016, 2:48 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp, line 64
> > <https://reviews.apache.org/r/46621/diff/1/?file=1358750#file1358750line64>
> >
> > Does it makes sense to support multiple aliases?
that was how the original patch was in my branch. but reverted it because i didn't have any use cases to use multiple aliases. we can add it later if we need it.
> On April 25, 2016, 2:48 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp, line 81
> > <https://reviews.apache.org/r/46621/diff/1/?file=1358750#file1358750line81>
> >
> > We could return a const ref here.
just as an optmization? don't think we do that in our code base.
> On April 25, 2016, 2:48 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, lines 152-171
> > <https://reviews.apache.org/r/46621/diff/1/?file=1358751#file1358751line152>
> >
> > These suffer from the problem described in MESOS-3335.
yes. but IIUC this issue is not introduced here>? i would rather we fix it in one swoop when we fix MESOS-3335.
> On April 25, 2016, 2:48 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, lines 192-200
> > <https://reviews.apache.org/r/46621/diff/1/?file=1358751#file1358751line192>
> >
> > This also suffers from the problem described in MESOS-3335.
see above.
> On April 25, 2016, 2:48 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, lines 212-216
> > <https://reviews.apache.org/r/46621/diff/1/?file=1358751#file1358751line212>
> >
> > This also suffers from the problem described in MESOS-3335.
see above.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46621/#review130421
-----------------------------------------------------------
On April 25, 2016, 5:44 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46621/
> -----------------------------------------------------------
>
> (Updated April 25, 2016, 5:44 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-5271
> https://issues.apache.org/jira/browse/MESOS-5271
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add alias support for flags.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 2502d9e8a515b9adc1b3aa2f719e5710a0e7ef29
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp c3cbcdb781e1c282d381de1ad2bf4f386ee1db21
>
> Diff: https://reviews.apache.org/r/46621/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 46621: Add alias support for flags.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46621/#review130421
-----------------------------------------------------------
Did a quick review since I spent some time on working on a fix for MESOS-3335. It would be great if we wouldn't aggreviate that problem further.
What seems unclear to me ATM is how alias'ed flags can override each other, or what the preferred order would be.
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp (line 64)
<https://reviews.apache.org/r/46621/#comment194170>
Does it makes sense to support multiple aliases?
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp (line 81)
<https://reviews.apache.org/r/46621/#comment194162>
We could return a const ref here.
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (lines 152 - 171)
<https://reviews.apache.org/r/46621/#comment194163>
These suffer from the problem described in MESOS-3335.
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (lines 192 - 200)
<https://reviews.apache.org/r/46621/#comment194164>
This also suffers from the problem described in MESOS-3335.
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (lines 212 - 216)
<https://reviews.apache.org/r/46621/#comment194165>
This also suffers from the problem described in MESOS-3335.
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (lines 232 - 250)
<https://reviews.apache.org/r/46621/#comment194167>
These signatures do not in general suffer from the problem of MESOS-3335 since they use a member pointer instead of a pointer to (at some point) random memory.
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (lines 272 - 273)
<https://reviews.apache.org/r/46621/#comment194168>
Signature also safe re:MESOS-3335.
- Benjamin Bannier
On April 25, 2016, 7:44 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46621/
> -----------------------------------------------------------
>
> (Updated April 25, 2016, 7:44 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-5271
> https://issues.apache.org/jira/browse/MESOS-5271
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add alias support for flags.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 2502d9e8a515b9adc1b3aa2f719e5710a0e7ef29
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp c3cbcdb781e1c282d381de1ad2bf4f386ee1db21
>
> Diff: https://reviews.apache.org/r/46621/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 46621: Added alias support for flags.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46621/#review131245
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 841)
<https://reviews.apache.org/r/46621/#comment195145>
Why the raw pointer here?
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 880)
<https://reviews.apache.org/r/46621/#comment195144>
Backticks around `loaded_name`?
- Greg Mann
On April 30, 2016, 3:43 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46621/
> -----------------------------------------------------------
>
> (Updated April 30, 2016, 3:43 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-5271
> https://issues.apache.org/jira/browse/MESOS-5271
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This lets a user to load a flag either by the original name or an
> alias.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 2502d9e8a515b9adc1b3aa2f719e5710a0e7ef29
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp c3cbcdb781e1c282d381de1ad2bf4f386ee1db21
>
> Diff: https://reviews.apache.org/r/46621/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 46621: Added alias support for flags.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46621/
-----------------------------------------------------------
(Updated April 30, 2016, 3:43 a.m.)
Review request for mesos and Ben Mahler.
Changes
-------
fixed blank line.
Bugs: MESOS-5271
https://issues.apache.org/jira/browse/MESOS-5271
Repository: mesos
Description
-------
This lets a user to load a flag either by the original name or an
alias.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 2502d9e8a515b9adc1b3aa2f719e5710a0e7ef29
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp c3cbcdb781e1c282d381de1ad2bf4f386ee1db21
Diff: https://reviews.apache.org/r/46621/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 46621: Added alias support for flags.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46621/#review131240
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 498)
<https://reviews.apache.org/r/46621/#comment195143>
I'm getting the following error when I attempt to apply this patch with 'apply-review.sh': `3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp:498: Redundant blank line at the start of a code block should be deleted. [whitespace/blank_line] [2]`
- Greg Mann
On April 30, 2016, 2 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46621/
> -----------------------------------------------------------
>
> (Updated April 30, 2016, 2 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-5271
> https://issues.apache.org/jira/browse/MESOS-5271
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This lets a user to load a flag either by the original name or an
> alias.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 2502d9e8a515b9adc1b3aa2f719e5710a0e7ef29
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp c3cbcdb781e1c282d381de1ad2bf4f386ee1db21
>
> Diff: https://reviews.apache.org/r/46621/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 46621: Added alias support for flags.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46621/
-----------------------------------------------------------
(Updated April 30, 2016, 2 a.m.)
Review request for mesos and Ben Mahler.
Changes
-------
mpark's comments.
Bugs: MESOS-5271
https://issues.apache.org/jira/browse/MESOS-5271
Repository: mesos
Description (updated)
-------
This lets a user to load a flag either by the original name or an
alias.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 2502d9e8a515b9adc1b3aa2f719e5710a0e7ef29
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp c3cbcdb781e1c282d381de1ad2bf4f386ee1db21
Diff: https://reviews.apache.org/r/46621/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 46621: Added alias support for flags.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46621/
-----------------------------------------------------------
(Updated April 29, 2016, 3:26 a.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Ben's comments.
Bugs: MESOS-5271
https://issues.apache.org/jira/browse/MESOS-5271
Repository: mesos
Description
-------
Added alias support for flags.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 2502d9e8a515b9adc1b3aa2f719e5710a0e7ef29
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp c3cbcdb781e1c282d381de1ad2bf4f386ee1db21
Diff: https://reviews.apache.org/r/46621/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 46621: Added alias support for flags.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46621/#review130677
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp (line 80)
<https://reviews.apache.org/r/46621/#comment194524>
don't mention master/agent or HTTP here. make it more generic.
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp (line 87)
<https://reviews.apache.org/r/46621/#comment194525>
move this to usage() and make it a lambda instead.
- Vinod Kone
On April 26, 2016, 1:04 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46621/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 1:04 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-5271
> https://issues.apache.org/jira/browse/MESOS-5271
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added alias support for flags.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 2502d9e8a515b9adc1b3aa2f719e5710a0e7ef29
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp c3cbcdb781e1c282d381de1ad2bf4f386ee1db21
>
> Diff: https://reviews.apache.org/r/46621/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 46621: Added alias support for flags.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46621/#review130685
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 386)
<https://reviews.apache.org/r/46621/#comment194539>
use `->` instead of .get(). here and everywhere else.
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 853)
<https://reviews.apache.org/r/46621/#comment194540>
refactor this load (in a separate review) to be more readable.
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 999)
<https://reviews.apache.org/r/46621/#comment194542>
instead of `--{a, b}=VALUE` do `--a=VALUE, --b=VALUE`
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 1007)
<https://reviews.apache.org/r/46621/#comment194543>
change the format of the usage to do it in the next line and change `=VALUE` to `=<value>`
- Vinod Kone
On April 26, 2016, 1:04 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46621/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 1:04 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-5271
> https://issues.apache.org/jira/browse/MESOS-5271
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added alias support for flags.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 2502d9e8a515b9adc1b3aa2f719e5710a0e7ef29
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp c3cbcdb781e1c282d381de1ad2bf4f386ee1db21
>
> Diff: https://reviews.apache.org/r/46621/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 46621: Added alias support for flags.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46621/
-----------------------------------------------------------
(Updated April 26, 2016, 1:04 a.m.)
Review request for mesos and Ben Mahler.
Changes
-------
rebased and updated flag.load() signature to support deprecation later in the review chain.
Summary (updated)
-----------------
Added alias support for flags.
Bugs: MESOS-5271
https://issues.apache.org/jira/browse/MESOS-5271
Repository: mesos
Description (updated)
-------
Added alias support for flags.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 2502d9e8a515b9adc1b3aa2f719e5710a0e7ef29
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp c3cbcdb781e1c282d381de1ad2bf4f386ee1db21
Diff: https://reviews.apache.org/r/46621/diff/
Testing
-------
make check
Thanks,
Vinod Kone