You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Kevin Klues <kl...@gmail.com> on 2016/12/07 17:06:18 UTC
Review Request 54486: Added default values for all required
IOSwitchboardServerFlags.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54486/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier and Jie Yu.
Bugs: MESOS-6726
https://issues.apache.org/jira/browse/MESOS-6726
Repository: mesos
Description
-------
Added default values for all required IOSwitchboardServerFlags.
Diffs
-----
src/slave/containerizer/mesos/io/switchboard.hpp f691b182d4029a15bbb3b1c158b176d43f265cc1
src/slave/containerizer/mesos/io/switchboard_main.cpp 4a9ed22ca63f0aa55ebbf7b85404a1b168a8e34a
Diff: https://reviews.apache.org/r/54486/diff/
Testing
-------
GTEST_FILTER="" make -j check
GTEST_FILTER="*IOSwitchboard*" src/mesos-tests
Thanks,
Kevin Klues
Re: Review Request 54486: Added default values for all required
IOSwitchboardServerFlags.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54486/#review158349
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On Dec. 7, 2016, 5:06 p.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54486/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2016, 5:06 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-6726
> https://issues.apache.org/jira/browse/MESOS-6726
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added default values for all required IOSwitchboardServerFlags.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.hpp f691b182d4029a15bbb3b1c158b176d43f265cc1
> src/slave/containerizer/mesos/io/switchboard_main.cpp 4a9ed22ca63f0aa55ebbf7b85404a1b168a8e34a
>
> Diff: https://reviews.apache.org/r/54486/diff/
>
>
> Testing
> -------
>
> GTEST_FILTER="" make -j check
> GTEST_FILTER="*IOSwitchboard*" src/mesos-tests
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 54486: Added default values for all required
IOSwitchboardServerFlags.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54486/#review158409
-----------------------------------------------------------
Bad patch!
Reviews applied: [54486]
Failed command: ./support/apply-review.sh -n -r 54486
Error:
2016-12-07 21:54:38 URL:https://reviews.apache.org/r/54486/diff/raw/ [3415/3415] -> "54486.patch" [1]
error: patch failed: src/slave/containerizer/mesos/io/switchboard.hpp:181
error: src/slave/containerizer/mesos/io/switchboard.hpp: patch does not apply
error: patch failed: src/slave/containerizer/mesos/io/switchboard_main.cpp:67
error: src/slave/containerizer/mesos/io/switchboard_main.cpp: patch does not apply
Full log: https://builds.apache.org/job/mesos-reviewbot/16365/console
- Mesos ReviewBot
On Dec. 7, 2016, 5:06 p.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54486/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2016, 5:06 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-6726
> https://issues.apache.org/jira/browse/MESOS-6726
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added default values for all required IOSwitchboardServerFlags.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.hpp f691b182d4029a15bbb3b1c158b176d43f265cc1
> src/slave/containerizer/mesos/io/switchboard_main.cpp 4a9ed22ca63f0aa55ebbf7b85404a1b168a8e34a
>
> Diff: https://reviews.apache.org/r/54486/diff/
>
>
> Testing
> -------
>
> GTEST_FILTER="" make -j check
> GTEST_FILTER="*IOSwitchboard*" src/mesos-tests
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 54486: Added default values for all required
IOSwitchboardServerFlags.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 8, 2016, 11:13 a.m., Benjamin Bannier wrote:
> > src/slave/containerizer/mesos/io/switchboard_main.cpp, lines 71-76
> > <https://reviews.apache.org/r/54486/diff/1/?file=1578815#file1578815line71>
> >
> > Using sentinel values in the parameter domain as default values seems wrong to me. If you want to detect explicitly given flags you should just use an `Option<T>` for the parameter. This verification then becomes the semantically clearer
> >
> > if (flags.stdin_to_fd.isNone() ||
> > ...
> >
> > With that no default value in the `add()` calls is required.
> >
> > You should probably also adjust the error message to something like `Missing requireed parameters` and output `flags.usage()` after that.
>
> Jie Yu wrote:
> Sounds good.
>
> Benjamin Bannier wrote:
> @jieyu: I see this was committed without addressing any of these issues (or at least leaving a comment), and also without updating the RR. Is there any plan to fix these?
Yes. I'll fix those. thanks!
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54486/#review158513
-----------------------------------------------------------
On Dec. 7, 2016, 5:06 p.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54486/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2016, 5:06 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-6726
> https://issues.apache.org/jira/browse/MESOS-6726
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added default values for all required IOSwitchboardServerFlags.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.hpp f691b182d4029a15bbb3b1c158b176d43f265cc1
> src/slave/containerizer/mesos/io/switchboard_main.cpp 4a9ed22ca63f0aa55ebbf7b85404a1b168a8e34a
>
> Diff: https://reviews.apache.org/r/54486/diff/
>
>
> Testing
> -------
>
> GTEST_FILTER="" make -j check
> GTEST_FILTER="*IOSwitchboard*" src/mesos-tests
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 54486: Added default values for all required
IOSwitchboardServerFlags.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 8, 2016, 11:13 a.m., Benjamin Bannier wrote:
> > src/slave/containerizer/mesos/io/switchboard_main.cpp, lines 71-76
> > <https://reviews.apache.org/r/54486/diff/1/?file=1578815#file1578815line71>
> >
> > Using sentinel values in the parameter domain as default values seems wrong to me. If you want to detect explicitly given flags you should just use an `Option<T>` for the parameter. This verification then becomes the semantically clearer
> >
> > if (flags.stdin_to_fd.isNone() ||
> > ...
> >
> > With that no default value in the `add()` calls is required.
> >
> > You should probably also adjust the error message to something like `Missing requireed parameters` and output `flags.usage()` after that.
Sounds good.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54486/#review158513
-----------------------------------------------------------
On Dec. 7, 2016, 5:06 p.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54486/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2016, 5:06 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-6726
> https://issues.apache.org/jira/browse/MESOS-6726
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added default values for all required IOSwitchboardServerFlags.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.hpp f691b182d4029a15bbb3b1c158b176d43f265cc1
> src/slave/containerizer/mesos/io/switchboard_main.cpp 4a9ed22ca63f0aa55ebbf7b85404a1b168a8e34a
>
> Diff: https://reviews.apache.org/r/54486/diff/
>
>
> Testing
> -------
>
> GTEST_FILTER="" make -j check
> GTEST_FILTER="*IOSwitchboard*" src/mesos-tests
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 54486: Added default values for all required
IOSwitchboardServerFlags.
Posted by Kevin Klues <kl...@gmail.com>.
> On Dec. 8, 2016, 11:13 a.m., Benjamin Bannier wrote:
> > src/slave/containerizer/mesos/io/switchboard_main.cpp, lines 71-76
> > <https://reviews.apache.org/r/54486/diff/1/?file=1578815#file1578815line71>
> >
> > Using sentinel values in the parameter domain as default values seems wrong to me. If you want to detect explicitly given flags you should just use an `Option<T>` for the parameter. This verification then becomes the semantically clearer
> >
> > if (flags.stdin_to_fd.isNone() ||
> > ...
> >
> > With that no default value in the `add()` calls is required.
> >
> > You should probably also adjust the error message to something like `Missing requireed parameters` and output `flags.usage()` after that.
>
> Jie Yu wrote:
> Sounds good.
>
> Benjamin Bannier wrote:
> @jieyu: I see this was committed without addressing any of these issues (or at least leaving a comment), and also without updating the RR. Is there any plan to fix these?
>
> Jie Yu wrote:
> Yes. I'll fix those. thanks!
Should this be closed now?
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54486/#review158513
-----------------------------------------------------------
On Dec. 7, 2016, 5:06 p.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54486/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2016, 5:06 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-6726
> https://issues.apache.org/jira/browse/MESOS-6726
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added default values for all required IOSwitchboardServerFlags.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.hpp f691b182d4029a15bbb3b1c158b176d43f265cc1
> src/slave/containerizer/mesos/io/switchboard_main.cpp 4a9ed22ca63f0aa55ebbf7b85404a1b168a8e34a
>
> Diff: https://reviews.apache.org/r/54486/diff/
>
>
> Testing
> -------
>
> GTEST_FILTER="" make -j check
> GTEST_FILTER="*IOSwitchboard*" src/mesos-tests
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 54486: Added default values for all required
IOSwitchboardServerFlags.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Dec. 8, 2016, 12:13 p.m., Benjamin Bannier wrote:
> > src/slave/containerizer/mesos/io/switchboard_main.cpp, lines 71-76
> > <https://reviews.apache.org/r/54486/diff/1/?file=1578815#file1578815line71>
> >
> > Using sentinel values in the parameter domain as default values seems wrong to me. If you want to detect explicitly given flags you should just use an `Option<T>` for the parameter. This verification then becomes the semantically clearer
> >
> > if (flags.stdin_to_fd.isNone() ||
> > ...
> >
> > With that no default value in the `add()` calls is required.
> >
> > You should probably also adjust the error message to something like `Missing requireed parameters` and output `flags.usage()` after that.
>
> Jie Yu wrote:
> Sounds good.
@jieyu: I see this was committed without addressing any of these issues (or at least leaving a comment), and also without updating the RR. Is there any plan to fix these?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54486/#review158513
-----------------------------------------------------------
On Dec. 7, 2016, 6:06 p.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54486/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2016, 6:06 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-6726
> https://issues.apache.org/jira/browse/MESOS-6726
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added default values for all required IOSwitchboardServerFlags.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.hpp f691b182d4029a15bbb3b1c158b176d43f265cc1
> src/slave/containerizer/mesos/io/switchboard_main.cpp 4a9ed22ca63f0aa55ebbf7b85404a1b168a8e34a
>
> Diff: https://reviews.apache.org/r/54486/diff/
>
>
> Testing
> -------
>
> GTEST_FILTER="" make -j check
> GTEST_FILTER="*IOSwitchboard*" src/mesos-tests
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 54486: Added default values for all required
IOSwitchboardServerFlags.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54486/#review158513
-----------------------------------------------------------
src/slave/containerizer/mesos/io/switchboard_main.cpp (lines 71 - 76)
<https://reviews.apache.org/r/54486/#comment229281>
Using sentinel values in the parameter domain as default values seems wrong to me. If you want to detect explicitly given flags you should just use an `Option<T>` for the parameter. This verification then becomes the semantically clearer
if (flags.stdin_to_fd.isNone() ||
...
With that no default value in the `add()` calls is required.
You should probably also adjust the error message to something like `Missing requireed parameters` and output `flags.usage()` after that.
src/slave/containerizer/mesos/io/switchboard_main.cpp (line 79)
<https://reviews.apache.org/r/54486/#comment229282>
Unrelated to this patch, but this executable should probably also output usage information if invoked with `--help` (sadly this isn't handled on the `flags::FlagsBase` level). The canonical form is
if (flags.help) {
cerr << flags.usage();
return EXIT_SUCCESS;
}
- Benjamin Bannier
On Dec. 7, 2016, 6:06 p.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54486/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2016, 6:06 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-6726
> https://issues.apache.org/jira/browse/MESOS-6726
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added default values for all required IOSwitchboardServerFlags.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.hpp f691b182d4029a15bbb3b1c158b176d43f265cc1
> src/slave/containerizer/mesos/io/switchboard_main.cpp 4a9ed22ca63f0aa55ebbf7b85404a1b168a8e34a
>
> Diff: https://reviews.apache.org/r/54486/diff/
>
>
> Testing
> -------
>
> GTEST_FILTER="" make -j check
> GTEST_FILTER="*IOSwitchboard*" src/mesos-tests
>
>
> Thanks,
>
> Kevin Klues
>
>