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/12/10 05:08:05 UTC
Review Request 54620: Removed "enable_io_switchboard_server" agent
flag.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54620/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar, Jie Yu, and Kevin Klues.
Repository: mesos
Description
-------
Instead of an agent wide flag now we automatically launch the server
for DEBUG and TTY containers.
Diffs
-----
src/slave/containerizer/mesos/io/switchboard.hpp ae4c48d3ce0252189bc1a098c26f25ba30c870c9
src/slave/containerizer/mesos/io/switchboard.cpp a354188a3656b5c2fd797b214f6c02714d70ce2c
src/slave/flags.hpp 02af2845e8c2baf03c83c7eab897725f6e2e9752
src/slave/flags.cpp 74a6c9936b29d35a72d47a5b7e9939c3e49fc7b3
src/tests/api_tests.cpp 7a220116aa871650e56221a4e81ccb65f4252a56
src/tests/containerizer/io_switchboard_tests.cpp 7a902622237108661107054f01fad27bf459fd72
src/tests/mesos.cpp 73f17a74170dfc81a7c8a38e67d9030c95f1d52c
Diff: https://reviews.apache.org/r/54620/diff/
Testing
-------
AttachContainerInput test currently fails. Need to fix before committing.
Thanks,
Vinod Kone
Re: Review Request 54620: Removed "enable_io_switchboard_server"
agent flag.
Posted by Kevin Klues <kl...@gmail.com>.
> On Dec. 10, 2016, 10:36 p.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/io/switchboard.hpp, line 155
> > <https://reviews.apache.org/r/54620/diff/1/?file=1581300#file1581300line155>
> >
> > Shouldn't this be called `isEnabled()`. When I see `enable()` this implies to me that I need to call this in order to enable the switchboard.
>
> Vinod Kone wrote:
> `isEnabled` sounds wrong too because that sounds like you are asking SB server whether it is enabled? what we are essentially asking is whether SB server should be enabled, i.e., `shouldBeEnabled()` or `needed()` or `isRequired()`. thoughts?
`isRequired()` sounds good.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54620/#review158776
-----------------------------------------------------------
On Dec. 10, 2016, 5:08 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54620/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2016, 5:08 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Jie Yu, and Kevin Klues.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Instead of an agent wide flag now we automatically launch the server
> for DEBUG and TTY containers.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.hpp ae4c48d3ce0252189bc1a098c26f25ba30c870c9
> src/slave/containerizer/mesos/io/switchboard.cpp a354188a3656b5c2fd797b214f6c02714d70ce2c
> src/slave/flags.hpp 02af2845e8c2baf03c83c7eab897725f6e2e9752
> src/slave/flags.cpp 74a6c9936b29d35a72d47a5b7e9939c3e49fc7b3
> src/tests/api_tests.cpp 7a220116aa871650e56221a4e81ccb65f4252a56
> src/tests/containerizer/io_switchboard_tests.cpp 7a902622237108661107054f01fad27bf459fd72
> src/tests/mesos.cpp 73f17a74170dfc81a7c8a38e67d9030c95f1d52c
>
> Diff: https://reviews.apache.org/r/54620/diff/
>
>
> Testing
> -------
>
> AttachContainerInput test currently fails. Need to fix before committing.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 54620: Removed "enable_io_switchboard_server"
agent flag.
Posted by Vinod Kone <vi...@gmail.com>.
> On Dec. 10, 2016, 10:36 p.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, lines 1476-1481
> > <https://reviews.apache.org/r/54620/diff/1/?file=1581301#file1581301line1476>
> >
> > Maybe this belongs in a separate patch, but we should only close stdinFd here if we are not a pty.
i'll do this in a subsequent review.
> On Dec. 10, 2016, 10:36 p.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/io/switchboard.hpp, line 155
> > <https://reviews.apache.org/r/54620/diff/1/?file=1581300#file1581300line155>
> >
> > Shouldn't this be called `isEnabled()`. When I see `enable()` this implies to me that I need to call this in order to enable the switchboard.
`isEnabled` sounds wrong too because that sounds like you are asking SB server whether it is enabled? what we are essentially asking is whether SB server should be enabled, i.e., `shouldBeEnabled()` or `needed()` or `isRequired()`. thoughts?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54620/#review158776
-----------------------------------------------------------
On Dec. 10, 2016, 5:08 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54620/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2016, 5:08 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Jie Yu, and Kevin Klues.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Instead of an agent wide flag now we automatically launch the server
> for DEBUG and TTY containers.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.hpp ae4c48d3ce0252189bc1a098c26f25ba30c870c9
> src/slave/containerizer/mesos/io/switchboard.cpp a354188a3656b5c2fd797b214f6c02714d70ce2c
> src/slave/flags.hpp 02af2845e8c2baf03c83c7eab897725f6e2e9752
> src/slave/flags.cpp 74a6c9936b29d35a72d47a5b7e9939c3e49fc7b3
> src/tests/api_tests.cpp 7a220116aa871650e56221a4e81ccb65f4252a56
> src/tests/containerizer/io_switchboard_tests.cpp 7a902622237108661107054f01fad27bf459fd72
> src/tests/mesos.cpp 73f17a74170dfc81a7c8a38e67d9030c95f1d52c
>
> Diff: https://reviews.apache.org/r/54620/diff/
>
>
> Testing
> -------
>
> AttachContainerInput test currently fails. Need to fix before committing.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 54620: Removed "enable_io_switchboard_server"
agent flag.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54620/#review158776
-----------------------------------------------------------
src/slave/containerizer/mesos/io/switchboard.hpp (line 155)
<https://reviews.apache.org/r/54620/#comment229577>
Shouldn't this be called `isEnabled()`. When I see `enable()` this implies to me that I need to call this in order to enable the switchboard.
src/slave/containerizer/mesos/io/switchboard.cpp (lines 1467 - 1472)
<https://reviews.apache.org/r/54620/#comment229578>
Maybe this belongs in a separate patch, but we should only close stdinFd here if we are not a pty.
- Kevin Klues
On Dec. 10, 2016, 5:08 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54620/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2016, 5:08 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Jie Yu, and Kevin Klues.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Instead of an agent wide flag now we automatically launch the server
> for DEBUG and TTY containers.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.hpp ae4c48d3ce0252189bc1a098c26f25ba30c870c9
> src/slave/containerizer/mesos/io/switchboard.cpp a354188a3656b5c2fd797b214f6c02714d70ce2c
> src/slave/flags.hpp 02af2845e8c2baf03c83c7eab897725f6e2e9752
> src/slave/flags.cpp 74a6c9936b29d35a72d47a5b7e9939c3e49fc7b3
> src/tests/api_tests.cpp 7a220116aa871650e56221a4e81ccb65f4252a56
> src/tests/containerizer/io_switchboard_tests.cpp 7a902622237108661107054f01fad27bf459fd72
> src/tests/mesos.cpp 73f17a74170dfc81a7c8a38e67d9030c95f1d52c
>
> Diff: https://reviews.apache.org/r/54620/diff/
>
>
> Testing
> -------
>
> AttachContainerInput test currently fails. Need to fix before committing.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 54620: Removed "enable_io_switchboard_server"
agent flag.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54620/#review158764
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On Dec. 10, 2016, 5:08 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54620/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2016, 5:08 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Jie Yu, and Kevin Klues.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Instead of an agent wide flag now we automatically launch the server
> for DEBUG and TTY containers.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.hpp ae4c48d3ce0252189bc1a098c26f25ba30c870c9
> src/slave/containerizer/mesos/io/switchboard.cpp a354188a3656b5c2fd797b214f6c02714d70ce2c
> src/slave/flags.hpp 02af2845e8c2baf03c83c7eab897725f6e2e9752
> src/slave/flags.cpp 74a6c9936b29d35a72d47a5b7e9939c3e49fc7b3
> src/tests/api_tests.cpp 7a220116aa871650e56221a4e81ccb65f4252a56
> src/tests/containerizer/io_switchboard_tests.cpp 7a902622237108661107054f01fad27bf459fd72
> src/tests/mesos.cpp 73f17a74170dfc81a7c8a38e67d9030c95f1d52c
>
> Diff: https://reviews.apache.org/r/54620/diff/
>
>
> Testing
> -------
>
> AttachContainerInput test currently fails. Need to fix before committing.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 54620: Removed "enable_io_switchboard_server"
agent flag.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54620/
-----------------------------------------------------------
(Updated Dec. 11, 2016, 4:39 a.m.)
Review request for mesos, Anand Mazumdar, Jie Yu, and Kevin Klues.
Changes
-------
kevin's comments.
also updated the test to send "lorem ipsum.." chunk just once because sending large amounts of data to `cat` in terminal mode needs some workarounds.
Repository: mesos
Description
-------
Instead of an agent wide flag now we automatically launch the server
for DEBUG and TTY containers.
Diffs (updated)
-----
src/slave/containerizer/mesos/io/switchboard.hpp ae4c48d3ce0252189bc1a098c26f25ba30c870c9
src/slave/containerizer/mesos/io/switchboard.cpp a354188a3656b5c2fd797b214f6c02714d70ce2c
src/slave/flags.hpp 02af2845e8c2baf03c83c7eab897725f6e2e9752
src/slave/flags.cpp 74a6c9936b29d35a72d47a5b7e9939c3e49fc7b3
src/tests/api_tests.cpp 7a220116aa871650e56221a4e81ccb65f4252a56
src/tests/containerizer/io_switchboard_tests.cpp 7a902622237108661107054f01fad27bf459fd72
src/tests/mesos.cpp 73f17a74170dfc81a7c8a38e67d9030c95f1d52c
Diff: https://reviews.apache.org/r/54620/diff/
Testing (updated)
-------
make check
Thanks,
Vinod Kone