-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59183/#review175132
-----------------------------------------------------------
src/slave/containerizer/mesos/launch.cpp
Lines 234-237 (patched)
<https://reviews.apache.org/r/59183/#comment248775>
I don't see a compelling reason to hoist this helper. We typically avoid hoisting a helper
if it's only used in one place. We typically prefer inlining it. Jumping around the code affects
readability.
I checked the following patches, looks like the additional logic is not that significant.
Therefore, I'd suggest we don't do this code movement.
src/slave/containerizer/mesos/launch.cpp
Lines 276 (patched)
<https://reviews.apache.org/r/59183/#comment248506>
2 lines apart
src/slave/containerizer/mesos/launch.cpp
Line 454 (original), 498 (patched)
<https://reviews.apache.org/r/59183/#comment248776>
In retrospect, this should probably be Option (instead of a Try). If you can follow up
with a patch to convert this into a Try, that'll be great!
- Jie Yu
On May 11, 2017, 4:45 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59183/
> -----------------------------------------------------------
>
> (Updated May 11, 2017, 4:45 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-7476
> https://issues.apache.org/jira/browse/MESOS-7476
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Setting the capabilities for the launched process will become a bit more
> involved in subsequent commits so hoist it into a helper function with
> no semantic changes to make the code review more obvious.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/launch.cpp 2835beff9dbfa7f2a1cac306a58e2b1d66c14342
>
>
> Diff: https://reviews.apache.org/r/59183/diff/1/
>
>
> Testing
> -------
>
> make check (Fedora 25).
>
>
> Thanks,
>
> James Peach
>
>
|