You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2016/12/02 06:42:57 UTC
Review Request 54293: Supported TTY in I/O switchboard.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54293/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Kevin Klues.
Repository: mesos
Description
-------
Supported TTY in I/O switchboard.
Diffs
-----
include/mesos/slave/containerizer.proto 33b4c231cc88a048f3de8ab4b2e0e9eb2140132d
src/slave/containerizer/mesos/containerizer.cpp a7e2665d4c64b7b322d4ae8d5441e8e777236c59
src/slave/containerizer/mesos/io/switchboard.hpp 23c66bb2b05310a12fec24a5e5eebddd370ba0f0
src/slave/containerizer/mesos/io/switchboard.cpp 594c37e531c5b26599989a126aede56954d460fa
src/slave/containerizer/mesos/io/switchboard_main.cpp 5800217d3562a53218d966c57a14e8b768643c34
src/slave/containerizer/mesos/launch.cpp d78ca4df694712e60a15f1795878653eb5e0414e
src/tests/containerizer/io_switchboard_tests.cpp d82f22b5c9a8d177577db3aeaae9e571e7e2fb80
Diff: https://reviews.apache.org/r/54293/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 54293: Supported TTY in I/O switchboard.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54293/#review157959
-----------------------------------------------------------
Ship it!
Ship It!
- Kevin Klues
On Dec. 4, 2016, 9:21 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54293/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2016, 9:21 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Kevin Klues.
>
>
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Supported TTY in I/O switchboard.
>
>
> Diffs
> -----
>
> include/mesos/slave/containerizer.proto 33b4c231cc88a048f3de8ab4b2e0e9eb2140132d
> src/slave/containerizer/mesos/containerizer.cpp a7e2665d4c64b7b322d4ae8d5441e8e777236c59
> src/slave/containerizer/mesos/io/switchboard.hpp 23c66bb2b05310a12fec24a5e5eebddd370ba0f0
> src/slave/containerizer/mesos/io/switchboard.cpp 778367a268ec350ed438bc9fe9d359d63bdb5503
> src/slave/containerizer/mesos/io/switchboard_main.cpp 5800217d3562a53218d966c57a14e8b768643c34
> src/slave/containerizer/mesos/launch.cpp d78ca4df694712e60a15f1795878653eb5e0414e
> src/tests/containerizer/io_switchboard_tests.cpp d82f22b5c9a8d177577db3aeaae9e571e7e2fb80
>
> Diff: https://reviews.apache.org/r/54293/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 54293: Supported TTY in I/O switchboard.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54293/
-----------------------------------------------------------
(Updated Dec. 5, 2016, 5:37 a.m.)
Review request for mesos, Benjamin Hindman and Kevin Klues.
Changes
-------
Rebased. Addressed comments.
Bugs: MESOS-6470
https://issues.apache.org/jira/browse/MESOS-6470
Repository: mesos
Description
-------
Supported TTY in I/O switchboard.
Diffs (updated)
-----
include/mesos/slave/containerizer.proto 33b4c231cc88a048f3de8ab4b2e0e9eb2140132d
src/slave/containerizer/mesos/containerizer.cpp a7e2665d4c64b7b322d4ae8d5441e8e777236c59
src/slave/containerizer/mesos/io/switchboard.hpp 23c66bb2b05310a12fec24a5e5eebddd370ba0f0
src/slave/containerizer/mesos/io/switchboard.cpp 778367a268ec350ed438bc9fe9d359d63bdb5503
src/slave/containerizer/mesos/io/switchboard_main.cpp 5800217d3562a53218d966c57a14e8b768643c34
src/slave/containerizer/mesos/launch.cpp d78ca4df694712e60a15f1795878653eb5e0414e
src/tests/containerizer/io_switchboard_tests.cpp d82f22b5c9a8d177577db3aeaae9e571e7e2fb80
Diff: https://reviews.apache.org/r/54293/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 54293: Supported TTY in I/O switchboard.
Posted by Kevin Klues <kl...@gmail.com>.
> On Dec. 5, 2016, 2:02 a.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, line 248
> > <https://reviews.apache.org/r/54293/diff/2/?file=1576040#file1576040line248>
> >
> > Can you add a comment here about the difference between the two hashsets? Reading top to bottom, it's not clear exactly what each one represents just by looking at the name of the variable.
Maybe it's just enough to rename the second one to `childFds`.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54293/#review157937
-----------------------------------------------------------
On Dec. 4, 2016, 9:21 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54293/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2016, 9:21 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Kevin Klues.
>
>
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Supported TTY in I/O switchboard.
>
>
> Diffs
> -----
>
> include/mesos/slave/containerizer.proto 33b4c231cc88a048f3de8ab4b2e0e9eb2140132d
> src/slave/containerizer/mesos/containerizer.cpp a7e2665d4c64b7b322d4ae8d5441e8e777236c59
> src/slave/containerizer/mesos/io/switchboard.hpp 23c66bb2b05310a12fec24a5e5eebddd370ba0f0
> src/slave/containerizer/mesos/io/switchboard.cpp 778367a268ec350ed438bc9fe9d359d63bdb5503
> src/slave/containerizer/mesos/io/switchboard_main.cpp 5800217d3562a53218d966c57a14e8b768643c34
> src/slave/containerizer/mesos/launch.cpp d78ca4df694712e60a15f1795878653eb5e0414e
> src/tests/containerizer/io_switchboard_tests.cpp d82f22b5c9a8d177577db3aeaae9e571e7e2fb80
>
> Diff: https://reviews.apache.org/r/54293/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 54293: Supported TTY in I/O switchboard.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 5, 2016, 2:02 a.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, line 248
> > <https://reviews.apache.org/r/54293/diff/2/?file=1576040#file1576040line248>
> >
> > Can you add a comment here about the difference between the two hashsets? Reading top to bottom, it's not clear exactly what each one represents just by looking at the name of the variable.
>
> Kevin Klues wrote:
> Maybe it's just enough to rename the second one to `childFds`.
I added a comment to explain this.
> On Dec. 5, 2016, 2:02 a.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, line 279
> > <https://reviews.apache.org/r/54293/diff/2/?file=1576040#file1576040line279>
> >
> > Does it make sense to eventually move this function into stout?
yeah, I'll add a TODO. We should add a stout/posix/tty.hpp.
> On Dec. 5, 2016, 2:02 a.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/io/switchboard.hpp, line 126
> > <https://reviews.apache.org/r/54293/diff/2/?file=1576039#file1576039line126>
> >
> > It's somewhat arbitrary, but can we make this the second to last parameter, in order to group the booleans together? If so, can we do it throughout?
Let me do that in a separate patch.
> On Dec. 5, 2016, 2:02 a.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, lines 392-397
> > <https://reviews.apache.org/r/54293/diff/2/?file=1576040#file1576040line392>
> >
> > I guess you don't need to set the other flags anymore because you rely on their default values now?
Default is not STDOUT_FILENO and STDERR_FILENO. I'll add a comment here.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54293/#review157937
-----------------------------------------------------------
On Dec. 4, 2016, 9:21 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54293/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2016, 9:21 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Kevin Klues.
>
>
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Supported TTY in I/O switchboard.
>
>
> Diffs
> -----
>
> include/mesos/slave/containerizer.proto 33b4c231cc88a048f3de8ab4b2e0e9eb2140132d
> src/slave/containerizer/mesos/containerizer.cpp a7e2665d4c64b7b322d4ae8d5441e8e777236c59
> src/slave/containerizer/mesos/io/switchboard.hpp 23c66bb2b05310a12fec24a5e5eebddd370ba0f0
> src/slave/containerizer/mesos/io/switchboard.cpp 778367a268ec350ed438bc9fe9d359d63bdb5503
> src/slave/containerizer/mesos/io/switchboard_main.cpp 5800217d3562a53218d966c57a14e8b768643c34
> src/slave/containerizer/mesos/launch.cpp d78ca4df694712e60a15f1795878653eb5e0414e
> src/tests/containerizer/io_switchboard_tests.cpp d82f22b5c9a8d177577db3aeaae9e571e7e2fb80
>
> Diff: https://reviews.apache.org/r/54293/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 54293: Supported TTY in I/O switchboard.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54293/#review157937
-----------------------------------------------------------
src/slave/containerizer/mesos/io/switchboard.hpp (line 126)
<https://reviews.apache.org/r/54293/#comment228606>
It's somewhat arbitrary, but can we make this the second to last parameter, in order to group the booleans together? If so, can we do it throughout?
src/slave/containerizer/mesos/io/switchboard.hpp (line 178)
<https://reviews.apache.org/r/54293/#comment228607>
s/termial/terminal
src/slave/containerizer/mesos/io/switchboard.cpp (line 247)
<https://reviews.apache.org/r/54293/#comment228608>
Can you add a comment here about the difference between the two hashsets? Reading top to bottom, it's not clear exactly what each one represents just by looking at the name of the variable.
src/slave/containerizer/mesos/io/switchboard.cpp (line 258)
<https://reviews.apache.org/r/54293/#comment228609>
s/the pseudo/a pseudo
src/slave/containerizer/mesos/io/switchboard.cpp (line 277)
<https://reviews.apache.org/r/54293/#comment228610>
Does it make sense to eventually move this function into stout?
src/slave/containerizer/mesos/io/switchboard.cpp (line 283)
<https://reviews.apache.org/r/54293/#comment228611>
Does it make sense to eventually move this function into stout?
src/slave/containerizer/mesos/io/switchboard.cpp (lines 388 - 391)
<https://reviews.apache.org/r/54293/#comment228612>
I guess you don't need to set the other flags anymore because you rely on their default values now?
src/slave/containerizer/mesos/io/switchboard.cpp (lines 439 - 441)
<https://reviews.apache.org/r/54293/#comment228613>
Can you add a comment about why we do this? When I first looked at it, it wasn't obvious why it was necessary, but I see why it's needed later on (i.e. for future failures).
src/slave/containerizer/mesos/io/switchboard.cpp (lines 749 - 750)
<https://reviews.apache.org/r/54293/#comment228614>
Can you add a quick comment about why we don't need to redirect to `stderr` if we have a tty set up.
- Kevin Klues
On Dec. 4, 2016, 9:21 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54293/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2016, 9:21 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Kevin Klues.
>
>
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Supported TTY in I/O switchboard.
>
>
> Diffs
> -----
>
> include/mesos/slave/containerizer.proto 33b4c231cc88a048f3de8ab4b2e0e9eb2140132d
> src/slave/containerizer/mesos/containerizer.cpp a7e2665d4c64b7b322d4ae8d5441e8e777236c59
> src/slave/containerizer/mesos/io/switchboard.hpp 23c66bb2b05310a12fec24a5e5eebddd370ba0f0
> src/slave/containerizer/mesos/io/switchboard.cpp 778367a268ec350ed438bc9fe9d359d63bdb5503
> src/slave/containerizer/mesos/io/switchboard_main.cpp 5800217d3562a53218d966c57a14e8b768643c34
> src/slave/containerizer/mesos/launch.cpp d78ca4df694712e60a15f1795878653eb5e0414e
> src/tests/containerizer/io_switchboard_tests.cpp d82f22b5c9a8d177577db3aeaae9e571e7e2fb80
>
> Diff: https://reviews.apache.org/r/54293/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 54293: Supported TTY in I/O switchboard.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54293/
-----------------------------------------------------------
(Updated Dec. 4, 2016, 9:21 p.m.)
Review request for mesos, Benjamin Hindman and Kevin Klues.
Changes
-------
Used well known numbers to pass FDs.
Bugs: MESOS-6470
https://issues.apache.org/jira/browse/MESOS-6470
Repository: mesos
Description
-------
Supported TTY in I/O switchboard.
Diffs (updated)
-----
include/mesos/slave/containerizer.proto 33b4c231cc88a048f3de8ab4b2e0e9eb2140132d
src/slave/containerizer/mesos/containerizer.cpp a7e2665d4c64b7b322d4ae8d5441e8e777236c59
src/slave/containerizer/mesos/io/switchboard.hpp 23c66bb2b05310a12fec24a5e5eebddd370ba0f0
src/slave/containerizer/mesos/io/switchboard.cpp 778367a268ec350ed438bc9fe9d359d63bdb5503
src/slave/containerizer/mesos/io/switchboard_main.cpp 5800217d3562a53218d966c57a14e8b768643c34
src/slave/containerizer/mesos/launch.cpp d78ca4df694712e60a15f1795878653eb5e0414e
src/tests/containerizer/io_switchboard_tests.cpp d82f22b5c9a8d177577db3aeaae9e571e7e2fb80
Diff: https://reviews.apache.org/r/54293/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 54293: Supported TTY in I/O switchboard.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54293/
-----------------------------------------------------------
(Updated Dec. 2, 2016, 6:29 p.m.)
Review request for mesos, Benjamin Hindman and Kevin Klues.
Bugs: MESOS-6470
https://issues.apache.org/jira/browse/MESOS-6470
Repository: mesos
Description
-------
Supported TTY in I/O switchboard.
Diffs
-----
include/mesos/slave/containerizer.proto 33b4c231cc88a048f3de8ab4b2e0e9eb2140132d
src/slave/containerizer/mesos/containerizer.cpp a7e2665d4c64b7b322d4ae8d5441e8e777236c59
src/slave/containerizer/mesos/io/switchboard.hpp 23c66bb2b05310a12fec24a5e5eebddd370ba0f0
src/slave/containerizer/mesos/io/switchboard.cpp 594c37e531c5b26599989a126aede56954d460fa
src/slave/containerizer/mesos/io/switchboard_main.cpp 5800217d3562a53218d966c57a14e8b768643c34
src/slave/containerizer/mesos/launch.cpp d78ca4df694712e60a15f1795878653eb5e0414e
src/tests/containerizer/io_switchboard_tests.cpp d82f22b5c9a8d177577db3aeaae9e571e7e2fb80
Diff: https://reviews.apache.org/r/54293/diff/
Testing
-------
make check
Thanks,
Jie Yu