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/11/30 02:07:48 UTC
Review Request 54192: Made IOSwitchboard an isolator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54192/
-----------------------------------------------------------
Review request for mesos and Kevin Klues.
Bugs: MESOS-6651
https://issues.apache.org/jira/browse/MESOS-6651
Repository: mesos
Description
-------
Made IOSwitchboard an isolator.
Diffs
-----
include/mesos/slave/container_logger.hpp 43bd4b6b0365f67234477f8ed51024f342fa271f
include/mesos/slave/containerizer.proto cdb125a631c22d2e015b8ead421f13611ea56184
src/slave/containerizer/mesos/containerizer.hpp 45986c57e5d5b85ec153bd0380f57ebdf33c4f30
src/slave/containerizer/mesos/containerizer.cpp 4a03f97d10a1b34a0a13600af5ac6fc3679694fd
src/slave/containerizer/mesos/io/switchboard.hpp aaa3a35245b291f6003f519dbf8c0e1b82bc15fd
src/slave/containerizer/mesos/io/switchboard.cpp 25cbf2447d197134f0753b062b6f4130821005b2
src/tests/containerizer/docker_volume_isolator_tests.cpp 7c0349e423396b337b1e9cbed0d0073babcba79d
src/tests/containerizer/mesos_containerizer_tests.cpp 712bf750349d38ef30542b32358556c377ed602a
Diff: https://reviews.apache.org/r/54192/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 54192: Made IOSwitchboard an isolator.
Posted by Jie Yu <yu...@gmail.com>.
> On Nov. 30, 2016, 3:36 a.m., Kevin Klues wrote:
> > include/mesos/slave/container_logger.hpp, lines 121-123
> > <https://reviews.apache.org/r/54192/diff/1/?file=1572703#file1572703line121>
> >
> > I haven't usually seen member variables ending in `_` like this. Is this a standard pattern?
Yeah, i used that in a couple of places. Basically the google style.
> On Nov. 30, 2016, 3:36 a.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/io/switchboard.hpp, line 81
> > <https://reviews.apache.org/r/54192/diff/1/?file=1572707#file1572707line81>
> >
> > Why remove the `const` `&` here?
because ownership is supposed to be transferred here. Though our owned pointer is actually shared pointer
> On Nov. 30, 2016, 3:36 a.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, lines 138-140
> > <https://reviews.apache.org/r/54192/diff/1/?file=1572708#file1572708line138>
> >
> > Just curious, why is `_prepare()` preferred over a simple lambda here (as it was before)?
I prefer that if the lambda is too big.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54192/#review157361
-----------------------------------------------------------
On Nov. 30, 2016, 2:07 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54192/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2016, 2:07 a.m.)
>
>
> Review request for mesos and Kevin Klues.
>
>
> Bugs: MESOS-6651
> https://issues.apache.org/jira/browse/MESOS-6651
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made IOSwitchboard an isolator.
>
>
> Diffs
> -----
>
> include/mesos/slave/container_logger.hpp 43bd4b6b0365f67234477f8ed51024f342fa271f
> include/mesos/slave/containerizer.proto cdb125a631c22d2e015b8ead421f13611ea56184
> src/slave/containerizer/mesos/containerizer.hpp 45986c57e5d5b85ec153bd0380f57ebdf33c4f30
> src/slave/containerizer/mesos/containerizer.cpp 4a03f97d10a1b34a0a13600af5ac6fc3679694fd
> src/slave/containerizer/mesos/io/switchboard.hpp aaa3a35245b291f6003f519dbf8c0e1b82bc15fd
> src/slave/containerizer/mesos/io/switchboard.cpp 25cbf2447d197134f0753b062b6f4130821005b2
> src/tests/containerizer/docker_volume_isolator_tests.cpp 7c0349e423396b337b1e9cbed0d0073babcba79d
> src/tests/containerizer/mesos_containerizer_tests.cpp 712bf750349d38ef30542b32358556c377ed602a
>
> Diff: https://reviews.apache.org/r/54192/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 54192: Made IOSwitchboard an isolator.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54192/#review157361
-----------------------------------------------------------
Fix it, then Ship it!
Looks good overall. I'd probably add something to the commit message talking about how we currently don't implement recovery and watch, but we plan to add those in a subsequent commit.
include/mesos/slave/container_logger.hpp (lines 121 - 123)
<https://reviews.apache.org/r/54192/#comment227924>
I haven't usually seen member variables ending in `_` like this. Is this a standard pattern?
src/slave/containerizer/mesos/io/switchboard.hpp (line 66)
<https://reviews.apache.org/r/54192/#comment227927>
Why remove the `const` `&` here?
src/slave/containerizer/mesos/io/switchboard.cpp (lines 99 - 101)
<https://reviews.apache.org/r/54192/#comment227928>
Just curious, why is `_prepare()` preferred over a simple lambda here (as it was before)?
- Kevin Klues
On Nov. 30, 2016, 2:07 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54192/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2016, 2:07 a.m.)
>
>
> Review request for mesos and Kevin Klues.
>
>
> Bugs: MESOS-6651
> https://issues.apache.org/jira/browse/MESOS-6651
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made IOSwitchboard an isolator.
>
>
> Diffs
> -----
>
> include/mesos/slave/container_logger.hpp 43bd4b6b0365f67234477f8ed51024f342fa271f
> include/mesos/slave/containerizer.proto cdb125a631c22d2e015b8ead421f13611ea56184
> src/slave/containerizer/mesos/containerizer.hpp 45986c57e5d5b85ec153bd0380f57ebdf33c4f30
> src/slave/containerizer/mesos/containerizer.cpp 4a03f97d10a1b34a0a13600af5ac6fc3679694fd
> src/slave/containerizer/mesos/io/switchboard.hpp aaa3a35245b291f6003f519dbf8c0e1b82bc15fd
> src/slave/containerizer/mesos/io/switchboard.cpp 25cbf2447d197134f0753b062b6f4130821005b2
> src/tests/containerizer/docker_volume_isolator_tests.cpp 7c0349e423396b337b1e9cbed0d0073babcba79d
> src/tests/containerizer/mesos_containerizer_tests.cpp 712bf750349d38ef30542b32358556c377ed602a
>
> Diff: https://reviews.apache.org/r/54192/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>