mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 59183: Refactor setting capabilities into a helper function.
Date Wed, 17 May 2017 22:11:30 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message