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